Re: [PATCH v3] misc: bcm-vk: only support ttyVK if CONFIG_TTY is set

[Scott Branden]

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@xxxxxxxxxxxxx>
>> Signed-off-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
>>
>> ---
>> 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