* [PATCH 0/2] reset: Fix the behavior of optional reset
@ 2021-05-07 11:02 Kishon Vijay Abraham I
2021-05-07 11:02 ` [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() Kishon Vijay Abraham I
2021-05-07 11:02 ` [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA Kishon Vijay Abraham I
0 siblings, 2 replies; 14+ messages in thread
From: Kishon Vijay Abraham I @ 2021-05-07 11:02 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 14+ messages in thread* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-07 11:02 [PATCH 0/2] reset: Fix the behavior of optional reset Kishon Vijay Abraham I @ 2021-05-07 11:02 ` Kishon Vijay Abraham I 2021-05-07 16:34 ` Simon Glass 2021-05-07 11:02 ` [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA Kishon Vijay Abraham I 1 sibling, 1 reply; 14+ messages in thread From: Kishon Vijay Abraham I @ 2021-05-07 11:02 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-07 11:02 ` [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() Kishon Vijay Abraham I @ 2021-05-07 16:34 ` Simon Glass 2021-05-11 6:26 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 14+ messages in thread From: Simon Glass @ 2021-05-07 16:34 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-07 16:34 ` Simon Glass @ 2021-05-11 6:26 ` Kishon Vijay Abraham I 2021-05-11 16:39 ` Simon Glass 0 siblings, 1 reply; 14+ messages in thread From: Kishon Vijay Abraham I @ 2021-05-11 6:26 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-11 6:26 ` Kishon Vijay Abraham I @ 2021-05-11 16:39 ` Simon Glass 2021-05-13 6:15 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 14+ messages in thread From: Simon Glass @ 2021-05-11 16:39 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-11 16:39 ` Simon Glass @ 2021-05-13 6:15 ` Kishon Vijay Abraham I 2021-05-13 23:56 ` Simon Glass 0 siblings, 1 reply; 14+ messages in thread From: Kishon Vijay Abraham I @ 2021-05-13 6:15 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-13 6:15 ` Kishon Vijay Abraham I @ 2021-05-13 23:56 ` Simon Glass 2021-05-14 5:18 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 14+ messages in thread From: Simon Glass @ 2021-05-13 23:56 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-13 23:56 ` Simon Glass @ 2021-05-14 5:18 ` Kishon Vijay Abraham I 2021-06-08 10:40 ` Pratyush Yadav 0 siblings, 1 reply; 14+ messages in thread From: Kishon Vijay Abraham I @ 2021-05-14 5:18 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-05-14 5:18 ` Kishon Vijay Abraham I @ 2021-06-08 10:40 ` Pratyush Yadav 2021-06-19 17:32 ` Simon Glass 0 siblings, 1 reply; 14+ messages in thread From: Pratyush Yadav @ 2021-06-08 10:40 UTC (permalink / raw) To: Kishon Vijay Abraham I Cc: Simon Glass, Lokesh Vutla, Marek Vasut, Aswath Govindraju, U-Boot Mailing List 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() 2021-06-08 10:40 ` Pratyush Yadav @ 2021-06-19 17:32 ` Simon Glass 0 siblings, 0 replies; 14+ messages in thread From: Simon Glass @ 2021-06-19 17:32 UTC (permalink / raw) To: Pratyush Yadav Cc: Kishon Vijay Abraham I, Lokesh Vutla, Marek Vasut, Aswath Govindraju, U-Boot Mailing List 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA 2021-05-07 11:02 [PATCH 0/2] reset: Fix the behavior of optional reset Kishon Vijay Abraham I 2021-05-07 11:02 ` [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() Kishon Vijay Abraham I @ 2021-05-07 11:02 ` Kishon Vijay Abraham I 2021-05-07 16:34 ` Simon Glass 1 sibling, 1 reply; 14+ messages in thread From: Kishon Vijay Abraham I @ 2021-05-07 11:02 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA 2021-05-07 11:02 ` [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA Kishon Vijay Abraham I @ 2021-05-07 16:34 ` Simon Glass 2021-05-13 6:30 ` Kishon Vijay Abraham I 0 siblings, 1 reply; 14+ messages in thread From: Simon Glass @ 2021-05-07 16:34 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA 2021-05-07 16:34 ` Simon Glass @ 2021-05-13 6:30 ` Kishon Vijay Abraham I 2021-05-13 17:00 ` Simon Glass 0 siblings, 1 reply; 14+ messages in thread From: Kishon Vijay Abraham I @ 2021-05-13 6:30 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA 2021-05-13 6:30 ` Kishon Vijay Abraham I @ 2021-05-13 17:00 ` Simon Glass 0 siblings, 0 replies; 14+ messages in thread From: Simon Glass @ 2021-05-13 17:00 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-19 17:32 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-07 11:02 [PATCH 0/2] reset: Fix the behavior of optional reset Kishon Vijay Abraham I 2021-05-07 11:02 ` [PATCH 1/2] reset: Do not return NULL on error for devm_reset_control_get_optional() Kishon Vijay Abraham I 2021-05-07 16:34 ` Simon Glass 2021-05-11 6:26 ` Kishon Vijay Abraham I 2021-05-11 16:39 ` Simon Glass 2021-05-13 6:15 ` Kishon Vijay Abraham I 2021-05-13 23:56 ` Simon Glass 2021-05-14 5:18 ` Kishon Vijay Abraham I 2021-06-08 10:40 ` Pratyush Yadav 2021-06-19 17:32 ` Simon Glass 2021-05-07 11:02 ` [PATCH 2/2] reset: Let reset API's handle gracefully if reset_ctl is -ENODATA Kishon Vijay Abraham I 2021-05-07 16:34 ` Simon Glass 2021-05-13 6:30 ` Kishon Vijay Abraham I 2021-05-13 17:00 ` Simon Glass
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.