[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Kishon Vijay Abraham I]

Hi Simon,

On 11/05/21 10:09 pm, Simon Glass wrote:
> Hi Kishon,
> 
> On Tue, 11 May 2021 at 00:26, Kishon Vijay Abraham I <kishon at ti.com> wrote:
>>
>> Hi Simon,
>>
>> On 07/05/21 10:04 pm, Simon Glass wrote:
>>> Hi Kishon,
>>>
>>> On Fri, 7 May 2021 at 05:02, Kishon Vijay Abraham I <kishon at ti.com> wrote:
>>>>
>>>> In order for client to know whether it was able to successfully get a
>>>> reset controller or not, do not return NULL on error for
>>>> devm_reset_control_get_optional() and
>>>> devm_reset_bulk_get_optional_by_node().
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon at ti.com>
>>>> ---
>>>>  drivers/reset/reset-uclass.c       | 16 ++--------------
>>>>  drivers/reset/sandbox-reset-test.c |  2 +-
>>>>  2 files changed, 3 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
>>>> index ac89eaf098..906f58ce3d 100644
>>>> --- a/drivers/reset/reset-uclass.c
>>>> +++ b/drivers/reset/reset-uclass.c
>>>> @@ -299,12 +299,7 @@ struct reset_ctl *devm_reset_control_get(struct udevice *dev, const char *id)
>>>>  struct reset_ctl *devm_reset_control_get_optional(struct udevice *dev,
>>>>                                                   const char *id)
>>>>  {
>>>> -       struct reset_ctl *r = devm_reset_control_get(dev, id);
>>>> -
>>>> -       if (IS_ERR(r))
>>>> -               return NULL;
>>>> -
>>>> -       return r;
>>>> +       return devm_reset_control_get(dev, id);
>>>>  }
>>>
>>> We need to get some updates to the API (function comments in the
>>> header) here. I'm not sure what the intent is.
>>
>> right, that has to be fixed.
>>>
>>> I thought these functions were going to return a valid (but possibly
>>> empty) reset_ctl always?
>>
>> I thought about it again and felt it might not be correct to return
>> reset_ctl always. The reset control is optional only if the consumer
>> node doesn't have populated reset properties.
>>
>> If we always return valid reset_ctl possibly with it's dev member
>> initialized or not initialized, it would not be possible to tell it's
>> not initialized because of the absence of reset property or due to some
>> other valid error.
> 
> reset_valid() should check if dev is NULL or not, like with clock and
> GPIO. Is that enough?

clock compares with ENODATA and return "0" (code snippet below). For
reset we are modifying this to return ENODATA itself and use it for
comparing in the reset APIs.

int clk_get_optional_nodev(ofnode node, const char *name, struct clk *clk)
{
        int ret;

        ret = clk_get_by_name_nodev(node, name, clk);
        if (ret == -ENODATA)
                return 0;

        return ret;
}

I can add reset_valid() and add the conditional checks corresponding to
the changes made for reset (like return -ENODATA instead of 0).

Thanks
Kishon