Hi Kishon,
On Thu, 6 May 2021 at 07:09, Kishon Vijay Abraham I <kis...@ti.com> wrote:
>
> Hi Simon,
>
> On 06/05/21 5:07 am, Simon Glass wrote:
> > Hi Kishon,
> >
> > On Tue, 4 May 2021 at 21:25, Kishon Vijay Abraham I <kis...@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 <kis...@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 <vigne...@ti.com>
> >>>> Signed-off-by: Kishon Vijay Abraham I <kis...@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.
>
> So for API's like clk_enable(), clk_disable(), should it return 0 or
> -ENOENT; i.e if clk is valid and clk->dev is NULL?
> >
> > 2. Update the reset 'managed API' to return an empty reset record
> > instead of NULL.
>
> Okay, something like below?
>
> --- a/drivers/reset/reset-uclass.c
> +++ b/drivers/reset/reset-uclass.c
> @@ -326,9 +326,6 @@ struct reset_ctl
> *devm_reset_control_get_optional(struct udevice *dev,
> {
> struct reset_ctl *r = devm_reset_control_get(dev, id);
>
> - if (IS_ERR(r))
> - return NULL;
> -
> return r;
> }
>
> The only thing to note here is _get_optional() is a wrapper to _get()
> with no modification.
OK.
> >
> > 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 <jjhib...@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?
>
> I meant the comment in header is incorrect since it was returning NULL
> if it failed.
OK, well then you could add a patch for it. The problem with a pointer
value is that you cannot both communicate an error and return a valid
pointer.
Regards,
Simon
