[PATCH v2 0/2] Fix role-switch selection in type-c drivers

[PATCH v2 0/2] Fix role-switch selection in type-c drivers

[Bryan O'Donoghue]

A recent change to the typs6598x resulted in a link-time error caught by
kbuild. The first patch fixes that error, however since it is not included
in stable yet, it carries no fixes tag.

In trying to fix the typs6598x error, the first thing I did was try to take
the same fix, for the same error that had been applied to the hd3ss3220
but, I could still configure my kernel and produce the same link-time error
with the tps6598x.

I did a quick grep and saw that everywhere else we were doing "select
USB_ROLE_SWITCH" not "depends on USB_ROLE_SWITCH".

Using select fixes the error for me on tps6598x so I've also rolled the
same fix into the hd3ss3220 because it looks like the right thing to do
there also.

Bryan O'Donoghue (2):
  usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x
  usb: typec: Ensure USB_ROLE_SWITCH is selected for hd3ss3220

 drivers/usb/typec/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v2 1/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x

[Bryan O'Donoghue]

When I switched on USB role switching for the tps6598x I completely forgot
to add the Kconfig dependency.

Ensure USB_ROLE_SWITCH is selected to prevent the typs6598x driver being
compiled in but the role-switch driver being compiled as a module, leading
to link error.

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..a6cdf4ec0be6 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
+	select USB_ROLE_SWITCH
 	select REGMAP_I2C
 	help
 	  Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power
-- 
2.25.1

[PATCH v2 2/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for hd3ss3220

[Bryan O'Donoghue]

kbuild found a compile error with recent additions to the tps6598x to add
role-switch notification support. Digging into that error it turns out we
needed to add "select USB_ROLE_SWITCH" to Kconfig.

Adding "depends on USB_ROLE_SWITCH" as had previously been done for this
driver could still result in a link-time failure.

This patch propagates the tps6598x fix to hd3ss3220.

Fixes: da4b5d18dd94 ("usb: typec: add dependency for TYPEC_HD3SS3220")

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/typec/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
index a6cdf4ec0be6..8f84304802f4 100644
--- a/drivers/usb/typec/Kconfig
+++ b/drivers/usb/typec/Kconfig
@@ -53,7 +53,7 @@ source "drivers/usb/typec/ucsi/Kconfig"
 config TYPEC_HD3SS3220
 	tristate "TI HD3SS3220 Type-C DRP Port controller driver"
 	depends on I2C
-	depends on USB_ROLE_SWITCH
+	select USB_ROLE_SWITCH
 	help
 	  Say Y or M here if your system has TI HD3SS3220 Type-C DRP Port
 	  controller driver.
-- 
2.25.1

Re: [PATCH v2 2/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for hd3ss3220

[Heikki Krogerus]

On Wed, May 20, 2020 at 01:33:12PM +0100, Bryan O'Donoghue wrote:
> kbuild found a compile error with recent additions to the tps6598x to add
> role-switch notification support. Digging into that error it turns out we
> needed to add "select USB_ROLE_SWITCH" to Kconfig.
> 
> Adding "depends on USB_ROLE_SWITCH" as had previously been done for this
> driver could still result in a link-time failure.
> 
> This patch propagates the tps6598x fix to hd3ss3220.
> 
> Fixes: da4b5d18dd94 ("usb: typec: add dependency for TYPEC_HD3SS3220")
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

NAK.

This is a switch consumer not supplier. The switch may not be needed
on every platform with this driver.

> ---
>  drivers/usb/typec/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index a6cdf4ec0be6..8f84304802f4 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -53,7 +53,7 @@ source "drivers/usb/typec/ucsi/Kconfig"
>  config TYPEC_HD3SS3220
>  	tristate "TI HD3SS3220 Type-C DRP Port controller driver"
>  	depends on I2C
> -	depends on USB_ROLE_SWITCH
> +	select USB_ROLE_SWITCH
>  	help
>  	  Say Y or M here if your system has TI HD3SS3220 Type-C DRP Port
>  	  controller driver.

thanks,

-- 
heikki

Re: [PATCH v2 1/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x

[Heikki Krogerus]

On Wed, May 20, 2020 at 01:33:11PM +0100, Bryan O'Donoghue wrote:
> When I switched on USB role switching for the tps6598x I completely forgot
> to add the Kconfig dependency.
> 
> Ensure USB_ROLE_SWITCH is selected to prevent the typs6598x driver being
> compiled in but the role-switch driver being compiled as a module, leading
> to link error.
> 
> 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..a6cdf4ec0be6 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
> +	select USB_ROLE_SWITCH
>  	select REGMAP_I2C
>  	help
>  	  Say Y or M here if your system has TI TPS65982 or TPS65983 USB Power

The same here. You can depend on the class, but you do not select it.
Note that we need to be able to handle this situation:

CONFIG_TYPEC_TPS6598X=y
CONFIG_USB_ROLE_SWITCH=m

and this covers that case:

        depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH

thanks,

-- 
heikki

Re: [PATCH v2 1/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x

[Bryan O'Donoghue]

On 20/05/2020 14:17, Heikki Krogerus wrote:
> depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH

Hmm.

That broke for me with a recursive dependency

but this will work

+       depends on REGMAP_I2C
+       depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH

Re: [PATCH v2 1/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x

[Bryan O'Donoghue]

On 20/05/2020 14:24, Bryan O'Donoghue wrote:
> On 20/05/2020 14:17, Heikki Krogerus wrote:
>> depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
> 
> Hmm.
> 
> That broke for me with a recursive dependency
> 
> but this will work
> 
> +       depends on REGMAP_I2C
> +       depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
> 

Sorry Heikki.

If I make the above change and then do this to switch off where the USB 
controller in my build is selecting - role switch

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
         select USB_TEGRA_PHY if ARCH_TEGRA
         help


it breaks

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'
make[1]: *** 
[/home/deckard/Development/qualcomm/qlt-kernel/Makefile:1106: vmlinux] 
Error 1
make[1]: Leaving directory 
'/home/deckard/Development/qualcomm/qlt-kernel-tools/qlt-kernel/build/square_5.x-tracking'

to do what you want to do - shouldn't we have to make all of those 
"select USB_ROLE_SWITCH" into "depends on USB_ROLE_SWITCH" ?

i.e. make all of the consumers depends on instead of selects ?

Re: [PATCH v2 1/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x

[Heikki Krogerus]

On Wed, May 20, 2020 at 02:32:31PM +0100, Bryan O'Donoghue wrote:
> On 20/05/2020 14:24, Bryan O'Donoghue wrote:
> > On 20/05/2020 14:17, Heikki Krogerus wrote:
> > > depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
> > 
> > Hmm.
> > 
> > That broke for me with a recursive dependency
> > 
> > but this will work
> > 
> > +       depends on REGMAP_I2C
> > +       depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
> > 
> 
> Sorry Heikki.
> 
> If I make the above change and then do this to switch off where the USB
> controller in my build is selecting - role switch
> 
> 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
>         select USB_TEGRA_PHY if ARCH_TEGRA
>         help

That driver is a switch supplier. You should select the class here.

> it breaks
> 
> 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'
> make[1]: *** [/home/deckard/Development/qualcomm/qlt-kernel/Makefile:1106:
> vmlinux] Error 1
> make[1]: Leaving directory '/home/deckard/Development/qualcomm/qlt-kernel-tools/qlt-kernel/build/square_5.x-tracking'
> 
> to do what you want to do - shouldn't we have to make all of those "select
> USB_ROLE_SWITCH" into "depends on USB_ROLE_SWITCH" ?
> 
> i.e. make all of the consumers depends on instead of selects ?

Yes, ideally.

thanks,

-- 
heikki

Re: [PATCH v2 1/2] usb: typec: Ensure USB_ROLE_SWITCH is selected for tps6598x

[Bryan O'Donoghue]

On 20/05/2020 14:39, Heikki Krogerus wrote:
>> to do what you want to do - shouldn't we have to make all of those "select
>> USB_ROLE_SWITCH" into "depends on USB_ROLE_SWITCH" ?
>>
>> i.e. make all of the consumers depends on instead of selects ?
> Yes, ideally.
> 
> thanks,

I've built the offending x86 version, instead of my more contrived 
version deselecting USB_ROLE_SWITCH from the chipidea

-       select USB_ROLE_SWITCH
-       select REGMAP_I2C
+       depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
+       depends on REGMAP_I2C


works - we can look at the depends stuff for the consumers later, I'll 
send the above now to fix the build.

---
bod