[PATCH 3/4] usb: onboard-hub: Add support for multiple power supplies

[Łukasz Czechowski]

Hi Quentin,

pt., 16 maj 2025 o 17:05 Quentin Schulz <quentin.schulz at cherry.de> napisał(a):
>
> Hi Lukasz,
>
> On 4/25/25 12:56 PM, Lukasz Czechowski wrote:
> > Some of the onboard hubs require multiple power supplies, so extend
> > the driver to support them.
> > The implementation is inspired by the kernel driver, as introduced
> > by commit [1] in the v6.10 kernel.
> >
> > [1] https://github.com/torvalds/linux/commit/ec1848cd5df426f57a7f6a8a6b95b69259c52cfc
> > Signed-off-by: Lukasz Czechowski <lukasz.czechowski at thaumatec.com>
> > ---
> >   common/usb_onboard_hub.c | 45 +++++++++++++++++++++++++++++++++------------
> >   1 file changed, 33 insertions(+), 12 deletions(-)
> >
> > diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> > index 325c274ed952..b8fa38a4111d 100644
> > --- a/common/usb_onboard_hub.c
> > +++ b/common/usb_onboard_hub.c
> > @@ -20,14 +20,18 @@
> >   #define USB5744_CONFIG_REG_ACCESS   0x0037
> >   #define USB5744_CONFIG_REG_ACCESS_LSB       0x99
> >
> > +#define MAX_SUPPLIES 2
> > +
> >   struct onboard_hub {
> > -     struct udevice *vdd;
> > +     struct udevice *vdd[MAX_SUPPLIES];
> >       struct gpio_desc *reset_gpio;
> >   };
> >
> >   struct onboard_hub_data {
> >       unsigned long reset_us;
> >       unsigned long power_on_delay_us;
> > +     unsigned int num_supplies;
> > +     const char * const supply_names[MAX_SUPPLIES];
> >       int (*init)(struct udevice *dev);
> >   };
> >
> > @@ -144,20 +148,28 @@ static int usb_onboard_hub_probe(struct udevice *dev)
> >       struct onboard_hub_data *data =
> >               (struct onboard_hub_data *)dev_get_driver_data(dev);
> >       struct onboard_hub *hub = dev_get_priv(dev);
> > +     unsigned int i;
> >       int ret;
> >
> > -     ret = device_get_supply_regulator(dev, "vdd-supply", &hub->vdd);
> > -     if (ret && ret != -ENOENT) {
> > -             dev_err(dev, "can't get vdd-supply: %d\n", ret);
> > -             return ret;
> > +     if (data->num_supplies > MAX_SUPPLIES) {
> > +             dev_err(dev, "invalid supplies number, max supported: %d\n", MAX_SUPPLIES);
> > +             return -EINVAL;
> >       }
> >
> > -     if (hub->vdd) {
> > -             ret = regulator_set_enable_if_allowed(hub->vdd, true);
> > -             if (ret && ret != -ENOSYS) {
> > -                     dev_err(dev, "can't enable vdd-supply: %d\n", ret);
> > +     for (i = 0; i < data->num_supplies; i++) {
> > +             ret = device_get_supply_regulator(dev, data->supply_names[i], &hub->vdd[i]);
> > +             if (ret && ret != -ENOENT) {
> > +                     dev_err(dev, "can't get %s: %d\n", data->supply_names[i], ret);
> >                       return ret;
> >               }
> > +
> > +             if (hub->vdd[i]) {
> > +                     ret = regulator_set_enable_if_allowed(hub->vdd[i], true);
> > +                     if (ret && ret != -ENOSYS) {
> > +                             dev_err(dev, "can't enable %s: %d\n", data->supply_names[i], ret);
> > +                             return ret;
> > +                     }
> > +             }
>
> I'm wondering if we shouldn't have all return ret; actually be goto err;
> instead? I would assume that the error path in the probe function should
> be really close to what we have in remove function?
>
> To that extent, before this patch even, I think we should probably
> dm_gpio_set_value() the reset line when there's an error so that the hub
> is held in reset?
>
> Additionally, I believe the dm_gpio_free() in the remove function is
> unnecessary because we request the gpio with a devm_ function which
> should call dm_gpio_free() whenever appropriate?
>

 I think it's worth improving, and this can be done in separate patch,

> Finally, specifically for this patch here, I believe we should disable
> all regulators in the opposite order when in the error path?
>
> Something like:
>
> err:
>      for (i = data->num_supplies - 1; i >= 0; i--) {
>          ret = regulator_set_enable_if_allowed(hub->vdd[i], false);
>          if (ret)
>              dev_err(dev, "can't disable %s: %d\n",
> data->supply_names[i], ret);
>      }
>
> ? what do you think?
>

Thanks for the comments. That's a good point. I'll update it in v2.

> >       }
> >
> >       ret = usb_onboard_hub_reset(dev);
> > @@ -208,7 +220,10 @@ static int usb_onboard_hub_bind(struct udevice *dev)
> >
> >   static int usb_onboard_hub_remove(struct udevice *dev)
> >   {
> > +     struct onboard_hub_data *data =
> > +             (struct onboard_hub_data *)dev_get_driver_data(dev);
> >       struct onboard_hub *hub = dev_get_priv(dev);
> > +     unsigned int i;
> >       int ret;
> >
> >       if (hub->reset_gpio) {
> > @@ -216,9 +231,11 @@ static int usb_onboard_hub_remove(struct udevice *dev)
> >               dm_gpio_free(hub->reset_gpio->dev, hub->reset_gpio);
> >       }
> >
> > -     ret = regulator_set_enable_if_allowed(hub->vdd, false);
> > -     if (ret)
> > -             dev_err(dev, "can't disable vdd-supply: %d\n", ret);
> > +     for (i = 0; i < data->num_supplies; i++) {
> > +             ret = regulator_set_enable_if_allowed(hub->vdd[i], false);
> > +             if (ret)
> > +                     dev_err(dev, "can't disable %s: %d\n", data->supply_names[i], ret);
> > +     }
> >
>
> The error/remove path is usually unwinding in opposite order than the
> normal path, so that would be looping from last supply to first. C.f.
> regulator_bulk_disable in the Linux kernel.
>
> >       return ret;
>
> This one's an issue now, it'll return 0 if the last
> regulator_set_enable_if_allowed is 0, overriding the return code from
> dm_gpio_set_value and earlier regulator_set_enable_if_allowed calls. We
> should probably |= them or return some appropriate hardcoded value if at
> least one failed.
>
> Cheers,
> Quentin

Best regards,
Lukasz