[PATCH 0/2] reset: Fix the behavior of optional reset

[PATCH 0/2] reset: Fix the behavior of optional reset

[Kishon Vijay Abraham I]

This series is a result of discussion in [1]

This series does not return NULL for optional resets so that the client
driver knows whether it had obtained a reset or not.

If it returns -ENODATA, that would mean reset controller is not
obtained. However the reset API's wil not abort or throw error and
handle gracefully if it's passed -ENODATA.

[1] -> https://patchwork.ozlabs.org/project/uboot/patch/20210504104155.19222-4-kishon at ti.com/

Kishon Vijay Abraham I (2):
  reset: Do not return NULL on error for
    devm_reset_control_get_optional()
  reset: Let reset API's handle gracefully if reset_ctl is -ENODATA

 drivers/reset/reset-uclass.c       | 51 ++++++++++++++++++------------
 drivers/reset/sandbox-reset-test.c |  2 +-
 2 files changed, 32 insertions(+), 21 deletions(-)

-- 
2.17.1

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Kishon Vijay Abraham I]

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 <kishon@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);
 }
 
 static void devm_reset_bulk_release(struct udevice *dev, void *res)
@@ -337,14 +332,7 @@ struct reset_ctl_bulk *devm_reset_bulk_get_by_node(struct udevice *dev,
 struct reset_ctl_bulk *devm_reset_bulk_get_optional_by_node(struct udevice *dev,
 							    ofnode node)
 {
-	struct reset_ctl_bulk *bulk;
-
-	bulk = devm_reset_bulk_get_by_node(dev, node);
-
-	if (IS_ERR(bulk))
-		return NULL;
-
-	return bulk;
+	return devm_reset_bulk_get_by_node(dev, node);
 }
 
 struct reset_ctl_bulk *devm_reset_bulk_get(struct udevice *dev)
diff --git a/drivers/reset/sandbox-reset-test.c b/drivers/reset/sandbox-reset-test.c
index 51b79810c8..2a810fda3a 100644
--- a/drivers/reset/sandbox-reset-test.c
+++ b/drivers/reset/sandbox-reset-test.c
@@ -38,7 +38,7 @@ int sandbox_reset_test_get_devm(struct udevice *dev)
 		return -EINVAL;
 
 	r = devm_reset_control_get_optional(dev, "not-a-valid-reset-ctl");
-	if (r)
+	if (IS_ERR(r) && PTR_ERR(r) != -ENODATA)
 		return -EINVAL;
 
 	sbrt->ctlp = devm_reset_control_get(dev, "test");
-- 
2.17.1

[PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA

[Kishon Vijay Abraham I]

Let reset API's (like reset_assert(), reset_deassert(), ..) handle
gracefully if the argument reset_ctl is -ENODATA. This is for seamlessly
handling client drivers which get optional reset controller using
devm_reset_control_get_optional().

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/reset/reset-uclass.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
index 906f58ce3d..077ca956f4 100644
--- a/drivers/reset/reset-uclass.c
+++ b/drivers/reset/reset-uclass.c
@@ -162,7 +162,12 @@ int reset_get_by_name(struct udevice *dev, const char *name,
 
 int reset_request(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops;
+
+	if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
+		return 0;
+
+	ops = reset_dev_ops(reset_ctl->dev);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
@@ -171,7 +176,12 @@ int reset_request(struct reset_ctl *reset_ctl)
 
 int reset_free(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops;
+
+	if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
+		return 0;
+
+	ops = reset_dev_ops(reset_ctl->dev);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
@@ -180,7 +190,12 @@ int reset_free(struct reset_ctl *reset_ctl)
 
 int reset_assert(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops;
+
+	if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
+		return 0;
+
+	ops = reset_dev_ops(reset_ctl->dev);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
@@ -202,7 +217,12 @@ int reset_assert_bulk(struct reset_ctl_bulk *bulk)
 
 int reset_deassert(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops;
+
+	if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
+		return 0;
+
+	ops = reset_dev_ops(reset_ctl->dev);
 
 	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
 
@@ -224,9 +244,12 @@ int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
 
 int reset_status(struct reset_ctl *reset_ctl)
 {
-	struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
+	struct reset_ops *ops;
 
-	debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
+	if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
+		return 0;
+
+	ops = reset_dev_ops(reset_ctl->dev);
 
 	return ops->rst_status(reset_ctl);
 }
-- 
2.17.1

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Simon Glass]

Hi Kishon,

On Fri, 7 May 2021 at 05:02, Kishon Vijay Abraham I <kishon@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 <kishon@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.

I thought these functions were going to return a valid (but possibly
empty) reset_ctl always?

>
>  static void devm_reset_bulk_release(struct udevice *dev, void *res)
> @@ -337,14 +332,7 @@ struct reset_ctl_bulk *devm_reset_bulk_get_by_node(struct udevice *dev,
>  struct reset_ctl_bulk *devm_reset_bulk_get_optional_by_node(struct udevice *dev,
>                                                             ofnode node)
>  {
> -       struct reset_ctl_bulk *bulk;
> -
> -       bulk = devm_reset_bulk_get_by_node(dev, node);
> -
> -       if (IS_ERR(bulk))
> -               return NULL;
> -
> -       return bulk;
> +       return devm_reset_bulk_get_by_node(dev, node);
>  }
>
>  struct reset_ctl_bulk *devm_reset_bulk_get(struct udevice *dev)
> diff --git a/drivers/reset/sandbox-reset-test.c b/drivers/reset/sandbox-reset-test.c
> index 51b79810c8..2a810fda3a 100644
> --- a/drivers/reset/sandbox-reset-test.c
> +++ b/drivers/reset/sandbox-reset-test.c
> @@ -38,7 +38,7 @@ int sandbox_reset_test_get_devm(struct udevice *dev)
>                 return -EINVAL;
>
>         r = devm_reset_control_get_optional(dev, "not-a-valid-reset-ctl");
> -       if (r)
> +       if (IS_ERR(r) && PTR_ERR(r) != -ENODATA)
>                 return -EINVAL;
>
>         sbrt->ctlp = devm_reset_control_get(dev, "test");
> --
> 2.17.1
>

Regards,
SImon

[PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA

[Simon Glass]

Hi Kishon,

On Fri, 7 May 2021 at 05:02, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>
> Let reset API's (like reset_assert(), reset_deassert(), ..) handle
> gracefully if the argument reset_ctl is -ENODATA. This is for seamlessly
> handling client drivers which get optional reset controller using
> devm_reset_control_get_optional().
>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/reset/reset-uclass.c | 35 +++++++++++++++++++++++++++++------
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
> index 906f58ce3d..077ca956f4 100644
> --- a/drivers/reset/reset-uclass.c
> +++ b/drivers/reset/reset-uclass.c
> @@ -162,7 +162,12 @@ int reset_get_by_name(struct udevice *dev, const char *name,
>
>  int reset_request(struct reset_ctl *reset_ctl)
>  {
> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +       struct reset_ops *ops;
> +
> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
> +               return 0;

This should call reset_valid(), not do things here. Also I thought we
came to the conclusion that reset_ctl would always be a valid pointer?

In any case, if you change the logic of these function, you must also
change the header, otherwise there is no docs.

Finally, can you please update the tests?

> +
> +       ops = reset_dev_ops(reset_ctl->dev);
>
>         debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>
> @@ -171,7 +176,12 @@ int reset_request(struct reset_ctl *reset_ctl)
>
>  int reset_free(struct reset_ctl *reset_ctl)
>  {
> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +       struct reset_ops *ops;
> +
> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
> +               return 0;
> +
> +       ops = reset_dev_ops(reset_ctl->dev);
>
>         debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>
> @@ -180,7 +190,12 @@ int reset_free(struct reset_ctl *reset_ctl)
>
>  int reset_assert(struct reset_ctl *reset_ctl)
>  {
> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +       struct reset_ops *ops;
> +
> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
> +               return 0;
> +
> +       ops = reset_dev_ops(reset_ctl->dev);
>
>         debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>
> @@ -202,7 +217,12 @@ int reset_assert_bulk(struct reset_ctl_bulk *bulk)
>
>  int reset_deassert(struct reset_ctl *reset_ctl)
>  {
> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +       struct reset_ops *ops;
> +
> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
> +               return 0;
> +
> +       ops = reset_dev_ops(reset_ctl->dev);
>
>         debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
>
> @@ -224,9 +244,12 @@ int reset_deassert_bulk(struct reset_ctl_bulk *bulk)
>
>  int reset_status(struct reset_ctl *reset_ctl)
>  {
> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> +       struct reset_ops *ops;
>
> -       debug("%s(reset_ctl=%p)\n", __func__, reset_ctl);
> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
> +               return 0;
> +
> +       ops = reset_dev_ops(reset_ctl->dev);
>
>         return ops->rst_status(reset_ctl);
>  }
> --
> 2.17.1
>

Regards,
Simon

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Kishon Vijay Abraham I]

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 <kishon@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 <kishon@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.

Thanks
Kishon

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Simon Glass]

Hi Kishon,

On Tue, 11 May 2021 at 00:26, Kishon Vijay Abraham I <kishon@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 <kishon@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 <kishon@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?

Regards,
Simon

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Kishon Vijay Abraham I]

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 <kishon@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 <kishon@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 <kishon@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;
}

I can add reset_valid() and add the conditional checks corresponding to
the changes made for reset (like return -ENODATA instead of 0).

Thanks
Kishon

[PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA

[Kishon Vijay Abraham I]

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 <kishon@ti.com> wrote:
>>
>> Let reset API's (like reset_assert(), reset_deassert(), ..) handle
>> gracefully if the argument reset_ctl is -ENODATA. This is for seamlessly
>> handling client drivers which get optional reset controller using
>> devm_reset_control_get_optional().
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/reset/reset-uclass.c | 35 +++++++++++++++++++++++++++++------
>>  1 file changed, 29 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
>> index 906f58ce3d..077ca956f4 100644
>> --- a/drivers/reset/reset-uclass.c
>> +++ b/drivers/reset/reset-uclass.c
>> @@ -162,7 +162,12 @@ int reset_get_by_name(struct udevice *dev, const char *name,
>>
>>  int reset_request(struct reset_ctl *reset_ctl)
>>  {
>> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
>> +       struct reset_ops *ops;
>> +
>> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
>> +               return 0;
> 
> This should call reset_valid(), not do things here. Also I thought we
> came to the conclusion that reset_ctl would always be a valid pointer?
> 
> In any case, if you change the logic of these function, you must also
> change the header, otherwise there is no docs.
> 
> Finally, can you please update the tests?

Patch 1 updates sandbox_reset_test_get_devm() in sandbox-reset-test.c.
That's the only test I saw was using optional reset APIs. Do you think
any thing else needs to be updated?

Thanks
Kishon

[PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA

[Simon Glass]

Hi Kishon,

On Thu, 13 May 2021 at 00:30, Kishon Vijay Abraham I <kishon@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 <kishon@ti.com> wrote:
> >>
> >> Let reset API's (like reset_assert(), reset_deassert(), ..) handle
> >> gracefully if the argument reset_ctl is -ENODATA. This is for seamlessly
> >> handling client drivers which get optional reset controller using
> >> devm_reset_control_get_optional().
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/reset/reset-uclass.c | 35 +++++++++++++++++++++++++++++------
> >>  1 file changed, 29 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/reset/reset-uclass.c b/drivers/reset/reset-uclass.c
> >> index 906f58ce3d..077ca956f4 100644
> >> --- a/drivers/reset/reset-uclass.c
> >> +++ b/drivers/reset/reset-uclass.c
> >> @@ -162,7 +162,12 @@ int reset_get_by_name(struct udevice *dev, const char *name,
> >>
> >>  int reset_request(struct reset_ctl *reset_ctl)
> >>  {
> >> -       struct reset_ops *ops = reset_dev_ops(reset_ctl->dev);
> >> +       struct reset_ops *ops;
> >> +
> >> +       if (IS_ERR(reset_ctl) && PTR_ERR(reset_ctl) == -ENODATA)
> >> +               return 0;
> >
> > This should call reset_valid(), not do things here. Also I thought we
> > came to the conclusion that reset_ctl would always be a valid pointer?
> >
> > In any case, if you change the logic of these function, you must also
> > change the header, otherwise there is no docs.
> >
> > Finally, can you please update the tests?
>
> Patch 1 updates sandbox_reset_test_get_devm() in sandbox-reset-test.c.
> That's the only test I saw was using optional reset APIs. Do you think
> any thing else needs to be updated?

My point is that if you are changing the logic of reset_request(), you
should add a test for it and you need to update the docs.

Do you have someone at TI that can work through this with you? I am
not the maintainer of this subsystem, nor the expert here, but I feel
it could use a review with someone else as well, to really nut out all
the issues once and for all.

Regards,
Simon

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Simon Glass]

Hi Kishon,

On Thu, 13 May 2021 at 00:15, Kishon Vijay Abraham I <kishon@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 <kishon@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 <kishon@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 <kishon@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.

But all of your patches are about a function that returns a pointer,
which is a different thing....

> 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.

Regards,
Simon

[PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Kishon Vijay Abraham I]

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 <kishon@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 <kishon@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 <kishon@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 <kishon@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

Re: [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Pratyush Yadav]

Hi,

On 14/05/21 10:48AM, Kishon Vijay Abraham I wrote:
> 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 <kishon@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 <kishon@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 <kishon@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 <kishon@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.

I have been reading through this discussion, and there seems to be a lot 
of confusion and lots of ideas floating around. I think the first thing 
to decide is _what_ will the opional reset API do. Here is what I think 
it should do. The point of the optional reset is that the driver does 
not care if the reset is present or not. If the reset is present, then 
do as it requests. If it is not present, do nothing. If the reset is 
present and it cannot be obtained for some reason, tell the driver about 
it.

With that in mind, we can decide on how the API should work. I see three 
distinct classes come up: reset is successfully obtained, there as an 
error, reset is not present. The first class can be represented by a 
valid pointer. The second can be represented by ERR_PTR(). The third can 
be represented by NULL. Making this separation will let the code neatly 
take care of each class. Using ERR_PTR(-ENODATA) mixes up the "error" 
and "not present" classes. if (!rst) is much cleaner than if 
(PTR_ERR(rst) == -ENODATA).

I think the waters have been muddied because the "normal" reset APIs 
require the caller to pass in the reset struct to fill, and the devm and 
optional APIs allocate the struct themselves and return it. So the check 
for whether a reset is valid changes based on which of the two APIs was 
used to get the reset. Linux has been allocating a struct reset_control 
and returning the pointer from the get go so they don't have this 
problem. I think it is reasonable for U-Boot to update reset_valid() to 
check for pointer validity to accomodate both API styles.

So here is my suggestion summarized: return NULL when PTR_ERR() == 
-ENODATA. In all other cases return the pointer as it is. Then in all 
reset operations like reset_assert(), etc. check if the pointer passed 
in is NULL. If it is, do nothing. If it is an error pointer, return 
-EINVAL. Otherwise run the function like normal. reset_valid() should 
also be updated check for IS_ERR_OR_NULL().

> > 
> > 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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional()

[Simon Glass]

Hi Pratyush,

On Tue, 8 Jun 2021 at 04:40, Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi,
>
> On 14/05/21 10:48AM, Kishon Vijay Abraham I wrote:
> > 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 <kishon@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 <kishon@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 <kishon@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 <kishon@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.
>
> I have been reading through this discussion, and there seems to be a lot
> of confusion and lots of ideas floating around. I think the first thing
> to decide is _what_ will the opional reset API do. Here is what I think
> it should do. The point of the optional reset is that the driver does
> not care if the reset is present or not. If the reset is present, then
> do as it requests. If it is not present, do nothing. If the reset is
> present and it cannot be obtained for some reason, tell the driver about
> it.
>
> With that in mind, we can decide on how the API should work. I see three
> distinct classes come up: reset is successfully obtained, there as an
> error, reset is not present. The first class can be represented by a
> valid pointer. The second can be represented by ERR_PTR(). The third can
> be represented by NULL. Making this separation will let the code neatly
> take care of each class. Using ERR_PTR(-ENODATA) mixes up the "error"
> and "not present" classes. if (!rst) is much cleaner than if
> (PTR_ERR(rst) == -ENODATA).
>
> I think the waters have been muddied because the "normal" reset APIs
> require the caller to pass in the reset struct to fill, and the devm and
> optional APIs allocate the struct themselves and return it. So the check
> for whether a reset is valid changes based on which of the two APIs was
> used to get the reset. Linux has been allocating a struct reset_control
> and returning the pointer from the get go so they don't have this
> problem. I think it is reasonable for U-Boot to update reset_valid() to
> check for pointer validity to accomodate both API styles.
>
> So here is my suggestion summarized: return NULL when PTR_ERR() ==
> -ENODATA. In all other cases return the pointer as it is. Then in all
> reset operations like reset_assert(), etc. check if the pointer passed
> in is NULL. If it is, do nothing. If it is an error pointer, return
> -EINVAL. Otherwise run the function like normal. reset_valid() should
> also be updated check for IS_ERR_OR_NULL().

Thank you for the effort and explanation. This helps a lot.

In driver model U-Boot uses a separate arg for functions that return a
pointer. The return value is an int and indicates an error. This is a
design decision that was made early on. It is slightly less efficient,
although checking for (x >= 0xffff0000) is not free either. It avoids
the PTR_ERR() and ERR_PTR() stuff so the code is more readable. It
allows use of addresses at the top of address space, although this is
seldom useful. It also ensures that NULL is not a valid pointer, which
is likely appreciated by coverity and the like. Making the caller pass
in the pointer avoids malloc() which is also a design decision in
U-Boot. But mostly it is about readability.

That said, we aim for compatibility with Linux APIs, and devm is
imported from linux. So U-Boot's devm also uses the error-return
thing.

In the interests of linux compatibility I think it is somewhat
defensible for reset_valid() to check for a NULL pointer. However I
don't like returning errors from these functions. There is no way in
reset_valid() to know if the thing passed in came from a devm function
or a normal reset function. So if we check for IS_ERR() in
reset_valid() then we are forcing the linux convention onto the normal
reset calls.

To me a better approach is to return a valid 'struct reset_ctl'
pointer, but one where reset_valid() will return false (i.e. ->dev is
NULL). We can have one of these in the devm code, perhaps static, and
return the same one in all cases where there is a failure.

Do you need to get the error code from devm functions? Presumably if
it is -ENOENT then it means there is no reset but it is OK. Any other
error indicates something has gone wrong.

If you don't need the error code, then the above is all we need. If
you do need the error code, then I wonder whether we need something
that converts a struct reset_ctl (as returned by a devm function) into
an error and a reset_ctl? Then we can call this to interpret the
result from the devm function.

e.g.

static reset_ctl devm_no_reset;

int devm_to_reset(struct reset_ctl *in, struct reset_ctl **outp)
{
   if (IS_ERR_OR_NULL(in)) {
      *outp = &devm_no_reset;
      if (!in)
         return -ENOENT;
      else
         return PTR_ERR(in);
   }
   *outp = in;

   return 0;
}

The above (ugly) logic is what is being carried around in that pointer
variable, since it has three types of value:

- valid reset
- invalid reset but that's OK
- invalid reset due to an error

So sample code would change from:

   struct reset_ctl *reset;
   int ret;

   reset = devm_reset_control_get(dev, "myreset");
   if (IS_ERR(reset))
      return log_msg_ret("reset", -ENOENT);

to:

   struct reset_ctl *rerr, *reset;
   int ret;

   rerr = devm_reset_control_get(dev, "myreset");
   ret = devm_to_reset(rerr, &reset);
   if (ret)
      return log_msg_ret("reset", ret);

I think the above helps join the linux and U-Boot APIs without too
much pain. I will note that there is no use of the devm_reset_... API
in U-Boot, apart from in tests, so this cannot be described as an
critical feature.

What do you think?

Regards,
Simon


>
> > >
> > > 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
>
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.