[PATCH] usb: dwc3-generic: fix CONFIG_DM_REGULATOR-off case

[Jan Kiszka]

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 at 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 at 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 at linaro.org>

Can this regression fix be merged now, or should I add that gating?

Jan

-- 
Siemens AG, Technology
Linux Expert Center