[PATCH v1 0/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[PATCH v1 0/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

The enable gpio is not required when the SBU mux is used only for orientation,
make it optional.

Francesco Dolcini (2):
  dt-bindings: usb: gpio-sbu-mux: Make 'enable-gpios' optional
  usb: typec: mux: gpio-sbu: Make enable gpio optional

 .../devicetree/bindings/usb/gpio-sbu-mux.yaml         |  7 ++++++-
 drivers/usb/typec/mux/gpio-sbu-mux.c                  | 11 ++++++++---
 2 files changed, 14 insertions(+), 4 deletions(-)

-- 
2.39.2

[PATCH v1 1/2] dt-bindings: usb: gpio-sbu-mux: Make 'enable-gpios' optional

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

The enable gpio is not required when the SBU mux is used only for
orientation, make enable-gpios required only for alternate mode
switch use case.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
index 88e1607cf053..30edcce82f97 100644
--- a/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
+++ b/Documentation/devicetree/bindings/usb/gpio-sbu-mux.yaml
@@ -44,13 +44,18 @@ properties:
 
 required:
   - compatible
-  - enable-gpios
   - select-gpios
   - orientation-switch
   - port
 
 allOf:
   - $ref: usb-switch.yaml#
+  - if:
+      required:
+        - mode-switch
+    then:
+      required:
+        - enable-gpios
 
 additionalProperties: false
 
-- 
2.39.2

[PATCH v1 2/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Francesco Dolcini]

From: Francesco Dolcini <francesco.dolcini@toradex.com>

The enable gpio is not required when the SBU mux is used only for
orientation, make it optional.

Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
 drivers/usb/typec/mux/gpio-sbu-mux.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
index 374168482d36..cf44259980a1 100644
--- a/drivers/usb/typec/mux/gpio-sbu-mux.c
+++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
@@ -47,7 +47,7 @@ static int gpio_sbu_switch_set(struct typec_switch_dev *sw,
 		break;
 	}
 
-	if (enabled != sbu_mux->enabled)
+	if (sbu_mux->enable_gpio && enabled != sbu_mux->enabled)
 		gpiod_set_value_cansleep(sbu_mux->enable_gpio, enabled);
 
 	if (swapped != sbu_mux->swapped)
@@ -66,6 +66,9 @@ static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
 {
 	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
 
+	if (!sbu_mux->enable_gpio)
+		return -EOPNOTSUPP;
+
 	mutex_lock(&sbu_mux->lock);
 
 	switch (state->mode) {
@@ -102,7 +105,8 @@ static int gpio_sbu_mux_probe(struct platform_device *pdev)
 
 	mutex_init(&sbu_mux->lock);
 
-	sbu_mux->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	sbu_mux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+						       GPIOD_OUT_LOW);
 	if (IS_ERR(sbu_mux->enable_gpio))
 		return dev_err_probe(dev, PTR_ERR(sbu_mux->enable_gpio),
 				     "unable to acquire enable gpio\n");
@@ -141,7 +145,8 @@ static void gpio_sbu_mux_remove(struct platform_device *pdev)
 {
 	struct gpio_sbu_mux *sbu_mux = platform_get_drvdata(pdev);
 
-	gpiod_set_value_cansleep(sbu_mux->enable_gpio, 0);
+	if (sbu_mux->enable_gpio)
+		gpiod_set_value_cansleep(sbu_mux->enable_gpio, 0);
 
 	typec_mux_unregister(sbu_mux->mux);
 	typec_switch_unregister(sbu_mux->sw);
-- 
2.39.2

Re: [PATCH v1 2/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Dmitry Baryshkov]

On Fri, May 24, 2024 at 09:10:34AM +0200, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> The enable gpio is not required when the SBU mux is used only for
> orientation, make it optional.
> 
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
>  drivers/usb/typec/mux/gpio-sbu-mux.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> index 374168482d36..cf44259980a1 100644
> --- a/drivers/usb/typec/mux/gpio-sbu-mux.c
> +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
> @@ -47,7 +47,7 @@ static int gpio_sbu_switch_set(struct typec_switch_dev *sw,
>  		break;
>  	}
>  
> -	if (enabled != sbu_mux->enabled)
> +	if (sbu_mux->enable_gpio && enabled != sbu_mux->enabled)
>  		gpiod_set_value_cansleep(sbu_mux->enable_gpio, enabled);

I think it's safe to pass NULL to gpiod_set_value_cansleep().

>  
>  	if (swapped != sbu_mux->swapped)
> @@ -66,6 +66,9 @@ static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
>  {
>  	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
>  
> +	if (!sbu_mux->enable_gpio)
> +		return -EOPNOTSUPP;

Can we skip registering the mux if there is no enable_gpio? This can
save users from the unexpected errors during runtime.

> +
>  	mutex_lock(&sbu_mux->lock);
>  
>  	switch (state->mode) {
> @@ -102,7 +105,8 @@ static int gpio_sbu_mux_probe(struct platform_device *pdev)
>  
>  	mutex_init(&sbu_mux->lock);
>  
> -	sbu_mux->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
> +	sbu_mux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> +						       GPIOD_OUT_LOW);
>  	if (IS_ERR(sbu_mux->enable_gpio))
>  		return dev_err_probe(dev, PTR_ERR(sbu_mux->enable_gpio),
>  				     "unable to acquire enable gpio\n");
> @@ -141,7 +145,8 @@ static void gpio_sbu_mux_remove(struct platform_device *pdev)
>  {
>  	struct gpio_sbu_mux *sbu_mux = platform_get_drvdata(pdev);
>  
> -	gpiod_set_value_cansleep(sbu_mux->enable_gpio, 0);
> +	if (sbu_mux->enable_gpio)
> +		gpiod_set_value_cansleep(sbu_mux->enable_gpio, 0);
>  
>  	typec_mux_unregister(sbu_mux->mux);
>  	typec_switch_unregister(sbu_mux->sw);
> -- 
> 2.39.2
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Francesco Dolcini]

Hello Dmitry,
thanks for the review.

On Fri, May 24, 2024 at 12:56:15PM +0300, Dmitry Baryshkov wrote:
> On Fri, May 24, 2024 at 09:10:34AM +0200, Francesco Dolcini wrote:
> > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > 
> > The enable gpio is not required when the SBU mux is used only for
> > orientation, make it optional.
> > 
> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > ---
> >  drivers/usb/typec/mux/gpio-sbu-mux.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> > index 374168482d36..cf44259980a1 100644
> > --- a/drivers/usb/typec/mux/gpio-sbu-mux.c
> > +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
...
> > @@ -66,6 +66,9 @@ static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
> >  {
> >  	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
> >  
> > +	if (!sbu_mux->enable_gpio)
> > +		return -EOPNOTSUPP;
> 
> Can we skip registering the mux if there is no enable_gpio? This can
> save users from the unexpected errors during runtime.

Yes, I considered this option.

The rationale for the current implementation is that if the device tree is
correct (no mode-switch property, when enable-gpios is not present), nobody
will call gpio_sbu_mux_set() so no runtime error is possible. If the
configuration in the DT is not correct you get this runtime error.

With your proposal in case the DT configuration is not correct there will be no
errors from the kernel, but the functionality will not work.

Francesco

Re: [PATCH v1 2/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Dmitry Baryshkov]

On Fri, May 24, 2024 at 01:31:58PM +0200, Francesco Dolcini wrote:
> Hello Dmitry,
> thanks for the review.
> 
> On Fri, May 24, 2024 at 12:56:15PM +0300, Dmitry Baryshkov wrote:
> > On Fri, May 24, 2024 at 09:10:34AM +0200, Francesco Dolcini wrote:
> > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > 
> > > The enable gpio is not required when the SBU mux is used only for
> > > orientation, make it optional.
> > > 
> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > ---
> > >  drivers/usb/typec/mux/gpio-sbu-mux.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > index 374168482d36..cf44259980a1 100644
> > > --- a/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
> ...
> > > @@ -66,6 +66,9 @@ static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
> > >  {
> > >  	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
> > >  
> > > +	if (!sbu_mux->enable_gpio)
> > > +		return -EOPNOTSUPP;
> > 
> > Can we skip registering the mux if there is no enable_gpio? This can
> > save users from the unexpected errors during runtime.
> 
> Yes, I considered this option.
> 
> The rationale for the current implementation is that if the device tree is
> correct (no mode-switch property, when enable-gpios is not present), nobody
> will call gpio_sbu_mux_set() so no runtime error is possible. If the
> configuration in the DT is not correct you get this runtime error.
> 
> With your proposal in case the DT configuration is not correct there will be no
> errors from the kernel, but the functionality will not work.

I'm slightly biased maybe, but I prefer an error from probe (or
dependent devices being deferred). On the other hand, current motto is
that 'the kernel should not duplicate dt-validate's work'.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Francesco Dolcini]

Hello Dmitry,

On Fri, May 24, 2024 at 02:42:04PM +0300, Dmitry Baryshkov wrote:
> On Fri, May 24, 2024 at 01:31:58PM +0200, Francesco Dolcini wrote:
> > On Fri, May 24, 2024 at 12:56:15PM +0300, Dmitry Baryshkov wrote:
> > > On Fri, May 24, 2024 at 09:10:34AM +0200, Francesco Dolcini wrote:
> > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > 
> > > > The enable gpio is not required when the SBU mux is used only for
> > > > orientation, make it optional.
> > > > 
> > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > ---
> > > >  drivers/usb/typec/mux/gpio-sbu-mux.c | 11 ++++++++---
> > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > > index 374168482d36..cf44259980a1 100644
> > > > --- a/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > > +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
> > ...
> > > > @@ -66,6 +66,9 @@ static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
> > > >  {
> > > >  	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
> > > >  
> > > > +	if (!sbu_mux->enable_gpio)
> > > > +		return -EOPNOTSUPP;
> > > 
> > > Can we skip registering the mux if there is no enable_gpio? This can
> > > save users from the unexpected errors during runtime.
> > 
> > Yes, I considered this option.
> > 
> > The rationale for the current implementation is that if the device tree is
> > correct (no mode-switch property, when enable-gpios is not present), nobody
> > will call gpio_sbu_mux_set() so no runtime error is possible. If the
> > configuration in the DT is not correct you get this runtime error.
> > 
> > With your proposal in case the DT configuration is not correct there will be no
> > errors from the kernel, but the functionality will not work.
> 
> I'm slightly biased maybe, but I prefer an error from probe (or
> dependent devices being deferred). On the other hand, current motto is
> that 'the kernel should not duplicate dt-validate's work'.

I am in favor of "the kernel should not duplicate dt-validate's work".

Now the question is if you are ok with the current implementation or you want
me to change the way you suggested.

Or maybe there is a third variant, not doing the return -EOPNOTSUPP and
registering gpio_sbu_mux_set() even if the gpio get returns NULL. This is a
one-line patch and everything will work just fine.

Francesco

Re: [PATCH v1 1/2] dt-bindings: usb: gpio-sbu-mux: Make 'enable-gpios' optional

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 433 bytes --]

On Fri, May 24, 2024 at 09:10:33AM +0200, Francesco Dolcini wrote:
> From: Francesco Dolcini <francesco.dolcini@toradex.com>
> 
> The enable gpio is not required when the SBU mux is used only for
> orientation, make enable-gpios required only for alternate mode
> switch use case.
> 
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/2] usb: typec: mux: gpio-sbu: Make enable gpio optional

[Dmitry Baryshkov]

On Fri, May 24, 2024 at 02:51:10PM +0200, Francesco Dolcini wrote:
> Hello Dmitry,
> 
> On Fri, May 24, 2024 at 02:42:04PM +0300, Dmitry Baryshkov wrote:
> > On Fri, May 24, 2024 at 01:31:58PM +0200, Francesco Dolcini wrote:
> > > On Fri, May 24, 2024 at 12:56:15PM +0300, Dmitry Baryshkov wrote:
> > > > On Fri, May 24, 2024 at 09:10:34AM +0200, Francesco Dolcini wrote:
> > > > > From: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > > 
> > > > > The enable gpio is not required when the SBU mux is used only for
> > > > > orientation, make it optional.
> > > > > 
> > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> > > > > ---
> > > > >  drivers/usb/typec/mux/gpio-sbu-mux.c | 11 ++++++++---
> > > > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/usb/typec/mux/gpio-sbu-mux.c b/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > > > index 374168482d36..cf44259980a1 100644
> > > > > --- a/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > > > +++ b/drivers/usb/typec/mux/gpio-sbu-mux.c
> > > ...
> > > > > @@ -66,6 +66,9 @@ static int gpio_sbu_mux_set(struct typec_mux_dev *mux,
> > > > >  {
> > > > >  	struct gpio_sbu_mux *sbu_mux = typec_mux_get_drvdata(mux);
> > > > >  
> > > > > +	if (!sbu_mux->enable_gpio)
> > > > > +		return -EOPNOTSUPP;
> > > > 
> > > > Can we skip registering the mux if there is no enable_gpio? This can
> > > > save users from the unexpected errors during runtime.
> > > 
> > > Yes, I considered this option.
> > > 
> > > The rationale for the current implementation is that if the device tree is
> > > correct (no mode-switch property, when enable-gpios is not present), nobody
> > > will call gpio_sbu_mux_set() so no runtime error is possible. If the
> > > configuration in the DT is not correct you get this runtime error.
> > > 
> > > With your proposal in case the DT configuration is not correct there will be no
> > > errors from the kernel, but the functionality will not work.
> > 
> > I'm slightly biased maybe, but I prefer an error from probe (or
> > dependent devices being deferred). On the other hand, current motto is
> > that 'the kernel should not duplicate dt-validate's work'.
> 
> I am in favor of "the kernel should not duplicate dt-validate's work".
> 
> Now the question is if you are ok with the current implementation or you want
> me to change the way you suggested.
> 
> Or maybe there is a third variant, not doing the return -EOPNOTSUPP and
> registering gpio_sbu_mux_set() even if the gpio get returns NULL. This is a
> one-line patch and everything will work just fine.

I'm fine either way.

-- 
With best wishes
Dmitry