Hi Simon,
On 14/05/21 5:26 am, Simon Glass wrote:
> Hi Kishon,
>
> On Thu, 13 May 2021 at 00:15, Kishon Vijay Abraham I <kis...@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 <kis...@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 <kis...@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 <kis...@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.
hmm, I pointed to the wrong function from clk-uclass. The below clk
function is similar to our reset usecase
struct clk *devm_clk_get_optional(struct udevice *dev, const char *id)
{
struct clk *clk = devm_clk_get(dev, id);
if (PTR_ERR(clk) == -ENODATA)
return NULL;
return clk;
}
For this case, clk_valid() works because it checks for "clk" pointer and
for ENODATA case (get_optional), the client will have clk = NULL and
pass NULL to all the clock APIs.
IIUC, you don't prefer returning NULL to client on ENODATA (or any other
error)? And that's why unlike devm_clk_get_optional() which returns NULL
on ENODATA, $patch directly returns devm_reset_control_get().
So for reset it could return
1) valid reset_ctl pointer with dev initialized
2) -ENOMEM (if allocation of reset_ctl itself fails)
3) -ENODATA (if reset-names is not populated in DT)
4) -ENOENT/-EINVAL (for other DT errors)
clk-uclass has a separate class of APIs that ends with _nodev(). Here
the client allocates memory for clk and let clk framework populate
clk->dev. For the other APIs where clk framework itself returns clk
pointer, there is no case where clk framework returns a valid clk
pointer but without clk->dev initialized (the second case is what we
deal in reset).
Since reset at this point doesn't deal with _nodev(), the latter case
above for clk is what should be considered (i.e reset returns valid
reset_ctrl pointer with dev initialized or return an error value).
So again coming back to the return values mentioned above [ 1) valid
reset, 2) -ENOMEM, 3) -ENODATA, 4) -ENOENT/-EINVAL ].
For get_optional()
1) Should we return NULL on -ENODATA (similar to devm_clk_get_optional())?
2) If we decide to return error value instead of NULL, the check would
simply move to client and reset_valid(). i.e The client would check
return value of get_optional() and if it's -ENODDATA, it's not going to
error out and will also pass -ENODATA to reset APIs. The reset APIs will
check again if it's ENODATA and handle it gracefully (to be done in
reset_valid()).
3) We return NULL for optional() similar to devm_clk_get_optional()
and not consider the _nodev() APIs. Here reset_valid() will only check
if reset_ctrl is NULL or not.
>
> But all of your patches are about a function that returns a pointer,
> which is a different thing....
Sorry about this. I pointed to the wrong function creating more confusion.
>
>> 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.
I hope I've explained the differences between clk and reset above.
Please see and provide your feedback.
Thanks
Kishon
