lkml.org 
[lkml]   [2021]   [Feb]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set
    From
    Date
    Hi Greg,,

    I need a few clarifications before sending (hopefully) final revisions to the patch.

    On 2021-01-31 11:45 p.m., Greg Kroah-Hartman wrote:
    > On Sun, Jan 31, 2021 at 03:30:49PM -0800, Scott Branden wrote:
    >> Correct compile issue if CONFIG_TTY is not set by
    >> only adding ttyVK devices if CONFIG_BCM_VK_TTY is set.
    >>
    >> Reported-by: Randy Dunlap <rdunlap@infradead.org>
    >> Signed-off-by: Scott Branden <scott.branden@broadcom.com>
    >>
    >> ---
    >> Changes since v2:
    >> - add CONFIG_BCM_VK_TTY
    >> - add function and stub for bcm_vk_tty_set_irq_enabled
    >> Changes since v1:
    >> - add function stubs rather than compiling out code
    >> ---
    >> drivers/misc/bcm-vk/Kconfig | 16 ++++++++++++
    >> drivers/misc/bcm-vk/Makefile | 4 +--
    >> drivers/misc/bcm-vk/bcm_vk.h | 42 +++++++++++++++++++++++++++++---
    >> drivers/misc/bcm-vk/bcm_vk_dev.c | 5 ++--
    >> drivers/misc/bcm-vk/bcm_vk_tty.c | 6 +++++
    >> 5 files changed, 65 insertions(+), 8 deletions(-)
    >>
    >> diff --git a/drivers/misc/bcm-vk/Kconfig b/drivers/misc/bcm-vk/Kconfig
    >> index 052f6f28b540..16ce98c964b8 100644
    >> --- a/drivers/misc/bcm-vk/Kconfig
    >> +++ b/drivers/misc/bcm-vk/Kconfig
    >> @@ -15,3 +15,19 @@ config BCM_VK
    >> accelerators via /dev/bcm-vk.N devices.
    >>
    >> If unsure, say N.
    >> +
    >> +if BCM_VK
    > No need for this, just put it on the depends line, right?
    If you prefer I can but it on the depends on line.
    But, I actually prefer the if syntax in this case as it more clearly shows
    BCM_VK_TTY is a suboption of BCM_VK.

    Please let me know which method is "right"?
    >
    >> +
    >> +config BCM_VK_TTY
    >> + bool "Enable ttyVK"
    > Better config help text to explain what this is?
    I'll change it to the following?

    "Enable tty's on VK devices"
    >
    >> + depends on TTY
    >> + default y
    > Default y is only there if your system can not boot without it, please
    > remove it.
    I can remove if really needed but I'd like to learn more about such convention.
    Is there a document I can learn from describing such?

    We actually want a full featured driver by default.  Otherwise we'll end up asking people to enable this
    feature and recompile the driver to get missing features such as this.
    >
    >> + help
    >> + Select this option to enable ttyVK support to allow console
    >> + access to VK cards from host.
    > Again, more help text, what is a "VK"?
    VK is already described in BCM_VK.  Why would I need to add the same information again to a suboption?
    Perhaps you would like "config BCM_VK" changed to "menuconfig BCM_VK"
    >
    > thanks,
    >
    > greg k-h
    Thanks,
     Scott

    \
     
     \ /
      Last update: 2021-02-01 19:19    [W:8.451 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site