* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
@ 2019-11-04 10:04 ` Thierry Reding
0 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2019-11-04 10:04 UTC (permalink / raw)
To: Mao Wenan
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 1853 bytes --]
On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> below erros can be seen:
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
>
> This patch add dependency USB_ROLE_SWITCH for UDC driver.
>
> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> drivers/usb/gadget/udc/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index acaec3a..d103154 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
> tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
> depends on ARCH_TEGRA || COMPILE_TEST
> depends on PHY_TEGRA_XUSB
> + depends on USB_ROLE_SWITCH
It looks like most other drivers that use the USB role switch class do
"select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
visible symbol, which can lead to conflicts, so it should be avoided. I
think that in this case it might make sense to hide USB_ROLE_SWITCH and
then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
itself. It always needs a host and/or gadget driver to make use of it.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
2019-11-04 10:04 ` Thierry Reding
@ 2019-11-04 10:50 ` maowenan
-1 siblings, 0 replies; 24+ messages in thread
From: maowenan @ 2019-11-04 10:50 UTC (permalink / raw)
To: Thierry Reding
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
On 2019/11/4 18:04, Thierry Reding wrote:
> On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
>> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
>> below erros can be seen:
>> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
>> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
>> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
>> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
>> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
>> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
>>
>> This patch add dependency USB_ROLE_SWITCH for UDC driver.
>>
>> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>> drivers/usb/gadget/udc/Kconfig | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>> index acaec3a..d103154 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
>> tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
>> depends on ARCH_TEGRA || COMPILE_TEST
>> depends on PHY_TEGRA_XUSB
>> + depends on USB_ROLE_SWITCH
>
> It looks like most other drivers that use the USB role switch class do
> "select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
> visible symbol, which can lead to conflicts, so it should be avoided. I
> think that in this case it might make sense to hide USB_ROLE_SWITCH and
> then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
> USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
> itself. It always needs a host and/or gadget driver to make use of it.
>
Thanks, I send v2 and change 'depends on' to 'select' for this patch.
> Thierry
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
@ 2019-11-04 10:50 ` maowenan
0 siblings, 0 replies; 24+ messages in thread
From: maowenan @ 2019-11-04 10:50 UTC (permalink / raw)
To: Thierry Reding
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
On 2019/11/4 18:04, Thierry Reding wrote:
> On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
>> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
>> below erros can be seen:
>> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
>> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
>> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
>> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
>> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
>> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
>>
>> This patch add dependency USB_ROLE_SWITCH for UDC driver.
>>
>> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>> ---
>> drivers/usb/gadget/udc/Kconfig | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
>> index acaec3a..d103154 100644
>> --- a/drivers/usb/gadget/udc/Kconfig
>> +++ b/drivers/usb/gadget/udc/Kconfig
>> @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
>> tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
>> depends on ARCH_TEGRA || COMPILE_TEST
>> depends on PHY_TEGRA_XUSB
>> + depends on USB_ROLE_SWITCH
>
> It looks like most other drivers that use the USB role switch class do
> "select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
> visible symbol, which can lead to conflicts, so it should be avoided. I
> think that in this case it might make sense to hide USB_ROLE_SWITCH and
> then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
> USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
> itself. It always needs a host and/or gadget driver to make use of it.
>
Thanks, I send v2 and change 'depends on' to 'select' for this patch.
> Thierry
>
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH v2 -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
2019-11-04 10:50 ` maowenan
@ 2019-11-04 11:21 ` Mao Wenan
-1 siblings, 0 replies; 24+ messages in thread
From: Mao Wenan @ 2019-11-04 11:21 UTC (permalink / raw)
To: felipe.balbi, gregkh, treding, nkristam, arnd, johan, maowenan,
krzk
Cc: linux-usb, linux-kernel, kernel-janitors
If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
below erros can be seen:
drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
It should select USB_ROLE_SWITCH for UDC driver.
Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
v2: change 'depends on' to 'select'.
drivers/usb/gadget/udc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index acaec3a..d103154 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
depends on ARCH_TEGRA || COMPILE_TEST
depends on PHY_TEGRA_XUSB
+ select USB_ROLE_SWITCH
help
Enables NVIDIA Tegra USB 3.0 device mode controller driver.
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
@ 2019-11-04 11:21 ` Mao Wenan
0 siblings, 0 replies; 24+ messages in thread
From: Mao Wenan @ 2019-11-04 11:21 UTC (permalink / raw)
To: felipe.balbi, gregkh, treding, nkristam, arnd, johan, maowenan,
krzk
Cc: linux-usb, linux-kernel, kernel-janitors
If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
below erros can be seen:
drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
It should select USB_ROLE_SWITCH for UDC driver.
Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
v2: change 'depends on' to 'select'.
drivers/usb/gadget/udc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
index acaec3a..d103154 100644
--- a/drivers/usb/gadget/udc/Kconfig
+++ b/drivers/usb/gadget/udc/Kconfig
@@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
depends on ARCH_TEGRA || COMPILE_TEST
depends on PHY_TEGRA_XUSB
+ select USB_ROLE_SWITCH
help
Enables NVIDIA Tegra USB 3.0 device mode controller driver.
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
2019-11-04 11:21 ` Mao Wenan
@ 2019-11-04 13:52 ` Thierry Reding
-1 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2019-11-04 13:52 UTC (permalink / raw)
To: Mao Wenan
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 999 bytes --]
On Mon, Nov 04, 2019 at 07:21:04PM +0800, Mao Wenan wrote:
> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> below erros can be seen:
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
>
> It should select USB_ROLE_SWITCH for UDC driver.
>
> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> v2: change 'depends on' to 'select'.
> drivers/usb/gadget/udc/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
@ 2019-11-04 13:52 ` Thierry Reding
0 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2019-11-04 13:52 UTC (permalink / raw)
To: Mao Wenan
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 999 bytes --]
On Mon, Nov 04, 2019 at 07:21:04PM +0800, Mao Wenan wrote:
> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> below erros can be seen:
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
>
> It should select USB_ROLE_SWITCH for UDC driver.
>
> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> v2: change 'depends on' to 'select'.
> drivers/usb/gadget/udc/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
Acked-by: Thierry Reding <treding@nvidia.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
2019-11-04 10:50 ` maowenan
@ 2019-11-04 13:53 ` Thierry Reding
-1 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2019-11-04 13:53 UTC (permalink / raw)
To: maowenan
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]
On Mon, Nov 04, 2019 at 06:50:01PM +0800, maowenan wrote:
>
>
> On 2019/11/4 18:04, Thierry Reding wrote:
> > On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
> >> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> >> below erros can be seen:
> >> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> >> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> >> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> >> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> >> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> >> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
> >>
> >> This patch add dependency USB_ROLE_SWITCH for UDC driver.
> >>
> >> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> >> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> >> ---
> >> drivers/usb/gadget/udc/Kconfig | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> >> index acaec3a..d103154 100644
> >> --- a/drivers/usb/gadget/udc/Kconfig
> >> +++ b/drivers/usb/gadget/udc/Kconfig
> >> @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
> >> tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
> >> depends on ARCH_TEGRA || COMPILE_TEST
> >> depends on PHY_TEGRA_XUSB
> >> + depends on USB_ROLE_SWITCH
> >
> > It looks like most other drivers that use the USB role switch class do
> > "select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
> > visible symbol, which can lead to conflicts, so it should be avoided. I
> > think that in this case it might make sense to hide USB_ROLE_SWITCH and
> > then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
> > USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
> > itself. It always needs a host and/or gadget driver to make use of it.
> >
>
> Thanks, I send v2 and change 'depends on' to 'select' for this patch.
Great, can you also follow up with a patch to hide the USB_ROLE_SWITCH
option so that we can avoid any of the pitfalls associated with user-
visible symbols and "select"?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
@ 2019-11-04 13:53 ` Thierry Reding
0 siblings, 0 replies; 24+ messages in thread
From: Thierry Reding @ 2019-11-04 13:53 UTC (permalink / raw)
To: maowenan
Cc: felipe.balbi, gregkh, nkristam, arnd, johan, krzk, linux-usb,
linux-kernel, kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]
On Mon, Nov 04, 2019 at 06:50:01PM +0800, maowenan wrote:
>
>
> On 2019/11/4 18:04, Thierry Reding wrote:
> > On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
> >> If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> >> below erros can be seen:
> >> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> >> tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> >> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> >> tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> >> drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> >> tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
> >>
> >> This patch add dependency USB_ROLE_SWITCH for UDC driver.
> >>
> >> Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> >> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> >> ---
> >> drivers/usb/gadget/udc/Kconfig | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> >> index acaec3a..d103154 100644
> >> --- a/drivers/usb/gadget/udc/Kconfig
> >> +++ b/drivers/usb/gadget/udc/Kconfig
> >> @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
> >> tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
> >> depends on ARCH_TEGRA || COMPILE_TEST
> >> depends on PHY_TEGRA_XUSB
> >> + depends on USB_ROLE_SWITCH
> >
> > It looks like most other drivers that use the USB role switch class do
> > "select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
> > visible symbol, which can lead to conflicts, so it should be avoided. I
> > think that in this case it might make sense to hide USB_ROLE_SWITCH and
> > then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
> > USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
> > itself. It always needs a host and/or gadget driver to make use of it.
> >
>
> Thanks, I send v2 and change 'depends on' to 'select' for this patch.
Great, can you also follow up with a patch to hide the USB_ROLE_SWITCH
option so that we can avoid any of the pitfalls associated with user-
visible symbols and "select"?
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
2019-11-04 13:53 ` Thierry Reding
@ 2019-11-04 14:48 ` Mao Wenan
-1 siblings, 0 replies; 24+ messages in thread
From: Mao Wenan @ 2019-11-04 14:48 UTC (permalink / raw)
To: gregkh, heikki.krogerus, maowenan
Cc: linux-usb, linux-kernel, kernel-janitors
The USB role switch class is, after all,
not useful by itself. Hiding USB_ROLE_SWITCH
so we can avoid any of the pitfalls associated
with user-visible symbols and "select".
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/usb/roles/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
index f8b31aa..1da58d4 100644
--- a/drivers/usb/roles/Kconfig
+++ b/drivers/usb/roles/Kconfig
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
config USB_ROLE_SWITCH
- tristate "USB Role Switch Support"
+ tristate
help
USB Role Switch is a device that can select the USB role - host or
device - for a USB port (connector). In most cases dual-role capable
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
@ 2019-11-04 14:48 ` Mao Wenan
0 siblings, 0 replies; 24+ messages in thread
From: Mao Wenan @ 2019-11-04 14:48 UTC (permalink / raw)
To: gregkh, heikki.krogerus, maowenan
Cc: linux-usb, linux-kernel, kernel-janitors
The USB role switch class is, after all,
not useful by itself. Hiding USB_ROLE_SWITCH
so we can avoid any of the pitfalls associated
with user-visible symbols and "select".
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/usb/roles/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
index f8b31aa..1da58d4 100644
--- a/drivers/usb/roles/Kconfig
+++ b/drivers/usb/roles/Kconfig
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
config USB_ROLE_SWITCH
- tristate "USB Role Switch Support"
+ tristate
help
USB Role Switch is a device that can select the USB role - host or
device - for a USB port (connector). In most cases dual-role capable
--
2.7.4
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
2019-11-04 14:48 ` Mao Wenan
@ 2019-11-05 12:42 ` Heikki Krogerus
-1 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-11-05 12:42 UTC (permalink / raw)
To: Mao Wenan; +Cc: gregkh, linux-usb, linux-kernel, kernel-janitors
On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> The USB role switch class is, after all,
> not useful by itself. Hiding USB_ROLE_SWITCH
> so we can avoid any of the pitfalls associated
> with user-visible symbols and "select".
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> drivers/usb/roles/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> index f8b31aa..1da58d4 100644
> --- a/drivers/usb/roles/Kconfig
> +++ b/drivers/usb/roles/Kconfig
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
> config USB_ROLE_SWITCH
> - tristate "USB Role Switch Support"
> + tristate
> help
> USB Role Switch is a device that can select the USB role - host or
> device - for a USB port (connector). In most cases dual-role capable
You didn't actually convert the "depends on USB_ROLE_SWTICH" to
"select USB_ROLE_SWITCH" before this. You also left the help text that
is now useless.
I really think that instead of this, we should just convert all
"select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
thanks,
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
@ 2019-11-05 12:42 ` Heikki Krogerus
0 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-11-05 12:42 UTC (permalink / raw)
To: Mao Wenan; +Cc: gregkh, linux-usb, linux-kernel, kernel-janitors
On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> The USB role switch class is, after all,
> not useful by itself. Hiding USB_ROLE_SWITCH
> so we can avoid any of the pitfalls associated
> with user-visible symbols and "select".
>
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
> ---
> drivers/usb/roles/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> index f8b31aa..1da58d4 100644
> --- a/drivers/usb/roles/Kconfig
> +++ b/drivers/usb/roles/Kconfig
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
>
> config USB_ROLE_SWITCH
> - tristate "USB Role Switch Support"
> + tristate
> help
> USB Role Switch is a device that can select the USB role - host or
> device - for a USB port (connector). In most cases dual-role capable
You didn't actually convert the "depends on USB_ROLE_SWTICH" to
"select USB_ROLE_SWITCH" before this. You also left the help text that
is now useless.
I really think that instead of this, we should just convert all
"select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
thanks,
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
2019-11-05 12:42 ` Heikki Krogerus
@ 2019-11-05 13:16 ` Dan Carpenter
-1 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2019-11-05 13:16 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Mao Wenan, gregkh, linux-usb, linux-kernel, kernel-janitors
On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > The USB role switch class is, after all,
> > not useful by itself. Hiding USB_ROLE_SWITCH
> > so we can avoid any of the pitfalls associated
> > with user-visible symbols and "select".
> >
> > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > ---
> > drivers/usb/roles/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > index f8b31aa..1da58d4 100644
> > --- a/drivers/usb/roles/Kconfig
> > +++ b/drivers/usb/roles/Kconfig
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> > config USB_ROLE_SWITCH
> > - tristate "USB Role Switch Support"
> > + tristate
> > help
> > USB Role Switch is a device that can select the USB role - host or
> > device - for a USB port (connector). In most cases dual-role capable
>
> You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> "select USB_ROLE_SWITCH" before this. You also left the help text that
> is now useless.
>
> I really think that instead of this, we should just convert all
> "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
The you have to find USB_ROLE_SWITCH first when you want to enable your
hardware... It's feels really confusing when you want to create a
.config file...
I sometimes think maybe I'm too stupid to configure a kernel these days
and that's sort of sad because how is Aunt Tillie supposed to manage?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
@ 2019-11-05 13:16 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2019-11-05 13:16 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Mao Wenan, gregkh, linux-usb, linux-kernel, kernel-janitors
On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > The USB role switch class is, after all,
> > not useful by itself. Hiding USB_ROLE_SWITCH
> > so we can avoid any of the pitfalls associated
> > with user-visible symbols and "select".
> >
> > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > ---
> > drivers/usb/roles/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > index f8b31aa..1da58d4 100644
> > --- a/drivers/usb/roles/Kconfig
> > +++ b/drivers/usb/roles/Kconfig
> > @@ -1,7 +1,7 @@
> > # SPDX-License-Identifier: GPL-2.0
> >
> > config USB_ROLE_SWITCH
> > - tristate "USB Role Switch Support"
> > + tristate
> > help
> > USB Role Switch is a device that can select the USB role - host or
> > device - for a USB port (connector). In most cases dual-role capable
>
> You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> "select USB_ROLE_SWITCH" before this. You also left the help text that
> is now useless.
>
> I really think that instead of this, we should just convert all
> "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
The you have to find USB_ROLE_SWITCH first when you want to enable your
hardware... It's feels really confusing when you want to create a
.config file...
I sometimes think maybe I'm too stupid to configure a kernel these days
and that's sort of sad because how is Aunt Tillie supposed to manage?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
2019-11-05 13:16 ` Dan Carpenter
@ 2019-11-05 15:26 ` Heikki Krogerus
-1 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-11-05 15:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Mao Wenan, gregkh, linux-usb, linux-kernel, kernel-janitors
Hi Dan,
On Tue, Nov 05, 2019 at 04:16:05PM +0300, Dan Carpenter wrote:
> On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> > On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > > The USB role switch class is, after all,
> > > not useful by itself. Hiding USB_ROLE_SWITCH
> > > so we can avoid any of the pitfalls associated
> > > with user-visible symbols and "select".
> > >
> > > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > > ---
> > > drivers/usb/roles/Kconfig | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > > index f8b31aa..1da58d4 100644
> > > --- a/drivers/usb/roles/Kconfig
> > > +++ b/drivers/usb/roles/Kconfig
> > > @@ -1,7 +1,7 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > >
> > > config USB_ROLE_SWITCH
> > > - tristate "USB Role Switch Support"
> > > + tristate
> > > help
> > > USB Role Switch is a device that can select the USB role - host or
> > > device - for a USB port (connector). In most cases dual-role capable
> >
> > You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> > "select USB_ROLE_SWITCH" before this. You also left the help text that
> > is now useless.
> >
> > I really think that instead of this, we should just convert all
> > "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
>
> The you have to find USB_ROLE_SWITCH first when you want to enable your
> hardware... It's feels really confusing when you want to create a
> .config file...
Unfortunately selecting the class alone is not enough. The USB role
switch on the system may be a dual-role capable USB controller, but it
may also be a mux that has its own separate driver.
It's equally or even more confusing for the user if the USB drivers
are enabled, including the dual-role mode, but the connector still
works only in one role, or in worst case not at all (if there is no
mux driver and the mux is left in "safe mode" so that the pins on the
connector are not connected to anything).
I still think that we should make these drivers depend on the class
instead of just selecting it. That way we at least give the user a
hint that there are also separate USB role switch drivers that may be
needed.
> I sometimes think maybe I'm too stupid to configure a kernel these days
> and that's sort of sad because how is Aunt Tillie supposed to manage?
We can always use something like conditional comments in the
Kconfig files to make sure that the user is told that in order to
select the driver, a dependency must be satisfied:
config MY_AWESOME_DRIVER
tristate "My awesome driver!"
depends on USB_ROLE_SWITCH
help
That's right! IT REALLY IS AWESOME!
comment "My awesome driver depends on USB_ROLE_SWITCH..."
depends on USB_ROLE_SWITCH=n
thanks,
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
@ 2019-11-05 15:26 ` Heikki Krogerus
0 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-11-05 15:26 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Mao Wenan, gregkh, linux-usb, linux-kernel, kernel-janitors
Hi Dan,
On Tue, Nov 05, 2019 at 04:16:05PM +0300, Dan Carpenter wrote:
> On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> > On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > > The USB role switch class is, after all,
> > > not useful by itself. Hiding USB_ROLE_SWITCH
> > > so we can avoid any of the pitfalls associated
> > > with user-visible symbols and "select".
> > >
> > > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > > ---
> > > drivers/usb/roles/Kconfig | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > > index f8b31aa..1da58d4 100644
> > > --- a/drivers/usb/roles/Kconfig
> > > +++ b/drivers/usb/roles/Kconfig
> > > @@ -1,7 +1,7 @@
> > > # SPDX-License-Identifier: GPL-2.0
> > >
> > > config USB_ROLE_SWITCH
> > > - tristate "USB Role Switch Support"
> > > + tristate
> > > help
> > > USB Role Switch is a device that can select the USB role - host or
> > > device - for a USB port (connector). In most cases dual-role capable
> >
> > You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> > "select USB_ROLE_SWITCH" before this. You also left the help text that
> > is now useless.
> >
> > I really think that instead of this, we should just convert all
> > "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
>
> The you have to find USB_ROLE_SWITCH first when you want to enable your
> hardware... It's feels really confusing when you want to create a
> .config file...
Unfortunately selecting the class alone is not enough. The USB role
switch on the system may be a dual-role capable USB controller, but it
may also be a mux that has its own separate driver.
It's equally or even more confusing for the user if the USB drivers
are enabled, including the dual-role mode, but the connector still
works only in one role, or in worst case not at all (if there is no
mux driver and the mux is left in "safe mode" so that the pins on the
connector are not connected to anything).
I still think that we should make these drivers depend on the class
instead of just selecting it. That way we at least give the user a
hint that there are also separate USB role switch drivers that may be
needed.
> I sometimes think maybe I'm too stupid to configure a kernel these days
> and that's sort of sad because how is Aunt Tillie supposed to manage?
We can always use something like conditional comments in the
Kconfig files to make sure that the user is told that in order to
select the driver, a dependency must be satisfied:
config MY_AWESOME_DRIVER
tristate "My awesome driver!"
depends on USB_ROLE_SWITCH
help
That's right! IT REALLY IS AWESOME!
comment "My awesome driver depends on USB_ROLE_SWITCH..."
depends on USB_ROLE_SWITCH=n
thanks,
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
2019-11-05 15:26 ` Heikki Krogerus
@ 2019-11-06 11:23 ` Dan Carpenter
-1 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2019-11-06 11:23 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Mao Wenan, gregkh, linux-usb, linux-kernel, kernel-janitors
On Tue, Nov 05, 2019 at 05:26:24PM +0200, Heikki Krogerus wrote:
> Hi Dan,
>
> On Tue, Nov 05, 2019 at 04:16:05PM +0300, Dan Carpenter wrote:
> > On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> > > On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > > > The USB role switch class is, after all,
> > > > not useful by itself. Hiding USB_ROLE_SWITCH
> > > > so we can avoid any of the pitfalls associated
> > > > with user-visible symbols and "select".
> > > >
> > > > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > > > ---
> > > > drivers/usb/roles/Kconfig | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > > > index f8b31aa..1da58d4 100644
> > > > --- a/drivers/usb/roles/Kconfig
> > > > +++ b/drivers/usb/roles/Kconfig
> > > > @@ -1,7 +1,7 @@
> > > > # SPDX-License-Identifier: GPL-2.0
> > > >
> > > > config USB_ROLE_SWITCH
> > > > - tristate "USB Role Switch Support"
> > > > + tristate
> > > > help
> > > > USB Role Switch is a device that can select the USB role - host or
> > > > device - for a USB port (connector). In most cases dual-role capable
> > >
> > > You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> > > "select USB_ROLE_SWITCH" before this. You also left the help text that
> > > is now useless.
> > >
> > > I really think that instead of this, we should just convert all
> > > "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
> >
> > The you have to find USB_ROLE_SWITCH first when you want to enable your
> > hardware... It's feels really confusing when you want to create a
> > .config file...
>
> Unfortunately selecting the class alone is not enough. The USB role
> switch on the system may be a dual-role capable USB controller, but it
> may also be a mux that has its own separate driver.
>
> It's equally or even more confusing for the user if the USB drivers
> are enabled, including the dual-role mode, but the connector still
> works only in one role, or in worst case not at all (if there is no
> mux driver and the mux is left in "safe mode" so that the pins on the
> connector are not connected to anything).
>
> I still think that we should make these drivers depend on the class
> instead of just selecting it. That way we at least give the user a
> hint that there are also separate USB role switch drivers that may be
> needed.
>
I guess I see your point. My problem then is just with the menuconfig
system, not really with this patch.
> > I sometimes think maybe I'm too stupid to configure a kernel these days
> > and that's sort of sad because how is Aunt Tillie supposed to manage?
>
> We can always use something like conditional comments in the
> Kconfig files to make sure that the user is told that in order to
> select the driver, a dependency must be satisfied:
>
> config MY_AWESOME_DRIVER
> tristate "My awesome driver!"
> depends on USB_ROLE_SWITCH
> help
> That's right! IT REALLY IS AWESOME!
>
> comment "My awesome driver depends on USB_ROLE_SWITCH..."
> depends on USB_ROLE_SWITCH=n
>
Those sorts of help don't show up unless you already meet the
dependencies or if you search for it. And if you search for it then it
already lists the depnds so the comment isn't required...
I guess that in the olden days you used to go through the menus and
look at all the high level options and figure it out. These days you
sort of have to know what you want already. And then you use the search
feature to enable it.
The menuconfig system really is very broken. I've looked at alternative
ways to do it, but it quite a bit of work involved...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH
@ 2019-11-06 11:23 ` Dan Carpenter
0 siblings, 0 replies; 24+ messages in thread
From: Dan Carpenter @ 2019-11-06 11:23 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Mao Wenan, gregkh, linux-usb, linux-kernel, kernel-janitors
On Tue, Nov 05, 2019 at 05:26:24PM +0200, Heikki Krogerus wrote:
> Hi Dan,
>
> On Tue, Nov 05, 2019 at 04:16:05PM +0300, Dan Carpenter wrote:
> > On Tue, Nov 05, 2019 at 02:42:18PM +0200, Heikki Krogerus wrote:
> > > On Mon, Nov 04, 2019 at 10:48:50PM +0800, Mao Wenan wrote:
> > > > The USB role switch class is, after all,
> > > > not useful by itself. Hiding USB_ROLE_SWITCH
> > > > so we can avoid any of the pitfalls associated
> > > > with user-visible symbols and "select".
> > > >
> > > > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > > > ---
> > > > drivers/usb/roles/Kconfig | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig
> > > > index f8b31aa..1da58d4 100644
> > > > --- a/drivers/usb/roles/Kconfig
> > > > +++ b/drivers/usb/roles/Kconfig
> > > > @@ -1,7 +1,7 @@
> > > > # SPDX-License-Identifier: GPL-2.0
> > > >
> > > > config USB_ROLE_SWITCH
> > > > - tristate "USB Role Switch Support"
> > > > + tristate
> > > > help
> > > > USB Role Switch is a device that can select the USB role - host or
> > > > device - for a USB port (connector). In most cases dual-role capable
> > >
> > > You didn't actually convert the "depends on USB_ROLE_SWTICH" to
> > > "select USB_ROLE_SWITCH" before this. You also left the help text that
> > > is now useless.
> > >
> > > I really think that instead of this, we should just convert all
> > > "select USB_ROLE_SWTICH" to "depends on USB_ROLE_SWITCH".
> >
> > The you have to find USB_ROLE_SWITCH first when you want to enable your
> > hardware... It's feels really confusing when you want to create a
> > .config file...
>
> Unfortunately selecting the class alone is not enough. The USB role
> switch on the system may be a dual-role capable USB controller, but it
> may also be a mux that has its own separate driver.
>
> It's equally or even more confusing for the user if the USB drivers
> are enabled, including the dual-role mode, but the connector still
> works only in one role, or in worst case not at all (if there is no
> mux driver and the mux is left in "safe mode" so that the pins on the
> connector are not connected to anything).
>
> I still think that we should make these drivers depend on the class
> instead of just selecting it. That way we at least give the user a
> hint that there are also separate USB role switch drivers that may be
> needed.
>
I guess I see your point. My problem then is just with the menuconfig
system, not really with this patch.
> > I sometimes think maybe I'm too stupid to configure a kernel these days
> > and that's sort of sad because how is Aunt Tillie supposed to manage?
>
> We can always use something like conditional comments in the
> Kconfig files to make sure that the user is told that in order to
> select the driver, a dependency must be satisfied:
>
> config MY_AWESOME_DRIVER
> tristate "My awesome driver!"
> depends on USB_ROLE_SWITCH
> help
> That's right! IT REALLY IS AWESOME!
>
> comment "My awesome driver depends on USB_ROLE_SWITCH..."
> depends on USB_ROLE_SWITCH=n
>
Those sorts of help don't show up unless you already meet the
dependencies or if you search for it. And if you search for it then it
already lists the depnds so the comment isn't required...
I guess that in the olden days you used to go through the menus and
look at all the high level options and figure it out. These days you
sort of have to know what you want already. And then you use the search
feature to enable it.
The menuconfig system really is very broken. I've looked at alternative
ways to do it, but it quite a bit of work involved...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
2019-11-04 10:04 ` Thierry Reding
@ 2019-11-05 12:37 ` Heikki Krogerus
-1 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-11-05 12:37 UTC (permalink / raw)
To: Thierry Reding
Cc: Mao Wenan, felipe.balbi, gregkh, nkristam, arnd, johan, krzk,
linux-usb, linux-kernel, kernel-janitors
On Mon, Nov 04, 2019 at 11:04:10AM +0100, Thierry Reding wrote:
> On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
> > If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> > below erros can be seen:
> > drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> > tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> > drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> > tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> > drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> > tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
> >
> > This patch add dependency USB_ROLE_SWITCH for UDC driver.
> >
> > Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > ---
> > drivers/usb/gadget/udc/Kconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> > index acaec3a..d103154 100644
> > --- a/drivers/usb/gadget/udc/Kconfig
> > +++ b/drivers/usb/gadget/udc/Kconfig
> > @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
> > tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
> > depends on ARCH_TEGRA || COMPILE_TEST
> > depends on PHY_TEGRA_XUSB
> > + depends on USB_ROLE_SWITCH
>
> It looks like most other drivers that use the USB role switch class do
> "select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
> visible symbol, which can lead to conflicts, so it should be avoided. I
> think that in this case it might make sense to hide USB_ROLE_SWITCH and
> then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
> USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
> itself. It always needs a host and/or gadget driver to make use of it.
USB host/gadget drivers actually never operate the role switches. If
the USB controller on the system is dual-role capable, then the driver
for that controller can supply the role switch, but it doesn't operate
it. Note that on some systems the USB host and USB peripheral
controllers are separate, and there is a mux (like the Intel USB role
mux) between them and the connector. On those systems the driver for
the mux represents the USB role switch.
The operation of the switch is done from the USB Type-C drivers with
USB Type-C connectors and from what ever driver can sense the ID-pin
and VBUS with micro-B/AB connectors, but with other type of
connectors the role swapping has to be done from user space. The use
case for that is probable something like Apple CarPlay that requires
the system to be able to swap the role even if the connector was good
old Type-A connector.
The point is in any case that the user of the switch is always
separate from the supplier of the switch.
I'm not sure hiding the option and converting all "depends on
USB_ROLE_SWITCH" to "select USB_ROLE_SWITCH" is the correct thing to
do. I would do the opposite and convert all "select USB_ROLE_SWTICH"
to "depends on USB_ROLE_SWITCH", and leave the option user selectable.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC
@ 2019-11-05 12:37 ` Heikki Krogerus
0 siblings, 0 replies; 24+ messages in thread
From: Heikki Krogerus @ 2019-11-05 12:37 UTC (permalink / raw)
To: Thierry Reding
Cc: Mao Wenan, felipe.balbi, gregkh, nkristam, arnd, johan, krzk,
linux-usb, linux-kernel, kernel-janitors
On Mon, Nov 04, 2019 at 11:04:10AM +0100, Thierry Reding wrote:
> On Mon, Nov 04, 2019 at 10:59:45AM +0800, Mao Wenan wrote:
> > If CONFIG_USB_TEGRA_XUDC=y and CONFIG_USB_ROLE_SWITCH=m,
> > below erros can be seen:
> > drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_remove':
> > tegra-xudc.c:(.text+0x6b0): undefined reference to `usb_role_switch_unregister'
> > drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_probe':
> > tegra-xudc.c:(.text+0x1b88): undefined reference to `usb_role_switch_register'
> > drivers/usb/gadget/udc/tegra-xudc.o: In function `tegra_xudc_usb_role_sw_work':
> > tegra-xudc.c:(.text+0x5ecc): undefined reference to `usb_role_switch_get_role'
> >
> > This patch add dependency USB_ROLE_SWITCH for UDC driver.
> >
> > Fixes: 49db427232fe ("usb: gadget: Add UDC driver for tegra XUSB device mode controller")
> > Signed-off-by: Mao Wenan <maowenan@huawei.com>
> > ---
> > drivers/usb/gadget/udc/Kconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> > index acaec3a..d103154 100644
> > --- a/drivers/usb/gadget/udc/Kconfig
> > +++ b/drivers/usb/gadget/udc/Kconfig
> > @@ -445,6 +445,7 @@ config USB_TEGRA_XUDC
> > tristate "NVIDIA Tegra Superspeed USB 3.0 Device Controller"
> > depends on ARCH_TEGRA || COMPILE_TEST
> > depends on PHY_TEGRA_XUSB
> > + depends on USB_ROLE_SWITCH
>
> It looks like most other drivers that use the USB role switch class do
> "select" here. Now, that's suboptimal because USB_ROLE_SWITCH is a user-
> visible symbol, which can lead to conflicts, so it should be avoided. I
> think that in this case it might make sense to hide USB_ROLE_SWITCH and
> then convert all "depends on USB_ROLE_SWITCH" occurrences to "select
> USB_ROLE_SWITCH". The USB role switch class is, after all, not useful by
> itself. It always needs a host and/or gadget driver to make use of it.
USB host/gadget drivers actually never operate the role switches. If
the USB controller on the system is dual-role capable, then the driver
for that controller can supply the role switch, but it doesn't operate
it. Note that on some systems the USB host and USB peripheral
controllers are separate, and there is a mux (like the Intel USB role
mux) between them and the connector. On those systems the driver for
the mux represents the USB role switch.
The operation of the switch is done from the USB Type-C drivers with
USB Type-C connectors and from what ever driver can sense the ID-pin
and VBUS with micro-B/AB connectors, but with other type of
connectors the role swapping has to be done from user space. The use
case for that is probable something like Apple CarPlay that requires
the system to be able to swap the role even if the connector was good
old Type-A connector.
The point is in any case that the user of the switch is always
separate from the supplier of the switch.
I'm not sure hiding the option and converting all "depends on
USB_ROLE_SWITCH" to "select USB_ROLE_SWITCH" is the correct thing to
do. I would do the opposite and convert all "select USB_ROLE_SWTICH"
to "depends on USB_ROLE_SWITCH", and leave the option user selectable.
thanks,
--
heikki
^ permalink raw reply [flat|nested] 24+ messages in thread