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

[Quentin Schulz]

Hi Lukasz,

On 5/26/25 4:24 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 | 65 ++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 49 insertions(+), 16 deletions(-)
> 
> diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
> index dcaa24d30ece..2d2114ea6d17 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,53 @@ 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;
> +	int i;
> +
> +	for (i = data->num_supplies - 1; i >= 0; i--)
> +		if (hub->vdd[i])
> +			ret |= regulator_set_enable_if_allowed(hub->vdd[i], false);
> +

I would suggest to print here the failing regulators which I find more 
helpful than printing a global "something went wrong when powering off 
regulators, here's the joined error code, if there are more than one 
failing, good luck" kind of message in the caller.

> +	return ret;
> +}
> +
>   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);
> -			return 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) {

Please check for -ENOSYS as well, like in 77b053502f39 ("usb: 
onboard-hub: Fix return type for regulator APIs") (which is the reason 
why your patch doesn't apply cleanly anymore as far as I could tell?).

Looks ok to me otherwise.

Cheers,
Quentin