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

[Simon Glass]

Hi Kishon,

On Thu, 13 May 2021 at 00:15, Kishon Vijay Abraham I <kishon at ti.com> wrote:
>
> 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;
> }
>

We must just be talking at cross-purposes. The function you show above
returns an error code, so there is no need for the caller to check clk
if an error is returned. For the -ENODATA case, note that
clk_get_by_name_nodev() sets clk->dev to NULL, thus ensuring that
clk_valid() works as expected.

But all of your patches are about a function that returns a pointer,
which is a different thing....

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

I'm a bit lost at this point...will look at what you send.

Regards,
Simon