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 kishon@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@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@ti.com Signed-off-by: Kishon Vijay Abraham I kishon@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:
- 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?
- 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.
- 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@ti.com Date: Wed Sep 9 15:37:03 2020 +0530
drivers: reset: Add a managed API to get reset controllers from the DTOK. So what should it be?
I meant the comment in header is incorrect since it was returning NULL if it failed.
Thanks, Kishon