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. > > 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. Thanks, Kishon