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

[Quentin Schulz]

Hi Lukasz,

On 6/10/25 10:02 AM, 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 | 67 ++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 50 insertions(+), 17 deletions(-)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index 046831d09668..273b626dbaee 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);
>   };
>   
> @@ -139,30 +143,58 @@ int usb_onboard_hub_reset(struct udevice *dev)
>   	return 0;
>   }
>   
> +static int usb_onboard_hub_power_off(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);
> +	int ret = 0, ret2;
> +	int i;

Could be on the same line with ret and ret2.

I assume this was maybe due to using an unsigned int in the beginning 
but then the descending for-loop just below would never return because 
"i" would never be less than 0?

An option could be to have i represent the index + 1 and then whenever 
you use i to access an item in the array, you use i-1 instead.

So something like

for (i = data->num_supplies; i > 0; i--) {
     if (hub->vdd[i - 1]) {
         ret2 = regulator_set_enable_if_allowed(hub->vdd[i - 1], false);
         if (ret2 && ret2 != -ENOSYS) {
             dev_err(dev, "can't disable %s: %d\n", data->supply_names[i 
- 1], ret2);
             ret |= ret2;
         }
     }
}

instead maybe? No strong opinion on my side on that. We could probably 
even go for an u8 instead since it's very unlikely we'll have more than 
255 supplies for a given hub :)

> +
> +	for (i = data->num_supplies - 1; i >= 0; i--)
> +		if (hub->vdd[i]) {
> +			ret2 = regulator_set_enable_if_allowed(hub->vdd[i], false);
> +			if (ret2 && ret2 != -ENOSYS) {
> +				dev_err(dev, "can't disable %s: %d\n", data->supply_names[i], ret2);
> +				ret |= ret2;
> +			}
> +		}
> +

Please have the for-loop have curly brackets.

Aside from those minor nitpicks, looks good to me so:

Reviewed-by: Quentin Schulz <quentin.schulz at cherry.de>

Thanks!
Quentin