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.czechow...@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?
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?
}
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
