| Message ID | 20180217205843.17680-1-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] i2c: designware: Consider SCL GPIO optional | expand |
On Sat, 2018-02-17 at 22:58 +0200, Andy Shevchenko wrote: > GPIO library can return -ENOSYS for the failed request. > Instead of failing ->probe() in this case override error code to 0. > Oops, Cc'ing Dominik. Dominik, I meant this one. > Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") > Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Tim Sander <tim@krieglstein.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-master.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-designware-master.c > b/drivers/i2c/busses/i2c-designware-master.c > index ae691884d071..5778908351a2 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -644,7 +644,7 @@ static int i2c_dw_init_recovery_info(struct > dw_i2c_dev *dev) > gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); > if (IS_ERR(gpio)) { > r = PTR_ERR(gpio); > - if (r == -ENOENT) > + if (r == -ENOENT || r == -ENOSYS) > return 0; > return r; > }
On 20/02/2018 19:01, Andy Shevchenko wrote: > On Sat, 2018-02-17 at 22:58 +0200, Andy Shevchenko wrote: >> GPIO library can return -ENOSYS for the failed request. >> Instead of failing ->probe() in this case override error code to 0. >> > > Oops, Cc'ing Dominik. > Dominik, I meant this one. > >> Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") >> Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> >> Cc: Tim Sander <tim@krieglstein.org> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/i2c/busses/i2c-designware-master.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-master.c >> b/drivers/i2c/busses/i2c-designware-master.c >> index ae691884d071..5778908351a2 100644 >> --- a/drivers/i2c/busses/i2c-designware-master.c >> +++ b/drivers/i2c/busses/i2c-designware-master.c >> @@ -644,7 +644,7 @@ static int i2c_dw_init_recovery_info(struct >> dw_i2c_dev *dev) >> gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); >> if (IS_ERR(gpio)) { >> r = PTR_ERR(gpio); >> - if (r == -ENOENT) >> + if (r == -ENOENT || r == -ENOSYS) >> return 0; >> return r; >> } > Looking at this again, I think I should have used devm_gpiod_get_optional. I think this would do the same thing. get_optional returns null instead of ENOSYS gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH); if (IS_ERR_OR_NULL(gpio)) return PTR_ERR(gpio);
On Tue, Feb 20, 2018 at 01:01:21PM +0200, Andy Shevchenko wrote: > On Sat, 2018-02-17 at 22:58 +0200, Andy Shevchenko wrote: > > GPIO library can return -ENOSYS for the failed request. > > Instead of failing ->probe() in this case override error code to 0. > > > > Oops, Cc'ing Dominik. > Dominik, I meant this one. > > > Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") > > Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> > > Cc: Tim Sander <tim@krieglstein.org> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > drivers/i2c/busses/i2c-designware-master.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/i2c/busses/i2c-designware-master.c > > b/drivers/i2c/busses/i2c-designware-master.c > > index ae691884d071..5778908351a2 100644 > > --- a/drivers/i2c/busses/i2c-designware-master.c > > +++ b/drivers/i2c/busses/i2c-designware-master.c > > @@ -644,7 +644,7 @@ static int i2c_dw_init_recovery_info(struct > > dw_i2c_dev *dev) > > gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); > > if (IS_ERR(gpio)) { > > r = PTR_ERR(gpio); > > - if (r == -ENOENT) > > + if (r == -ENOENT || r == -ENOSYS) > > return 0; > > return r; > > } Yes, this works as well. s/Reported-by/Reported-and-Tested-by/ Thanks, Dominik
On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: > GPIO library can return -ENOSYS for the failed request. > Instead of failing ->probe() in this case override error code to 0. I wonder if the code wouldn't be much easier if i2c_dw_init_recovery_info() was simply returning void? Or just give a warning in the log but not bail out of probe()? > > Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") > Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Tim Sander <tim@krieglstein.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-master.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c > index ae691884d071..5778908351a2 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -644,7 +644,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) > gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); > if (IS_ERR(gpio)) { > r = PTR_ERR(gpio); > - if (r == -ENOENT) > + if (r == -ENOENT || r == -ENOSYS) > return 0; > return r; > } > -- > 2.15.1 >
On 21/02/2018 16:31, Wolfram Sang wrote: > On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: >> GPIO library can return -ENOSYS for the failed request. >> Instead of failing ->probe() in this case override error code to 0. > > I wonder if the code wouldn't be much easier if > i2c_dw_init_recovery_info() was simply returning void? Or just give a > warning in the log but not bail out of probe()? You need to handle EPROBE_DEFER. > >> >> Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") >> Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> >> Cc: Tim Sander <tim@krieglstein.org> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/i2c/busses/i2c-designware-master.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c >> index ae691884d071..5778908351a2 100644 >> --- a/drivers/i2c/busses/i2c-designware-master.c >> +++ b/drivers/i2c/busses/i2c-designware-master.c >> @@ -644,7 +644,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) >> gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); >> if (IS_ERR(gpio)) { >> r = PTR_ERR(gpio); >> - if (r == -ENOENT) >> + if (r == -ENOENT || r == -ENOSYS) >> return 0; >> return r; >> } >> -- >> 2.15.1 >>
On Wed, Feb 21, 2018 at 04:38:10PM +0800, Phil Reid wrote: > On 21/02/2018 16:31, Wolfram Sang wrote: > > On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: > > > GPIO library can return -ENOSYS for the failed request. > > > Instead of failing ->probe() in this case override error code to 0. > > > > I wonder if the code wouldn't be much easier if > > i2c_dw_init_recovery_info() was simply returning void? Or just give a > > warning in the log but not bail out of probe()? > > You need to handle EPROBE_DEFER. Right. Then we should probably encode that explicitly?
On 21/02/2018 16:41, Wolfram Sang wrote: > On Wed, Feb 21, 2018 at 04:38:10PM +0800, Phil Reid wrote: >> On 21/02/2018 16:31, Wolfram Sang wrote: >>> On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: >>>> GPIO library can return -ENOSYS for the failed request. >>>> Instead of failing ->probe() in this case override error code to 0. >>> >>> I wonder if the code wouldn't be much easier if >>> i2c_dw_init_recovery_info() was simply returning void? Or just give a >>> warning in the log but not bail out of probe()? >> >> You need to handle EPROBE_DEFER. > > Right. Then we should probably encode that explicitly? > I think this will handle all cases correctly. gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH); if (IS_ERR_OR_NULL(gpio)) return PTR_ERR_OR_ZERO(gpio); My logic at the time was that scl is not optional for the i2c recovery. But it's optional form the driver point of view. The core returns NULL for that call if GPIO is not selected. And NULL if ENOENT was returned by the inner gpiod_get when GPIO is selected. Let me know if you want a patch.
On Wed, Feb 21, 2018 at 04:56:36PM +0800, Phil Reid wrote: > On 21/02/2018 16:41, Wolfram Sang wrote: > > On Wed, Feb 21, 2018 at 04:38:10PM +0800, Phil Reid wrote: > > > On 21/02/2018 16:31, Wolfram Sang wrote: > > > > On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: > > > > > GPIO library can return -ENOSYS for the failed request. > > > > > Instead of failing ->probe() in this case override error code to 0. > > > > > > > > I wonder if the code wouldn't be much easier if > > > > i2c_dw_init_recovery_info() was simply returning void? Or just give a > > > > warning in the log but not bail out of probe()? > > > > > > You need to handle EPROBE_DEFER. > > > > Right. Then we should probably encode that explicitly? > > > > I think this will handle all cases correctly. > > gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH); > if (IS_ERR_OR_NULL(gpio)) > return PTR_ERR_OR_ZERO(gpio); > > My logic at the time was that scl is not optional for the i2c recovery. > But it's optional form the driver point of view. > > The core returns NULL for that call if GPIO is not selected. > And NULL if ENOENT was returned by the inner gpiod_get when GPIO is selected. > > Let me know if you want a patch. My preference would still be to encode that only for -EPROBE_DEFER we bail out of probe and for every other error, we simply don't do bus recovery. I think it's way more understandable to say what error we are interested in instead of masking all those out we are not interested in. Or?
On 21/02/2018 17:02, Wolfram Sang wrote: > On Wed, Feb 21, 2018 at 04:56:36PM +0800, Phil Reid wrote: >> On 21/02/2018 16:41, Wolfram Sang wrote: >>> On Wed, Feb 21, 2018 at 04:38:10PM +0800, Phil Reid wrote: >>>> On 21/02/2018 16:31, Wolfram Sang wrote: >>>>> On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: >>>>>> GPIO library can return -ENOSYS for the failed request. >>>>>> Instead of failing ->probe() in this case override error code to 0. >>>>> >>>>> I wonder if the code wouldn't be much easier if >>>>> i2c_dw_init_recovery_info() was simply returning void? Or just give a >>>>> warning in the log but not bail out of probe()? >>>> >>>> You need to handle EPROBE_DEFER. >>> >>> Right. Then we should probably encode that explicitly? >>> >> >> I think this will handle all cases correctly. >> >> gpio = devm_gpiod_get_optional(dev->dev, "scl", GPIOD_OUT_HIGH); >> if (IS_ERR_OR_NULL(gpio)) >> return PTR_ERR_OR_ZERO(gpio); >> >> My logic at the time was that scl is not optional for the i2c recovery. >> But it's optional form the driver point of view. >> >> The core returns NULL for that call if GPIO is not selected. >> And NULL if ENOENT was returned by the inner gpiod_get when GPIO is selected. >> >> Let me know if you want a patch. > > My preference would still be to encode that only for -EPROBE_DEFER we > bail out of probe and for every other error, we simply don't do bus > recovery. I think it's way more understandable to say what error we are > interested in instead of masking all those out we are not interested in. > > Or? > I would think all the other errors are probably something that needs to be investigated. eg: incorrect GPIO flags on the dt binding etc.
On Wed, 2018-02-21 at 19:52 +0800, Phil Reid wrote: > On 21/02/2018 17:02, Wolfram Sang wrote: > > On Wed, Feb 21, 2018 at 04:56:36PM +0800, Phil Reid wrote: > > > On 21/02/2018 16:41, Wolfram Sang wrote: > > > > On Wed, Feb 21, 2018 at 04:38:10PM +0800, Phil Reid wrote: > > > > > > I think this will handle all cases correctly. > > > > > > gpio = devm_gpiod_get_optional(dev->dev, "scl", > > > GPIOD_OUT_HIGH); > > > if (IS_ERR_OR_NULL(gpio)) > > > return PTR_ERR_OR_ZERO(gpio); > > > > > > My logic at the time was that scl is not optional for the i2c > > > recovery. > > > But it's optional form the driver point of view. > > > > > > The core returns NULL for that call if GPIO is not selected. > > > And NULL if ENOENT was returned by the inner gpiod_get when GPIO > > > is selected. > > > > > > Let me know if you want a patch. > > > > My preference would still be to encode that only for -EPROBE_DEFER > > we > > bail out of probe and for every other error, we simply don't do bus > > recovery. I think it's way more understandable to say what error we > > are > > interested in instead of masking all those out we are not interested > > in. > > > > Or? > > > > I would think all the other errors are probably something that needs > to be investigated. > eg: incorrect GPIO flags on the dt binding etc. I agree with Phil, OTOH there are both patterns in the kernel to check by IS_ERR_OR_NULL vs. -ENOSYS and -ENOENT. So, I have no strong opinion which one to use.
> > I would think all the other errors are probably something that needs > > to be investigated. > > eg: incorrect GPIO flags on the dt binding etc. > > I agree with Phil, OTOH there are both patterns in the kernel to check > by IS_ERR_OR_NULL vs. -ENOSYS and -ENOENT. > > So, I have no strong opinion which one to use. Okay, if you both agree that it is better to reject the driver binding in such a case instead of printing a warning and continuing without recovery support, then be it and I'll take this patch. My milage varies, though, but well, nothing major.
On Sat, Feb 17, 2018 at 10:58:43PM +0200, Andy Shevchenko wrote: > GPIO library can return -ENOSYS for the failed request. > Instead of failing ->probe() in this case override error code to 0. > > Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") > Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> > Cc: Tim Sander <tim@krieglstein.org> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Applied to for-current, thanks!
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index ae691884d071..5778908351a2 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -644,7 +644,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev) gpio = devm_gpiod_get(dev->dev, "scl", GPIOD_OUT_HIGH); if (IS_ERR(gpio)) { r = PTR_ERR(gpio); - if (r == -ENOENT) + if (r == -ENOENT || r == -ENOSYS) return 0; return r; }
GPIO library can return -ENOSYS for the failed request. Instead of failing ->probe() in this case override error code to 0. Fixes: ca382f5b38f3 ("i2c: designware: add i2c gpio recovery option") Reported-by: Dominik Brodowski <linux@dominikbrodowski.net> Cc: Tim Sander <tim@krieglstein.org> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/i2c/busses/i2c-designware-master.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)