[PATCH] usb: dwc3-generic: fix CONFIG_DM_REGULATOR-off case
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com --- drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc;
rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); - if (rc) + if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc);
- /* Only returns an error if regulator is valid and failed to enable due to a driver issue */ + /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); - if (rc) + if (rc && rc != -ENOSYS) return rc;
hccr = (struct xhci_hccr *)priv->gen_priv.base;
Hi Jan,
On 08/08/2024 10:51, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc;
rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply);
- if (rc)
- if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc);
- /* Only returns an error if regulator is valid and failed to enable due to a driver issue */
- /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true);
- if (rc)
- if (rc && rc != -ENOSYS)
regulator_set_enable_if_allowed() will return 0 if the call to regulator_set_enable() returns -ENOSYS
Maybe it would make sense to have the stub implementation of regulator_set_enable_if_allowed() return 0?
Somewhat confusing to check for -ENOSYS here imo, since it isn't really obvious when that would be the case.
return rc;hccr = (struct xhci_hccr *)priv->gen_priv.base;
On 08.08.24 16:27, Caleb Connolly wrote:
Hi Jan,
On 08/08/2024 10:51, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc; rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); - if (rc) + if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc); - /* Only returns an error if regulator is valid and failed to enable due to a driver issue */ + /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); - if (rc) + if (rc && rc != -ENOSYS)
regulator_set_enable_if_allowed() will return 0 if the call to regulator_set_enable() returns -ENOSYS
Maybe it would make sense to have the stub implementation of regulator_set_enable_if_allowed() return 0?
Possible. Would that be the only case where a stub should not return ENOSYS? All do so far.
Somewhat confusing to check for -ENOSYS here imo, since it isn't really obvious when that would be the case.
But that would still leave is a with a misleading message, even if it is just a debug output. The absence of a regulator is not per se a bug to my understanding.
Jan
return rc; hccr = (struct xhci_hccr *)priv->gen_priv.base;
On 09/08/2024 07:19, Jan Kiszka wrote:
On 08.08.24 16:27, Caleb Connolly wrote:
Hi Jan,
On 08/08/2024 10:51, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc; rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); - if (rc) + if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc); - /* Only returns an error if regulator is valid and failed to enable due to a driver issue */ + /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); - if (rc) + if (rc && rc != -ENOSYS)
regulator_set_enable_if_allowed() will return 0 if the call to regulator_set_enable() returns -ENOSYS
Maybe it would make sense to have the stub implementation of regulator_set_enable_if_allowed() return 0?
Possible. Would that be the only case where a stub should not return ENOSYS? All do so far.
Somewhat confusing to check for -ENOSYS here imo, since it isn't really obvious when that would be the case.
But that would still leave is a with a misleading message, even if it is just a debug output. The absence of a regulator is not per se a bug to my understanding.
Ah good point.
Alternatively, maybe it would be easier to gate this code block with if (CONFIG_IS_ENABLED(DM_REGULATOR)?
But yeah maybe fine as is.
Reviewed-by: Caleb Connolly caleb.connolly@linaro.org
Jan
return rc; hccr = (struct xhci_hccr *)priv->gen_priv.base;
On 09.08.24 17:19, Caleb Connolly wrote:
On 09/08/2024 07:19, Jan Kiszka wrote:
On 08.08.24 16:27, Caleb Connolly wrote:
Hi Jan,
On 08/08/2024 10:51, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc; rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); - if (rc) + if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc); - /* Only returns an error if regulator is valid and failed to enable due to a driver issue */ + /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); - if (rc) + if (rc && rc != -ENOSYS)
regulator_set_enable_if_allowed() will return 0 if the call to regulator_set_enable() returns -ENOSYS
Maybe it would make sense to have the stub implementation of regulator_set_enable_if_allowed() return 0?
Possible. Would that be the only case where a stub should not return ENOSYS? All do so far.
Somewhat confusing to check for -ENOSYS here imo, since it isn't really obvious when that would be the case.
But that would still leave is a with a misleading message, even if it is just a debug output. The absence of a regulator is not per se a bug to my understanding.
Ah good point.
Alternatively, maybe it would be easier to gate this code block with if (CONFIG_IS_ENABLED(DM_REGULATOR)?
But yeah maybe fine as is.
Reviewed-by: Caleb Connolly caleb.connolly@linaro.org
Can this regression fix be merged now, or should I add that gating?
Jan
On 30.09.24 06:54, Jan Kiszka wrote:
On 09.08.24 17:19, Caleb Connolly wrote:
On 09/08/2024 07:19, Jan Kiszka wrote:
On 08.08.24 16:27, Caleb Connolly wrote:
Hi Jan,
On 08/08/2024 10:51, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc; rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); - if (rc) + if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc); - /* Only returns an error if regulator is valid and failed to enable due to a driver issue */ + /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); - if (rc) + if (rc && rc != -ENOSYS)
regulator_set_enable_if_allowed() will return 0 if the call to regulator_set_enable() returns -ENOSYS
Maybe it would make sense to have the stub implementation of regulator_set_enable_if_allowed() return 0?
Possible. Would that be the only case where a stub should not return ENOSYS? All do so far.
Somewhat confusing to check for -ENOSYS here imo, since it isn't really obvious when that would be the case.
But that would still leave is a with a misleading message, even if it is just a debug output. The absence of a regulator is not per se a bug to my understanding.
Ah good point.
Alternatively, maybe it would be easier to gate this code block with if (CONFIG_IS_ENABLED(DM_REGULATOR)?
But yeah maybe fine as is.
Reviewed-by: Caleb Connolly caleb.connolly@linaro.org
Can this regression fix be merged now, or should I add that gating?
This now also missed the v2024.10 release - can we please merge it now?
Jan
On 10/10/24 7:36 AM, Jan Kiszka wrote:
On 30.09.24 06:54, Jan Kiszka wrote:
On 09.08.24 17:19, Caleb Connolly wrote:
On 09/08/2024 07:19, Jan Kiszka wrote:
On 08.08.24 16:27, Caleb Connolly wrote:
Hi Jan,
On 08/08/2024 10:51, Jan Kiszka wrote:
From: Jan Kiszka jan.kiszka@siemens.com
When DM_REGULATOR is disabled, all calls will return -ENOSYS. Account for that so that targets like the IOT2050 will work again.
Fixes: de451d5d5b6f ("usb: dwc3-generic: support external vbus regulator") Signed-off-by: Jan Kiszka jan.kiszka@siemens.com
drivers/usb/dwc3/dwc3-generic.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index a9ba315463c..2ab41cbae45 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -246,12 +246,12 @@ static int dwc3_generic_host_probe(struct udevice *dev) return rc; rc = device_get_supply_regulator(dev, "vbus-supply", &priv->vbus_supply); - if (rc) + if (rc && rc != -ENOSYS) debug("%s: No vbus regulator found: %d\n", dev->name, rc); - /* Only returns an error if regulator is valid and failed to enable due to a driver issue */ + /* Does not return an error if regulator is invalid - but does so when DM_REGULATOR is disabled */ rc = regulator_set_enable_if_allowed(priv->vbus_supply, true); - if (rc) + if (rc && rc != -ENOSYS)
regulator_set_enable_if_allowed() will return 0 if the call to regulator_set_enable() returns -ENOSYS
Maybe it would make sense to have the stub implementation of regulator_set_enable_if_allowed() return 0?
Possible. Would that be the only case where a stub should not return ENOSYS? All do so far.
Somewhat confusing to check for -ENOSYS here imo, since it isn't really obvious when that would be the case.
But that would still leave is a with a misleading message, even if it is just a debug output. The absence of a regulator is not per se a bug to my understanding.
Ah good point.
Alternatively, maybe it would be easier to gate this code block with if (CONFIG_IS_ENABLED(DM_REGULATOR)?
But yeah maybe fine as is.
Reviewed-by: Caleb Connolly caleb.connolly@linaro.org
Can this regression fix be merged now, or should I add that gating?
This now also missed the v2024.10 release - can we please merge it now?
Sorry. Applied to usb/master .
If there is a critical patch before release, please, ping me directly if there is a chance it will be missed.
participants (3)
-
Caleb Connolly -
Jan Kiszka -
Marek Vasut