[PATCH v3 03/20] drivers: reset: Handle gracefully NULL pointers

[Simon Glass]

Hi Kishon,

On Tue, 4 May 2021 at 21:25, Kishon Vijay Abraham I <kishon at ti.com> wrote:
>
> Hi Simon,
>
> On 04/05/21 10:28 pm, Simon Glass wrote:
> > Hi Kishon,
> >
> > On Tue, 4 May 2021 at 04:42, Kishon Vijay Abraham I <kishon at ti.com> wrote:
> >>
> >> The reset framework provides devm_reset_control_get_optional()
> >> which can return NULL (not an error case). So all the other reset_ops
> >> should handle NULL gracefully. Prepare the way for a managed reset
> >> API by handling NULL pointers without crashing nor failing.
> >>
> >> Signed-off-by: Vignesh Raghavendra <vigneshr at ti.com>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon at ti.com>
> >> ---
> >>  drivers/reset/reset-uclass.c | 35 ++++++++++++++++++++++++++++++-----
> >>  1 file changed, 30 insertions(+), 5 deletions(-)
> >
> > I am still not a fan of this. There is no way to know whether the
> > reset API call actually did anything. Normally we return -EINVAL or
> > something like that when the value is invalid (e.g. see GPIO uclass).
>
> The reset modification is more in-line with how clk uclass is designed.
>
> There every API first checks if the clock is valid and returns 0 (will
> be the case for optional clocks)
>        if (!clk_valid(clk))
>                 return 0;
>
> If you don't prefer this approach, I'll drop this patch and handle it
> appropriately in the client driver (serdes driver). Please let me know.

Yes I see that with clk. IMO the return code is incorrect there also,
since it should be -ENOENT or something like that, to indicate that
there is actually no clock.

The clk.h header gives no indication that it accepts a NULL clock. So
here is what I think:

1. Update clk_valid() to require clk to be a valid pointer, so it is
considered invalid only if clk-dev.

2. Update the reset 'managed API' to return an empty reset record
instead of NULL.

3. Your problem goes away, and (I think) we are still compatible with
the linux API.

>
> >
> > The function you mention says:  * Returns a struct reset_ctl or a
> > dummy reset controller if it failed.
> >
> > But it does not appear to do that?
>
> Right, looks like it's incorrect from when the patch was actually
> introduced. It was returning "NULL" for optional resets.
>
> commit 139e4a1cbe60de96d4febbc6db5882929801ca46
> Author: Jean-Jacques Hiblot <jjhiblot at ti.com>
> Date:   Wed Sep 9 15:37:03 2020 +0530
>
>     drivers: reset: Add a managed API to get reset controllers from the DT

OK. So what should it be?

Regards,
Simon