Re: [PATCH v2] gpio: dwapb_gpio: Enable all clocks

On 8/15/26 4:05 PM, Ralph Siemsen wrote:
> On Sat, Aug 15, 2026 at 12:05:55AM +0200, Marek Vasut wrote:
>> On 8/14/26 8:29 PM, Ralph Siemsen wrote:
>>
>> [...]
>>
>>> @@ -132,12 +134,34 @@ static int gpio_dwapb_reset(struct udevice *dev)
>>>      return 0;
>>>  }
>>> +static int gpio_dwapb_enable_optional_clocks(struct udevice *dev)
>>> +{
>>> +    struct gpio_dwapb_priv *priv = dev_get_priv(dev);
>>> +    int ret;
>>> +
>>> +    ret = clk_get_bulk(dev, &priv->clks);
>>> +    if (ret) {
>>> +        /* Clocks are optional */
>>> +        if (ret != -ENOENT)
>>> +            dev_err(dev, "Failed to get clocks: %d\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = clk_enable_bulk(&priv->clks);
>>> +    if (ret)
>>> +        dev_err(dev, "Failed to enable clocks: %d\n", ret);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>  static int gpio_dwapb_probe(struct udevice *dev)
>>>  {
>>>      struct gpio_dev_priv *priv = dev_get_uclass_priv(dev);
>>>      struct gpio_dwapb_plat *plat = dev_get_plat(dev);
>>>      if (!plat) {
>>> +        gpio_dwapb_enable_optional_clocks(dev);
> 
>> Error is not propagated from this call.
> 
> This was intentional. All existing users work without explicitly 
> enabling the clock. They probably do it in board specific code, or in a 
> previous boot stage.

ENOENT (clock not found) is fine to convert to 0 and return that 0 to 
cover such cases. Other errors (e.g. ENOMEM?) should be propagated.

> While adding clk_enable() should be harmless, what if it fails on some 
> of the existing boards? I would rather not break them, this can be quite 
> hard to debug.

If the users care and test, the error will get found and fixed. Else, 
apply best effort, sigh.

>> Fail path that would disable the clock is missing in this function.
> 
> Okay, I can add this.
Thank you