* [PATCH] reset: Put back *_optional variants @ 2016-05-26 1:15 John Youn 2016-05-26 20:25 ` Hans de Goede 0 siblings, 1 reply; 8+ messages in thread From: John Youn @ 2016-05-26 1:15 UTC (permalink / raw) To: linux-arm-kernel Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions wrappers"), the optional variants returned -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set. This patch reverts to this behavior. Otherwise those calls will return -EINVAL causing users to think that an error occurred when CONFIG_RESET_CONTROLLER is not set. Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions wrappers") Signed-off-by: John Youn <johnyoun@synopsys.com> --- Hi Philipp, Hans, The commit referenced above breaks an upcoming patch for the dwc2 driver that adds an optional reset control. https://marc.info/?l=linux-usb&m=146161328211584&w=2 I've attempted to add the optional variants back the way they were working before. Let me know if I need to do anything else to fix it or if it should be done another way. Regards, John include/linux/reset.h | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/include/linux/reset.h b/include/linux/reset.h index ec0306ce..739d33d 100644 --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -14,10 +14,24 @@ int reset_control_status(struct reset_control *rstc); struct reset_control *__of_reset_control_get(struct device_node *node, const char *id, int index, int shared); + +struct reset_control *__of_reset_control_get_optional(struct device_node *node, + const char *id, int index, int shared) +{ + return __of_reset_control_get(node, id, index, shared); +} + void reset_control_put(struct reset_control *rstc); struct reset_control *__devm_reset_control_get(struct device *dev, const char *id, int index, int shared); +static inline struct reset_control *__devm_reset_control_get_optional( + struct device *dev, + const char *id, int index, int shared) +{ + return __devm_reset_control_get(dev, id, index, shared); +} + int __must_check device_reset(struct device *dev); static inline int device_reset_optional(struct device *dev) @@ -74,6 +88,13 @@ static inline struct reset_control *__of_reset_control_get( return ERR_PTR(-EINVAL); } +static inline struct reset_control *__of_reset_control_get_optional( + struct device_node *node, + const char *id, int index, int shared) +{ + return ERR_PTR(-ENOTSUPP); +} + static inline struct reset_control *__devm_reset_control_get( struct device *dev, const char *id, int index, int shared) @@ -81,6 +102,13 @@ static inline struct reset_control *__devm_reset_control_get( return ERR_PTR(-EINVAL); } +static inline struct reset_control *__devm_reset_control_get_optional( + struct device *dev, + const char *id, int index, int shared) +{ + return ERR_PTR(-ENOTSUPP); +} + #endif /* CONFIG_RESET_CONTROLLER */ /** @@ -110,7 +138,8 @@ static inline struct reset_control *__must_check reset_control_get( static inline struct reset_control *reset_control_get_optional( struct device *dev, const char *id) { - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); + return __of_reset_control_get_optional(dev ? dev->of_node : NULL, + id, 0, 0); } /** @@ -194,7 +223,7 @@ static inline struct reset_control *__must_check devm_reset_control_get( static inline struct reset_control *devm_reset_control_get_optional( struct device *dev, const char *id) { - return __devm_reset_control_get(dev, id, 0, 0); + return __devm_reset_control_get_optional(dev, id, 0, 0); } /** -- 2.8.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-26 1:15 [PATCH] reset: Put back *_optional variants John Youn @ 2016-05-26 20:25 ` Hans de Goede 2016-05-26 21:44 ` John Youn 0 siblings, 1 reply; 8+ messages in thread From: Hans de Goede @ 2016-05-26 20:25 UTC (permalink / raw) To: linux-arm-kernel Hi, On 26-05-16 03:15, John Youn wrote: > Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] > functions wrappers"), the optional variants returned -ENOTSUPP when > CONFIG_RESET_CONTROLLER was not set. This patch reverts to this > behavior. Otherwise those calls will return -EINVAL causing users to > think that an error occurred when CONFIG_RESET_CONTROLLER is not set. > > Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions wrappers") > Signed-off-by: John Youn <johnyoun@synopsys.com> > --- > > Hi Philipp, Hans, > > The commit referenced above breaks an upcoming patch for the dwc2 > driver that adds an optional reset control. > > https://marc.info/?l=linux-usb&m=146161328211584&w=2 > > I've attempted to add the optional variants back the way they were > working before. Let me know if I need to do anything else to fix it or > if it should be done another way. > > Regards, > John Hmm, I don't like all the extra code your patch adds just to fix a return value... Looking at the code before my "reset: Make [of_]reset_control_get[_foo] functions wrappers" patch, all the dev*_get* functions were returning ENOTSUPP except for [devm_]reset_control_get, so following your logic we should also change the of_reset_control_get_by_index variant to return -ENOTSUP. Or better, simply make them all return -ENOTSUP, that seems both consistent and more KISS to me, this would mean an error code change for [devm_]reset_control_get, but will fix all the other getters from having a changed error-code, and I would callers of [devm_]reset_control_get to not care which error code they get, except for -EPROBE_DEFER. So IMHO the following change would be a better way to fix this: --- a/include/linux/reset.h +++ b/include/linux/reset.h @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( struct device_node *node, const char *id, int index, int shared) { - return ERR_PTR(-EINVAL); + return ERR_PTR(-ENOTSUPP); } static inline struct reset_control *__devm_reset_control_get( struct device *dev, const char *id, int index, int shared) { - return ERR_PTR(-EINVAL); + return ERR_PTR(-ENOTSUPP); } #endif /* CONFIG_RESET_CONTROLLER */ Regards, Hans > > include/linux/reset.h | 33 +++++++++++++++++++++++++++++++-- > 1 file changed, 31 insertions(+), 2 deletions(-) > > diff --git a/include/linux/reset.h b/include/linux/reset.h > index ec0306ce..739d33d 100644 > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -14,10 +14,24 @@ int reset_control_status(struct reset_control *rstc); > > struct reset_control *__of_reset_control_get(struct device_node *node, > const char *id, int index, int shared); > + > +struct reset_control *__of_reset_control_get_optional(struct device_node *node, > + const char *id, int index, int shared) > +{ > + return __of_reset_control_get(node, id, index, shared); > +} > + > void reset_control_put(struct reset_control *rstc); > struct reset_control *__devm_reset_control_get(struct device *dev, > const char *id, int index, int shared); > > +static inline struct reset_control *__devm_reset_control_get_optional( > + struct device *dev, > + const char *id, int index, int shared) > +{ > + return __devm_reset_control_get(dev, id, index, shared); > +} > + > int __must_check device_reset(struct device *dev); > > static inline int device_reset_optional(struct device *dev) > @@ -74,6 +88,13 @@ static inline struct reset_control *__of_reset_control_get( > return ERR_PTR(-EINVAL); > } > > +static inline struct reset_control *__of_reset_control_get_optional( > + struct device_node *node, > + const char *id, int index, int shared) > +{ > + return ERR_PTR(-ENOTSUPP); > +} > + > static inline struct reset_control *__devm_reset_control_get( > struct device *dev, > const char *id, int index, int shared) > @@ -81,6 +102,13 @@ static inline struct reset_control *__devm_reset_control_get( > return ERR_PTR(-EINVAL); > } > > +static inline struct reset_control *__devm_reset_control_get_optional( > + struct device *dev, > + const char *id, int index, int shared) > +{ > + return ERR_PTR(-ENOTSUPP); > +} > + > #endif /* CONFIG_RESET_CONTROLLER */ > > /** > @@ -110,7 +138,8 @@ static inline struct reset_control *__must_check reset_control_get( > static inline struct reset_control *reset_control_get_optional( > struct device *dev, const char *id) > { > - return __of_reset_control_get(dev ? dev->of_node : NULL, id, 0, 0); > + return __of_reset_control_get_optional(dev ? dev->of_node : NULL, > + id, 0, 0); > } > > /** > @@ -194,7 +223,7 @@ static inline struct reset_control *__must_check devm_reset_control_get( > static inline struct reset_control *devm_reset_control_get_optional( > struct device *dev, const char *id) > { > - return __devm_reset_control_get(dev, id, 0, 0); > + return __devm_reset_control_get_optional(dev, id, 0, 0); > } > > /** > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-26 20:25 ` Hans de Goede @ 2016-05-26 21:44 ` John Youn 2016-05-27 7:06 ` Hans de Goede 0 siblings, 1 reply; 8+ messages in thread From: John Youn @ 2016-05-26 21:44 UTC (permalink / raw) To: linux-arm-kernel On 5/26/2016 1:25 PM, Hans de Goede wrote: > Hi, > > On 26-05-16 03:15, John Youn wrote: >> Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] >> functions wrappers"), the optional variants returned -ENOTSUPP when >> CONFIG_RESET_CONTROLLER was not set. This patch reverts to this >> behavior. Otherwise those calls will return -EINVAL causing users to >> think that an error occurred when CONFIG_RESET_CONTROLLER is not set. >> >> Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions wrappers") >> Signed-off-by: John Youn <johnyoun@synopsys.com> >> --- >> >> Hi Philipp, Hans, >> >> The commit referenced above breaks an upcoming patch for the dwc2 >> driver that adds an optional reset control. >> >> https://marc.info/?l=linux-usb&m=146161328211584&w=2 >> >> I've attempted to add the optional variants back the way they were >> working before. Let me know if I need to do anything else to fix it or >> if it should be done another way. >> >> Regards, >> John > > Hmm, I don't like all the extra code your patch adds just to fix > a return value... > > Looking at the code before my "reset: Make [of_]reset_control_get[_foo] > functions wrappers" patch, all the dev*_get* functions were > returning ENOTSUPP except for [devm_]reset_control_get, so following > your logic we should also change the of_reset_control_get_by_index > variant to return -ENOTSUP. > > Or better, simply make them all return -ENOTSUP, that seems both > consistent and more KISS to me, this would mean an error code > change for [devm_]reset_control_get, but will fix all the other > getters from having a changed error-code, and I would callers > of [devm_]reset_control_get to not care which error code they > get, except for -EPROBE_DEFER. > > So IMHO the following change would be a better way to fix this: > > --- a/include/linux/reset.h > +++ b/include/linux/reset.h > @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( > struct device_node *node, > const char *id, int index, int shared) > { > - return ERR_PTR(-EINVAL); > + return ERR_PTR(-ENOTSUPP); > } > > static inline struct reset_control *__devm_reset_control_get( > struct device *dev, > const char *id, int index, int shared) > { > - return ERR_PTR(-EINVAL); > + return ERR_PTR(-ENOTSUPP); > } > > #endif /* CONFIG_RESET_CONTROLLER */ I'm good with this. However, per Philipp on a previous thread, the intended behavior is to return -EINVAL for the non-optional functions. http://marc.info/?l=linux-usb&m=146156945528848&w=2 Philipp, Any suggestions? Regards, John ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-26 21:44 ` John Youn @ 2016-05-27 7:06 ` Hans de Goede 2016-05-30 10:18 ` Philipp Zabel 0 siblings, 1 reply; 8+ messages in thread From: Hans de Goede @ 2016-05-27 7:06 UTC (permalink / raw) To: linux-arm-kernel Hi, On 26-05-16 23:44, John Youn wrote: > On 5/26/2016 1:25 PM, Hans de Goede wrote: >> Hi, >> >> On 26-05-16 03:15, John Youn wrote: >>> Prior to commit 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] >>> functions wrappers"), the optional variants returned -ENOTSUPP when >>> CONFIG_RESET_CONTROLLER was not set. This patch reverts to this >>> behavior. Otherwise those calls will return -EINVAL causing users to >>> think that an error occurred when CONFIG_RESET_CONTROLLER is not set. >>> >>> Fixes: 6c96f05c8bb8 ("reset: Make [of_]reset_control_get[_foo] functions wrappers") >>> Signed-off-by: John Youn <johnyoun@synopsys.com> >>> --- >>> >>> Hi Philipp, Hans, >>> >>> The commit referenced above breaks an upcoming patch for the dwc2 >>> driver that adds an optional reset control. >>> >>> https://marc.info/?l=linux-usb&m=146161328211584&w=2 >>> >>> I've attempted to add the optional variants back the way they were >>> working before. Let me know if I need to do anything else to fix it or >>> if it should be done another way. >>> >>> Regards, >>> John >> >> Hmm, I don't like all the extra code your patch adds just to fix >> a return value... >> >> Looking at the code before my "reset: Make [of_]reset_control_get[_foo] >> functions wrappers" patch, all the dev*_get* functions were >> returning ENOTSUPP except for [devm_]reset_control_get, so following >> your logic we should also change the of_reset_control_get_by_index >> variant to return -ENOTSUP. >> >> Or better, simply make them all return -ENOTSUP, that seems both >> consistent and more KISS to me, this would mean an error code >> change for [devm_]reset_control_get, but will fix all the other >> getters from having a changed error-code, and I would callers >> of [devm_]reset_control_get to not care which error code they >> get, except for -EPROBE_DEFER. >> >> So IMHO the following change would be a better way to fix this: >> >> --- a/include/linux/reset.h >> +++ b/include/linux/reset.h >> @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( >> struct device_node *node, >> const char *id, int index, int shared) >> { >> - return ERR_PTR(-EINVAL); >> + return ERR_PTR(-ENOTSUPP); >> } >> >> static inline struct reset_control *__devm_reset_control_get( >> struct device *dev, >> const char *id, int index, int shared) >> { >> - return ERR_PTR(-EINVAL); >> + return ERR_PTR(-ENOTSUPP); >> } >> >> #endif /* CONFIG_RESET_CONTROLLER */ > > > I'm good with this. However, per Philipp on a previous thread, the > intended behavior is to return -EINVAL for the non-optional functions. > > http://marc.info/?l=linux-usb&m=146156945528848&w=2 As Philipp rightfully points out, calling the non-optional functions without CONFIG_RESET_CONTROLLER is a no-no, so IMHO we should not have to care too much about the error code in that case, as long as we return an error. Also note that even before my patch things were already inconsistent with the of_...get... functions already always returning -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set, to me it seems best and also KISS to just return -ENOTSUPP from all get functions when CONFIG_RESET_CONTROLLER is not set. Anyways this is Philipp's call, this is all just my 2 cents. Regards, Hans ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-27 7:06 ` Hans de Goede @ 2016-05-30 10:18 ` Philipp Zabel 2016-05-30 11:32 ` Hans de Goede 0 siblings, 1 reply; 8+ messages in thread From: Philipp Zabel @ 2016-05-30 10:18 UTC (permalink / raw) To: linux-arm-kernel Hi, Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede: [...] > >> So IMHO the following change would be a better way to fix this: > >> > >> --- a/include/linux/reset.h > >> +++ b/include/linux/reset.h > >> @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( > >> struct device_node *node, > >> const char *id, int index, int shared) > >> { > >> - return ERR_PTR(-EINVAL); > >> + return ERR_PTR(-ENOTSUPP); > >> } > >> > >> static inline struct reset_control *__devm_reset_control_get( > >> struct device *dev, > >> const char *id, int index, int shared) > >> { > >> - return ERR_PTR(-EINVAL); > >> + return ERR_PTR(-ENOTSUPP); > >> } > >> > >> #endif /* CONFIG_RESET_CONTROLLER */ > > > > > > I'm good with this. However, per Philipp on a previous thread, the > > intended behavior is to return -EINVAL for the non-optional functions. > > > > http://marc.info/?l=linux-usb&m=146156945528848&w=2 Adding Dinh to Cc: because he wanted this changed from -EINVAL. My point then was that WARN_ON + -EINVAL is indented in this case. Given that the warning backtrace already helps to identify the issue (CONFIG_RESET_CONTROLLER disabled), and that changing the return value to -ENOTSUPP improves consistency with the rest of the API and reduces the amount of inline wrappers, I concur. > As Philipp rightfully points out, calling the non-optional functions > without CONFIG_RESET_CONTROLLER is a no-no, so IMHO we should not have to > care too much about the error code in that case, as long as we return > an error. > > Also note that even before my patch things were already inconsistent > with the of_...get... functions already always returning > -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set, to me it seems > best and also KISS to just return -ENOTSUPP from all get functions > when CONFIG_RESET_CONTROLLER is not set. > > Anyways this is Philipp's call, this is all just my 2 cents. Let the stubs return -ENOTSUPP consistently. A quick grep doesn't show any driver handling -EINVAL explicitly either. regards Philipp ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-30 10:18 ` Philipp Zabel @ 2016-05-30 11:32 ` Hans de Goede 2016-05-30 12:11 ` Philipp Zabel 2016-05-31 5:53 ` John Youn 0 siblings, 2 replies; 8+ messages in thread From: Hans de Goede @ 2016-05-30 11:32 UTC (permalink / raw) To: linux-arm-kernel Hi, On 30-05-16 12:18, Philipp Zabel wrote: > Hi, > > Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede: > [...] >>>> So IMHO the following change would be a better way to fix this: >>>> >>>> --- a/include/linux/reset.h >>>> +++ b/include/linux/reset.h >>>> @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( >>>> struct device_node *node, >>>> const char *id, int index, int shared) >>>> { >>>> - return ERR_PTR(-EINVAL); >>>> + return ERR_PTR(-ENOTSUPP); >>>> } >>>> >>>> static inline struct reset_control *__devm_reset_control_get( >>>> struct device *dev, >>>> const char *id, int index, int shared) >>>> { >>>> - return ERR_PTR(-EINVAL); >>>> + return ERR_PTR(-ENOTSUPP); >>>> } >>>> >>>> #endif /* CONFIG_RESET_CONTROLLER */ >>> >>> >>> I'm good with this. However, per Philipp on a previous thread, the >>> intended behavior is to return -EINVAL for the non-optional functions. >>> >>> http://marc.info/?l=linux-usb&m=146156945528848&w=2 > > Adding Dinh to Cc: because he wanted this changed from -EINVAL. > My point then was that WARN_ON + -EINVAL is indented in this case. > > Given that the warning backtrace already helps to identify the issue > (CONFIG_RESET_CONTROLLER disabled), and that changing the return value > to -ENOTSUPP improves consistency with the rest of the API and reduces > the amount of inline wrappers, I concur. > >> As Philipp rightfully points out, calling the non-optional functions >> without CONFIG_RESET_CONTROLLER is a no-no, so IMHO we should not have to >> care too much about the error code in that case, as long as we return >> an error. >> >> Also note that even before my patch things were already inconsistent >> with the of_...get... functions already always returning >> -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set, to me it seems >> best and also KISS to just return -ENOTSUPP from all get functions >> when CONFIG_RESET_CONTROLLER is not set. >> >> Anyways this is Philipp's call, this is all just my 2 cents. > > Let the stubs return -ENOTSUPP consistently. A quick grep doesn't show > any driver handling -EINVAL explicitly either. Great. John Youn, can you submit a patch changing the return of the stubs from -EINVAL to -ENOTSUPP please ? Thanks & Regards, Hans ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-30 11:32 ` Hans de Goede @ 2016-05-30 12:11 ` Philipp Zabel 2016-05-31 5:53 ` John Youn 1 sibling, 0 replies; 8+ messages in thread From: Philipp Zabel @ 2016-05-30 12:11 UTC (permalink / raw) To: linux-arm-kernel Am Montag, den 30.05.2016, 13:32 +0200 schrieb Hans de Goede: > Hi, > > On 30-05-16 12:18, Philipp Zabel wrote: > > Hi, > > > > Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede: > > [...] > >>>> So IMHO the following change would be a better way to fix this: > >>>> > >>>> --- a/include/linux/reset.h > >>>> +++ b/include/linux/reset.h > >>>> @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( > >>>> struct device_node *node, > >>>> const char *id, int index, int shared) > >>>> { > >>>> - return ERR_PTR(-EINVAL); > >>>> + return ERR_PTR(-ENOTSUPP); > >>>> } > >>>> > >>>> static inline struct reset_control *__devm_reset_control_get( > >>>> struct device *dev, > >>>> const char *id, int index, int shared) > >>>> { > >>>> - return ERR_PTR(-EINVAL); > >>>> + return ERR_PTR(-ENOTSUPP); > >>>> } > >>>> > >>>> #endif /* CONFIG_RESET_CONTROLLER */ > >>> > >>> > >>> I'm good with this. However, per Philipp on a previous thread, the > >>> intended behavior is to return -EINVAL for the non-optional functions. > >>> > >>> http://marc.info/?l=linux-usb&m=146156945528848&w=2 > > > > Adding Dinh to Cc: because he wanted this changed from -EINVAL. > > My point then was that WARN_ON + -EINVAL is indented in this case. No idea how this happened, but I meant "intended", obviously :) regards Philipp ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] reset: Put back *_optional variants 2016-05-30 11:32 ` Hans de Goede 2016-05-30 12:11 ` Philipp Zabel @ 2016-05-31 5:53 ` John Youn 1 sibling, 0 replies; 8+ messages in thread From: John Youn @ 2016-05-31 5:53 UTC (permalink / raw) To: linux-arm-kernel On 5/30/2016 4:32 AM, Hans de Goede wrote: > Hi, > > On 30-05-16 12:18, Philipp Zabel wrote: >> Hi, >> >> Am Freitag, den 27.05.2016, 09:06 +0200 schrieb Hans de Goede: >> [...] >>>>> So IMHO the following change would be a better way to fix this: >>>>> >>>>> --- a/include/linux/reset.h >>>>> +++ b/include/linux/reset.h >>>>> @@ -65,14 +65,14 @@ static inline struct reset_control *__of_reset_control_get( >>>>> struct device_node *node, >>>>> const char *id, int index, int shared) >>>>> { >>>>> - return ERR_PTR(-EINVAL); >>>>> + return ERR_PTR(-ENOTSUPP); >>>>> } >>>>> >>>>> static inline struct reset_control *__devm_reset_control_get( >>>>> struct device *dev, >>>>> const char *id, int index, int shared) >>>>> { >>>>> - return ERR_PTR(-EINVAL); >>>>> + return ERR_PTR(-ENOTSUPP); >>>>> } >>>>> >>>>> #endif /* CONFIG_RESET_CONTROLLER */ >>>> >>>> >>>> I'm good with this. However, per Philipp on a previous thread, the >>>> intended behavior is to return -EINVAL for the non-optional functions. >>>> >>>> http://marc.info/?l=linux-usb&m=146156945528848&w=2 >> >> Adding Dinh to Cc: because he wanted this changed from -EINVAL. >> My point then was that WARN_ON + -EINVAL is indented in this case. >> >> Given that the warning backtrace already helps to identify the issue >> (CONFIG_RESET_CONTROLLER disabled), and that changing the return value >> to -ENOTSUPP improves consistency with the rest of the API and reduces >> the amount of inline wrappers, I concur. >> >>> As Philipp rightfully points out, calling the non-optional functions >>> without CONFIG_RESET_CONTROLLER is a no-no, so IMHO we should not have to >>> care too much about the error code in that case, as long as we return >>> an error. >>> >>> Also note that even before my patch things were already inconsistent >>> with the of_...get... functions already always returning >>> -ENOTSUPP when CONFIG_RESET_CONTROLLER was not set, to me it seems >>> best and also KISS to just return -ENOTSUPP from all get functions >>> when CONFIG_RESET_CONTROLLER is not set. >>> >>> Anyways this is Philipp's call, this is all just my 2 cents. >> >> Let the stubs return -ENOTSUPP consistently. A quick grep doesn't show >> any driver handling -EINVAL explicitly either. > > Great. > > John Youn, can you submit a patch changing the return of the stubs > from -EINVAL to -ENOTSUPP please ? > Sure. I'll send a patch tomorrow. Regards, John ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-05-31 5:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-26 1:15 [PATCH] reset: Put back *_optional variants John Youn 2016-05-26 20:25 ` Hans de Goede 2016-05-26 21:44 ` John Youn 2016-05-27 7:06 ` Hans de Goede 2016-05-30 10:18 ` Philipp Zabel 2016-05-30 11:32 ` Hans de Goede 2016-05-30 12:11 ` Philipp Zabel 2016-05-31 5:53 ` John Youn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).