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

[Caleb Connolly]

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.

> 
>>> > 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)

-- 
// Caleb (they/them)