Message ID | 20200520100526.2729-1-bryan.odonoghue@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | usb: typec: Ensure USB_ROLE_SWITCH is a dependency for tps6598x | expand |
On Wed, May 20, 2020 at 11:05:26AM +0100, Bryan O'Donoghue wrote: > When I switched on USB role switching for the tps6598x I completely forgot > to add the Kconfig dependency. > > This patch ensures the dependency is there to prevent compilation error > when role-switching is off. There are stubs for the those functions, so there should not be any compilation errors. > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > drivers/usb/typec/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig > index b4f2aac7ae8a..4ea18301b15e 100644 > --- a/drivers/usb/typec/Kconfig > +++ b/drivers/usb/typec/Kconfig > @@ -64,6 +64,7 @@ config TYPEC_HD3SS3220 > config TYPEC_TPS6598X > tristate "TI TPS6598x USB Power Delivery controller driver" > depends on I2C > + depends on USB_ROLE_SWITCH How about this: depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH We then would have an option to use this driver even without that when its not needed. thanks,
On 20/05/2020 11:35, Heikki Krogerus wrote: > On Wed, May 20, 2020 at 11:05:26AM +0100, Bryan O'Donoghue wrote: >> When I switched on USB role switching for the tps6598x I completely forgot >> to add the Kconfig dependency. >> >> This patch ensures the dependency is there to prevent compilation error >> when role-switching is off. > > There are stubs for the those functions, so there should not be any > compilation errors. > That's what I initially thought too, then I saw this. git show da4b5d18dd949abdda7c8ea76c9483b5edd49616 but looking at role.h #if IS_ENABLED(CONFIG_USB_ROLE_SWITCH) int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role); #else static inline int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role) { return 0; } #endif That should work. Hmm, let me see if I can figure this out...
On 20/05/2020 12:08, Bryan O'Donoghue wrote: > On 20/05/2020 11:35, Heikki Krogerus wrote: >> On Wed, May 20, 2020 at 11:05:26AM +0100, Bryan O'Donoghue wrote: >>> When I switched on USB role switching for the tps6598x I completely >>> forgot >>> to add the Kconfig dependency. >>> >>> This patch ensures the dependency is there to prevent compilation error >>> when role-switching is off. >> >> There are stubs for the those functions, so there should not be any >> compilation errors. >> > > That's what I initially thought too, then I saw this. > > git show da4b5d18dd949abdda7c8ea76c9483b5edd49616 > > but looking at role.h > > #if IS_ENABLED(CONFIG_USB_ROLE_SWITCH) > > int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role > role); > > #else > > static inline int usb_role_switch_set_role(struct usb_role_switch *sw, > enum usb_role role) > { > return 0; > } > > #endif > > That should work. > > Hmm, let me see if I can figure this out... Well if I do this diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig index d53db520e209..636a5428b47e 100644 --- a/drivers/usb/chipidea/Kconfig +++ b/drivers/usb/chipidea/Kconfig @@ -6,7 +6,6 @@ config USB_CHIPIDEA select EXTCON select RESET_CONTROLLER select USB_ULPI_BUS - select USB_ROLE_SWITCH my build does this drivers/usb/dwc3/drd.o: In function `dwc3_usb_role_switch_get': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/dwc3/drd.c:508: undefined reference to `usb_role_switch_get_drvdata' drivers/usb/dwc3/drd.o: In function `dwc3_usb_role_switch_set': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/dwc3/drd.c:484: undefined reference to `usb_role_switch_get_drvdata' drivers/usb/dwc3/drd.o: In function `dwc3_setup_role_switch': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/dwc3/drd.c:555: undefined reference to `usb_role_switch_register' drivers/usb/dwc3/drd.o: In function `dwc3_drd_exit': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/dwc3/drd.c:628: undefined reference to `usb_role_switch_unregister' drivers/usb/chipidea/core.o: In function `ci_usb_role_switch_get': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/chipidea/core.c:621: undefined reference to `usb_role_switch_get_drvdata' drivers/usb/chipidea/core.o: In function `ci_usb_role_switch_set': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/chipidea/core.c:635: undefined reference to `usb_role_switch_get_drvdata' drivers/usb/chipidea/core.o: In function `ci_hdrc_remove': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/chipidea/core.c:1231: undefined reference to `usb_role_switch_unregister' drivers/usb/chipidea/core.o: In function `ci_hdrc_probe': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/chipidea/core.c:1210: undefined reference to `usb_role_switch_unregister' /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/chipidea/core.c:1143: undefined reference to `usb_role_switch_register' drivers/usb/typec/tps6598x.o: In function `tps6598x_set_data_role': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/typec/tps6598x.c:208: undefined reference to `usb_role_switch_set_role' drivers/usb/typec/tps6598x.o: In function `tps6598x_remove': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/typec/tps6598x.c:603: undefined reference to `usb_role_switch_put' drivers/usb/typec/tps6598x.o: In function `tps6598x_set_data_role': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/typec/tps6598x.c:208: undefined reference to `usb_role_switch_set_role' drivers/usb/typec/tps6598x.o: In function `tps6598x_probe': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/typec/tps6598x.c:520: undefined reference to `fwnode_usb_role_switch_get' /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/typec/tps6598x.c:590: undefined reference to `usb_role_switch_put' drivers/usb/typec/tps6598x.o: In function `tps6598x_set_data_role': /home/deckard/Development/qualcomm/qlt-kernel/drivers/usb/typec/tps6598x.c:208: undefined reference to `usb_role_switch_set_role' make[1]: *** [/home/deckard/Development/qualcomm/qlt-kernel/Makefile:1106: vmlinux] Error 1 so its consistent anyway
On 20/05/2020 12:13, Bryan O'Donoghue wrote: > On 20/05/2020 12:08, Bryan O'Donoghue wrote: >> On 20/05/2020 11:35, Heikki Krogerus wrote: >>> On Wed, May 20, 2020 at 11:05:26AM +0100, Bryan O'Donoghue wrote: >>>> When I switched on USB role switching for the tps6598x I completely >>>> forgot >>>> to add the Kconfig dependency. >>>> >>>> This patch ensures the dependency is there to prevent compilation error >>>> when role-switching is off. >>> >>> There are stubs for the those functions, so there should not be any >>> compilation errors. >>> >> >> That's what I initially thought too, then I saw this. >> >> git show da4b5d18dd949abdda7c8ea76c9483b5edd49616 >> >> but looking at role.h >> >> #if IS_ENABLED(CONFIG_USB_ROLE_SWITCH) >> >> int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role >> role); >> >> #else >> >> static inline int usb_role_switch_set_role(struct usb_role_switch *sw, >> enum usb_role role) >> { >> return 0; >> } >> >> #endif >> >> That should work. >> >> Hmm, let me see if I can figure this out... > > Well if I do this > > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig > index d53db520e209..636a5428b47e 100644 > --- a/drivers/usb/chipidea/Kconfig > +++ b/drivers/usb/chipidea/Kconfig > @@ -6,7 +6,6 @@ config USB_CHIPIDEA > select EXTCON > select RESET_CONTROLLER > select USB_ULPI_BUS > - select USB_ROLE_SWITCH > > my build does this ah ha - look at what role switch is defaulting to CONFIG_USB_ROLE_SWITCH=m So anything linked into the kernel image will not resolve those symbols
On 20/05/2020 12:22, Bryan O'Donoghue wrote: > On 20/05/2020 12:13, Bryan O'Donoghue wrote: >> On 20/05/2020 12:08, Bryan O'Donoghue wrote: >>> On 20/05/2020 11:35, Heikki Krogerus wrote: >>>> On Wed, May 20, 2020 at 11:05:26AM +0100, Bryan O'Donoghue wrote: >>>>> When I switched on USB role switching for the tps6598x I completely >>>>> forgot >>>>> to add the Kconfig dependency. >>>>> >>>>> This patch ensures the dependency is there to prevent compilation >>>>> error >>>>> when role-switching is off. >>>> >>>> There are stubs for the those functions, so there should not be any >>>> compilation errors. >>>> >>> >>> That's what I initially thought too, then I saw this. >>> >>> git show da4b5d18dd949abdda7c8ea76c9483b5edd49616 >>> >>> but looking at role.h >>> >>> #if IS_ENABLED(CONFIG_USB_ROLE_SWITCH) >>> >>> int usb_role_switch_set_role(struct usb_role_switch *sw, enum >>> usb_role role); >>> >>> #else >>> >>> static inline int usb_role_switch_set_role(struct usb_role_switch *sw, >>> enum usb_role role) >>> { >>> return 0; >>> } >>> >>> #endif >>> >>> That should work. >>> >>> Hmm, let me see if I can figure this out... >> >> Well if I do this >> >> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig >> index d53db520e209..636a5428b47e 100644 >> --- a/drivers/usb/chipidea/Kconfig >> +++ b/drivers/usb/chipidea/Kconfig >> @@ -6,7 +6,6 @@ config USB_CHIPIDEA >> select EXTCON >> select RESET_CONTROLLER >> select USB_ULPI_BUS >> - select USB_ROLE_SWITCH >> >> my build does this > > ah ha - look at what role switch is defaulting to > > CONFIG_USB_ROLE_SWITCH=m > > So anything linked into the kernel image will not resolve those symbols So it should be "select USB_ROLE_SWITCH" not "depends on USB_ROLE_SWITCH" grep -r "USB_ROLE_SWITCH" * | grep depend drivers/usb/typec/Kconfig: depends on USB_ROLE_SWITCH grep -r "USB_ROLE_SWITCH" * | grep select drivers/extcon/Kconfig: select USB_ROLE_SWITCH drivers/usb/dwc3/Kconfig: select USB_ROLE_SWITCH drivers/usb/cdns3/Kconfig: select USB_ROLE_SWITCH drivers/usb/gadget/udc/Kconfig: select USB_ROLE_SWITCH drivers/usb/mtu3/Kconfig: select USB_ROLE_SWITCH drivers/usb/musb/Kconfig: select USB_ROLE_SWITCH drivers/usb/musb/Kconfig: select USB_ROLE_SWITCH drivers/usb/typec/tcpm/Kconfig: select USB_ROLE_SWITCH drivers/usb/typec/Kconfig: select USB_ROLE_SWITCH drivers/usb/typec/mux/Kconfig: select USB_ROLE_SWITCH drivers/usb/common/Kconfig: select USB_ROLE_SWITCH
On Wed, May 20, 2020 at 12:52:36PM +0100, Bryan O'Donoghue wrote: > So it should be "select USB_ROLE_SWITCH" not "depends on USB_ROLE_SWITCH" No. The consumers of the switches should depend on it, not silently select it. > grep -r "USB_ROLE_SWITCH" * | grep depend > drivers/usb/typec/Kconfig: depends on USB_ROLE_SWITCH > > grep -r "USB_ROLE_SWITCH" * | grep select > drivers/extcon/Kconfig: select USB_ROLE_SWITCH > drivers/usb/dwc3/Kconfig: select USB_ROLE_SWITCH > drivers/usb/cdns3/Kconfig: select USB_ROLE_SWITCH > drivers/usb/gadget/udc/Kconfig: select USB_ROLE_SWITCH > drivers/usb/mtu3/Kconfig: select USB_ROLE_SWITCH > drivers/usb/musb/Kconfig: select USB_ROLE_SWITCH > drivers/usb/musb/Kconfig: select USB_ROLE_SWITCH > drivers/usb/typec/tcpm/Kconfig: select USB_ROLE_SWITCH > drivers/usb/typec/Kconfig: select USB_ROLE_SWITCH > drivers/usb/typec/mux/Kconfig: select USB_ROLE_SWITCH > drivers/usb/common/Kconfig: select USB_ROLE_SWITCH Note that all those except tcpm supply the switch. thanks,
Hi Bryan,
I love your patch! Yet something to improve:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on balbi-usb/testing/next peter.chen-usb/ci-for-usb-next v5.7-rc6 next-20200519]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Bryan-O-Donoghue/usb-typec-Ensure-USB_ROLE_SWITCH-is-a-dependency-for-tps6598x/20200521-011740
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: i386-tinyconfig
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce:
make ARCH=i386 tinyconfig
make ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/of/Kconfig:69:error: recursive dependency detected!
drivers/of/Kconfig:69: symbol OF_IRQ depends on IRQ_DOMAIN
kernel/irq/Kconfig:68: symbol IRQ_DOMAIN is selected by REGMAP
drivers/base/regmap/Kconfig:7: symbol REGMAP default is visible depending on REGMAP_I2C
drivers/base/regmap/Kconfig:19: symbol REGMAP_I2C is selected by TYPEC_TPS6598X
drivers/usb/typec/Kconfig:64: symbol TYPEC_TPS6598X depends on USB_ROLE_SWITCH
drivers/usb/roles/Kconfig:3: symbol USB_ROLE_SWITCH is selected by USB_MUSB_MEDIATEK
drivers/usb/musb/Kconfig:119: symbol USB_MUSB_MEDIATEK depends on GENERIC_PHY
drivers/phy/Kconfig:8: symbol GENERIC_PHY is selected by PHY_BCM_NS_USB3
drivers/phy/broadcom/Kconfig:41: symbol PHY_BCM_NS_USB3 depends on MDIO_BUS
drivers/net/phy/Kconfig:13: symbol MDIO_BUS depends on MDIO_DEVICE
drivers/net/phy/Kconfig:6: symbol MDIO_DEVICE is selected by PHYLIB
drivers/net/phy/Kconfig:243: symbol PHYLIB is selected by ARC_EMAC_CORE
drivers/net/ethernet/arc/Kconfig:19: symbol ARC_EMAC_CORE is selected by ARC_EMAC
drivers/net/ethernet/arc/Kconfig:25: symbol ARC_EMAC depends on OF_IRQ
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"
vim +69 drivers/of/Kconfig
5ab5fc7e35705c Grant Likely 2010-07-05 14
19fd74879a32fb Grant Likely 2014-11-04 15 config OF_UNITTEST
19fd74879a32fb Grant Likely 2014-11-04 16 bool "Device Tree runtime unit tests"
6019a3d07d7258 Rob Herring 2017-07-25 17 depends on !SPARC
6019a3d07d7258 Rob Herring 2017-07-25 18 select IRQ_DOMAIN
649e0a77e28a77 Rob Herring 2015-05-28 19 select OF_EARLY_FLATTREE
2eb46da2a760e5 Grant Likely 2014-10-02 20 select OF_RESOLVE
53a42093d96ef5 Grant Likely 2011-12-12 21 help
53a42093d96ef5 Grant Likely 2011-12-12 22 This option builds in test cases for the device tree infrastructure
5d9270869b6cd3 Geert Uytterhoeven 2013-12-24 23 that are executed once at boot time, and the results dumped to the
53a42093d96ef5 Grant Likely 2011-12-12 24 console.
53a42093d96ef5 Grant Likely 2011-12-12 25
53a42093d96ef5 Grant Likely 2011-12-12 26 If unsure, say N here, but this option is safe to enable.
53a42093d96ef5 Grant Likely 2011-12-12 27
1b7c501b51a8c8 Rob Herring 2015-10-06 28 config OF_ALL_DTBS
1b7c501b51a8c8 Rob Herring 2015-10-06 29 bool "Build all Device Tree Blobs"
1b7c501b51a8c8 Rob Herring 2015-10-06 30 depends on COMPILE_TEST
1b7c501b51a8c8 Rob Herring 2015-10-06 31 select DTC
1b7c501b51a8c8 Rob Herring 2015-10-06 32 help
1b7c501b51a8c8 Rob Herring 2015-10-06 33 This option builds all possible Device Tree Blobs (DTBs) for the
1b7c501b51a8c8 Rob Herring 2015-10-06 34 current architecture.
1b7c501b51a8c8 Rob Herring 2015-10-06 35
1b7c501b51a8c8 Rob Herring 2015-10-06 36 If unsure, say N here, but this option is safe to enable.
1b7c501b51a8c8 Rob Herring 2015-10-06 37
e169cfbef46d62 Grant Likely 2009-11-23 38 config OF_FLATTREE
e169cfbef46d62 Grant Likely 2009-11-23 39 bool
5ab5fc7e35705c Grant Likely 2010-07-05 40 select DTC
e6a6928c3ea1d0 Rob Herring 2014-04-02 41 select LIBFDT
08d53aa58cb162 Ard Biesheuvel 2014-11-14 42 select CRC32
e169cfbef46d62 Grant Likely 2009-11-23 43
e6ce1324e4f08b Stephen Neuendorffer 2010-11-18 44 config OF_EARLY_FLATTREE
e6ce1324e4f08b Stephen Neuendorffer 2010-11-18 45 bool
ff4c25f26a71b7 Christoph Hellwig 2019-02-03 46 select DMA_DECLARE_COHERENT if HAS_DMA
e6ce1324e4f08b Stephen Neuendorffer 2010-11-18 47 select OF_FLATTREE
e6ce1324e4f08b Stephen Neuendorffer 2010-11-18 48
3cfc535c5df812 Andres Salomon 2010-10-10 49 config OF_PROMTREE
3cfc535c5df812 Andres Salomon 2010-10-10 50 bool
3cfc535c5df812 Andres Salomon 2010-10-10 51
b56b5528f5b3c3 Rob Herring 2017-10-04 52 config OF_KOBJ
b56b5528f5b3c3 Rob Herring 2017-10-04 53 def_bool SYSFS
b56b5528f5b3c3 Rob Herring 2017-10-04 54
0f22dd395fc473 Grant Likely 2012-02-15 55 # Hardly any platforms need this. It is safe to select, but only do so if you
0f22dd395fc473 Grant Likely 2012-02-15 56 # need it.
fcdeb7fedf89f4 Grant Likely 2010-01-29 57 config OF_DYNAMIC
121c92cad33db2 Geert Uytterhoeven 2015-01-23 58 bool "Support for dynamic device trees" if OF_UNITTEST
b56b5528f5b3c3 Rob Herring 2017-10-04 59 select OF_KOBJ
121c92cad33db2 Geert Uytterhoeven 2015-01-23 60 help
121c92cad33db2 Geert Uytterhoeven 2015-01-23 61 On some platforms, the device tree can be manipulated at runtime.
121c92cad33db2 Geert Uytterhoeven 2015-01-23 62 While this option is selected automatically on such platforms, you
121c92cad33db2 Geert Uytterhoeven 2015-01-23 63 can enable it manually to improve device tree unit test coverage.
fcdeb7fedf89f4 Grant Likely 2010-01-29 64
6b884a8d50a6ee Grant Likely 2010-06-08 65 config OF_ADDRESS
6b884a8d50a6ee Grant Likely 2010-06-08 66 def_bool y
6019a3d07d7258 Rob Herring 2017-07-25 67 depends on !SPARC && (HAS_IOMEM || UML)
6b884a8d50a6ee Grant Likely 2010-06-08 68
e3873444990dd6 Grant Likely 2010-06-18 @69 config OF_IRQ
e3873444990dd6 Grant Likely 2010-06-18 70 def_bool y
63c60e3a6dc3ec Geert Uytterhoeven 2015-04-05 71 depends on !SPARC && IRQ_DOMAIN
e3873444990dd6 Grant Likely 2010-06-18 72
:::::: The code at line 69 was first introduced by commit
:::::: e3873444990dd6f8a095d1f72b5ad45192f8c506 of/irq: Move irq_of_parse_and_map() to common code
:::::: TO: Grant Likely <grant.likely@secretlab.ca>
:::::: CC: Grant Likely <grant.likely@secretlab.ca>
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig index b4f2aac7ae8a..4ea18301b15e 100644 --- a/drivers/usb/typec/Kconfig +++ b/drivers/usb/typec/Kconfig @@ -64,6 +64,7 @@ config TYPEC_HD3SS3220 config TYPEC_TPS6598X tristate "TI TPS6598x USB Power Delivery controller driver" depends on I2C + depends on USB_ROLE_SWITCH select REGMAP_I2C help Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power
When I switched on USB role switching for the tps6598x I completely forgot to add the Kconfig dependency. This patch ensures the dependency is there to prevent compilation error when role-switching is off. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- drivers/usb/typec/Kconfig | 1 + 1 file changed, 1 insertion(+)