[PATCH] usb: dwc3-generic: fix support without DM_REGULATOR

[Dragan Simic]

Hello Caleb,

On 2024-04-15 13:18, Caleb Connolly wrote:
> On 15/04/2024 12:14, Dragan Simic wrote:
>> Hello all,
>> 
>> On 2024-04-15 13:11, Robert Marko wrote:
>>> On Mon, Apr 15, 2024 at 12:57 PM Caleb Connolly
>>> <caleb.connolly at linaro.org> wrote:
>>>> 
>>>> On 15/04/2024 11:53, Robert Marko wrote:
>>>> > Recent addition of vbus-supply support has broke platform which
>>>> dont use
>>>> > controllable regulators for USB.
>>>> >
>>>> > Issue is that even withou DM_REGULATOR being enabled regulator related
>>>> > functions will still build as there is a stub in regulator.h but
>>>> they will
>>>> > simply return -ENOSYS which will then make dwc3_generic_host_probe()
>>>> > return the same error thus breaking probe.
>>>> 
>>>> Rather than stubbing out the code, could you check for -ENOSYS and
>>>> ignore the error in that case? I believe there's only one place 
>>>> where
>>>> this matters (marked below).
>>> 
>>> Sure, that was my first approach but it did not seem right to me.
>>> But if its OK with you, I can do that.
>> 
>> FWIW, ifdef'ing the code out seems like a cleaner approach to me.
> 
> Given that the stub functions exist, imho it makes sense to use them.
> It's also easy to forget to add those #ifdefs if there are other
> regulator changes in future patches.
> 
> But overall I don't feel too strongly about this, for what it does this
> code is fine, I'll defer to Marek.

Oh, sorry, I totally failed to notice that the stubs already exist
for the regulator_set_enable_if_allowed() function and its friends,
which are actually ifdef'ed code.  Having that in mind, I agree that
introducing more ifdef's isn't the preferred way to go.

Maybe there's something awkward about the stubs and the way they
need to be used, but that should be resolved by improving the whole
stubs thing, instead of introducing more ifdef's in the code that
ends up using the stubs.

>>>> > Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus
>>>> regulator")
>>>> > Signed-off-by: Robert Marko <robert.marko at sartura.hr>
> 
> Reviewed-by: Caleb Connolly <caleb.connolly at linaro.org>
>>>> > ---
>>>> >  drivers/usb/dwc3/dwc3-generic.c | 8 ++++++++
>>>> >  1 file changed, 8 insertions(+)
>>>> >
>>>> > diff --git a/drivers/usb/dwc3/dwc3-generic.c
>>>> b/drivers/usb/dwc3/dwc3-generic.c
>>>> > index 7a00529a2a..784d3ec2ed 100644
>>>> > --- a/drivers/usb/dwc3/dwc3-generic.c
>>>> > +++ b/drivers/usb/dwc3/dwc3-generic.c
>>>> > @@ -242,6 +242,7 @@ static int dwc3_generic_host_probe(struct
>>>> udevice *dev)
>>>> >       if (rc)
>>>> >               return rc;
>>>> >
>>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>> >       rc = device_get_supply_regulator(dev, "vbus-supply",
>>>> &priv->vbus_supply);
>>>> >       if (rc)
>>>> >               debug("%s: No vbus regulator found: %d\n", dev->name,
>>>> rc);
>>>> > @@ -250,14 +251,17 @@ static int dwc3_generic_host_probe(struct
>>>> udevice *dev)
>>>> >       rc = regulator_set_enable_if_allowed(priv->vbus_supply, true);
>>>> >       if (rc)
>>>> >               return rc;
>>>> 
>>>> Here, if (rc && rc != -ENOSYS)
>>>> or even if (CONFIG_IS_ENABLED(DM_REGULATOR) && rc) to be verbose 
>>>> (maybe
>>>> not the preferred style though).
>>>> 
>>>> All the other regulator_* calls either ignore the result or only 
>>>> print a
>>>> debug message.
>>>> > +#endif
>>>> >
>>>> >       hccr = (struct xhci_hccr *)priv->gen_priv.base;
>>>> >       hcor = (struct xhci_hcor *)(priv->gen_priv.base +
>>>> >                       HC_LENGTH(xhci_readl(&(hccr)->cr_capbase)));
>>>> >
>>>> >       rc = xhci_register(dev, hccr, hcor);
>>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>> >       if (rc)
>>>> >               regulator_set_enable_if_allowed(priv->vbus_supply,
>>>> false);
>>>> > +#endif
>>>> >
>>>> >       return rc;
>>>> >  }
>>>> > @@ -265,14 +269,18 @@ static int dwc3_generic_host_probe(struct
>>>> udevice *dev)
>>>> >  static int dwc3_generic_host_remove(struct udevice *dev)
>>>> >  {
>>>> >       struct dwc3_generic_host_priv *priv = dev_get_priv(dev);
>>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>> >       int rc;
>>>> > +#endif
>>>> >
>>>> >       /* This function always returns 0 */
>>>> >       xhci_deregister(dev);
>>>> >
>>>> > +#if CONFIG_IS_ENABLED(DM_REGULATOR)
>>>> >       rc = regulator_set_enable_if_allowed(priv->vbus_supply, false);
>>>> >       if (rc)
>>>> >               debug("%s: Failed to disable vbus regulator: %d\n",
>>>> dev->name, rc);
>>>> > +#endif
>>>> >
>>>> >       return dwc3_generic_remove(dev, &priv->gen_priv);
>>>> >  }
>>>> 
>>>> --
>>>> // Caleb (they/them)