Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Arnd Bergmann <arnd@arndb.de>, Kees Cook <keescook@chromium.org>,
        linux-kernel@vger.kernel.org,
        bcm-kernel-feedback-list@broadcom.com,
        Olof Johansson <olof@lixom.net>,
        Desmond Yan <desmond.yan@broadcom.com>
References: <20210131233049.5500-1-scott.branden@broadcom.com>
 <YBexqH9KZEMs2fq0@kroah.com>
From:   Scott Branden <scott.branden@broadcom.com>
Message-ID: <30098c21-f0aa-55f3-8e1f-c7bb8ae8be0e@broadcom.com>
Date:   Mon, 1 Feb 2021 10:13:55 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <YBexqH9KZEMs2fq0@kroah.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-CA
Precedence: bulk

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