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.czechow...@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.sch...@cherry.de>
Thanks!
Quentin
