* [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC @ 2019-11-04 2:59 Mao Wenan 2019-11-04 10:04 ` Thierry Reding 0 siblings, 1 reply; 12+ messages in thread From: Mao Wenan @ 2019-11-04 2:59 UTC (permalink / raw) To: felipe.balbi, gregkh, treding, nkristam, arnd, johan, krzk Cc: linux-usb, linux-kernel, kernel-janitors, Mao Wenan 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 help Enables NVIDIA Tegra USB 3.0 device mode controller driver. -- 2.7.4 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC 2019-11-04 2:59 [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC Mao Wenan @ 2019-11-04 10:04 ` Thierry Reding 2019-11-04 10:50 ` maowenan 2019-11-05 12:37 ` [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC Heikki Krogerus 0 siblings, 2 replies; 12+ 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] 12+ 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 2019-11-04 11:21 ` [PATCH v2 " Mao Wenan 2019-11-04 13:53 ` [PATCH " Thierry Reding 2019-11-05 12:37 ` [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC Heikki Krogerus 1 sibling, 2 replies; 12+ 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] 12+ 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 2019-11-04 13:52 ` Thierry Reding 2019-11-04 13:53 ` [PATCH " Thierry Reding 1 sibling, 1 reply; 12+ 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] 12+ messages in thread
* Re: [PATCH v2 -next] usb: gadget: Add dependency for USB_TEGRA_XUDC 2019-11-04 11:21 ` [PATCH v2 " Mao Wenan @ 2019-11-04 13:52 ` Thierry Reding 0 siblings, 0 replies; 12+ 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] 12+ messages in thread
* Re: [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC 2019-11-04 10:50 ` maowenan 2019-11-04 11:21 ` [PATCH v2 " Mao Wenan @ 2019-11-04 13:53 ` Thierry Reding 2019-11-04 14:48 ` [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH Mao Wenan 1 sibling, 1 reply; 12+ 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] 12+ messages in thread
* [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH 2019-11-04 13:53 ` [PATCH " Thierry Reding @ 2019-11-04 14:48 ` Mao Wenan 2019-11-05 12:42 ` Heikki Krogerus 0 siblings, 1 reply; 12+ 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] 12+ messages in thread
* Re: [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH 2019-11-04 14:48 ` [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH Mao Wenan @ 2019-11-05 12:42 ` Heikki Krogerus 2019-11-05 13:16 ` Dan Carpenter 0 siblings, 1 reply; 12+ 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] 12+ 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 2019-11-05 15:26 ` Heikki Krogerus 0 siblings, 1 reply; 12+ 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] 12+ 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 2019-11-06 11:23 ` Dan Carpenter 0 siblings, 1 reply; 12+ 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] 12+ 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 0 siblings, 0 replies; 12+ 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] 12+ 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 @ 2019-11-05 12:37 ` Heikki Krogerus 1 sibling, 0 replies; 12+ 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] 12+ messages in thread
end of thread, other threads:[~2019-11-06 11:25 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-04 2:59 [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC Mao Wenan 2019-11-04 10:04 ` Thierry Reding 2019-11-04 10:50 ` maowenan 2019-11-04 11:21 ` [PATCH v2 " Mao Wenan 2019-11-04 13:52 ` Thierry Reding 2019-11-04 13:53 ` [PATCH " Thierry Reding 2019-11-04 14:48 ` [PATCH -next] usb: roles: Hide option USB_ROLE_SWITCH Mao Wenan 2019-11-05 12:42 ` Heikki Krogerus 2019-11-05 13:16 ` Dan Carpenter 2019-11-05 15:26 ` Heikki Krogerus 2019-11-06 11:23 ` Dan Carpenter 2019-11-05 12:37 ` [PATCH -next] usb: gadget: Add dependency for USB_TEGRA_XUDC Heikki Krogerus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).