| Message ID | 20220413144124.72537-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,1/3] iio: imu: adis16480: Make use of device properties | expand |
> From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Sent: Wednesday, April 13, 2022 4:41 PM > To: Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko > <andriy.shevchenko@linux.intel.com>; linux-iio@vger.kernel.org; > linux-kernel@vger.kernel.org > Cc: Lars-Peter Clausen <lars@metafoo.de>; Hennerich, Michael > <Michael.Hennerich@analog.com>; Jonathan Cameron > <jic23@kernel.org> > Subject: [PATCH v1 3/3] iio: imu: adis16480: Fix getting the optional > clocks > > [External] > > The extended clocks are optional and may not be present for some > SoCs > supported by this driver. Nevertheless, in case the clock is provided > but some error happens during its getting, that error should be > handled > properly. Use devm_clk_get_optional() API for that. Also report > possible > errors using dev_err_probe() to handle properly -EPROBE_DEFER > error. > > Signed-off-by: Andy Shevchenko > <andriy.shevchenko@linux.intel.com> > --- This is a nice cleanup patch... But the subject might be a bit misleading as it says "Fix". So I would expect a Fixes tag which I'm not sure it's really worth it here. Yes, the code was pretty much doing clk_get_optional() "by hand" but I think it was still functional. So to me, this is more an improvement rather than a fix... Anyways, Reviewed-by: Nuno Sá <nuno.sa@analog.com>
On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote: > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Sent: Wednesday, April 13, 2022 4:41 PM > > The extended clocks are optional and may not be present for some > > SoCs > > supported by this driver. Nevertheless, in case the clock is provided > > but some error happens during its getting, that error should be > > handled > > properly. Use devm_clk_get_optional() API for that. Also report > > possible > > errors using dev_err_probe() to handle properly -EPROBE_DEFER > > error. > This is a nice cleanup patch... But the subject might be a bit > misleading as it says "Fix". So I would expect a Fixes tag which > I'm not sure it's really worth it here. Yes, the code was pretty much > doing clk_get_optional() "by hand" but I think it was still functional. > So to me, this is more an improvement rather than a fix... Actually it is a fix, but not critical since no-one complains aloud so far. The problematic part is logs exhausting if repetitive deferred probe happens. > Anyways, > > Reviewed-by: Nuno Sá <nuno.sa@analog.com> Thanks!
On Wed, 2022-04-13 at 19:58 +0300, Andy Shevchenko wrote: > On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote: > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > Sent: Wednesday, April 13, 2022 4:41 PM > > > > The extended clocks are optional and may not be present for some > > > SoCs > > > supported by this driver. Nevertheless, in case the clock is > > > provided > > > but some error happens during its getting, that error should be > > > handled > > > properly. Use devm_clk_get_optional() API for that. Also report > > > possible > > > errors using dev_err_probe() to handle properly -EPROBE_DEFER > > > error. > > > This is a nice cleanup patch... But the subject might be a bit > > misleading as it says "Fix". So I would expect a Fixes tag which > > I'm not sure it's really worth it here. Yes, the code was pretty > > much > > doing clk_get_optional() "by hand" but I think it was still > > functional. > > So to me, this is more an improvement rather than a fix... > > Actually it is a fix, but not critical since no-one complains aloud > so far. > The problematic part is logs exhausting if repetitive deferred probe > happens. > Still not really agree with it... In the commit message you state that errors are not properly handled and so let's use 'devm_clk_get_optional()'. I don't think that is true because If im not missing nothing there's no fundamental change between the previous code and using 'devm_clk_get_optional()'. So to me this is an enhancement because we were doing something "by hand" when we have an API for it. That said, introducing dev_err_probe() indeed stops possibly annoying error messages for EPROBE_DEFER (and that could be seen as a fix, not really devm_clk_get_optional()). I honestly still don't see it as fix but we are also not adding a Fixes tag so I don't really care :). (But I still think the commit message is a bit misleading) - Nuno Sá > >
On Thu, Apr 14, 2022 at 09:07:29AM +0200, Nuno Sá wrote: > On Wed, 2022-04-13 at 19:58 +0300, Andy Shevchenko wrote: > > On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote: > > > > From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > Sent: Wednesday, April 13, 2022 4:41 PM > > > > > > The extended clocks are optional and may not be present for some > > > > SoCs > > > > supported by this driver. Nevertheless, in case the clock is > > > > provided > > > > but some error happens during its getting, that error should be > > > > handled > > > > properly. Use devm_clk_get_optional() API for that. Also report > > > > possible > > > > errors using dev_err_probe() to handle properly -EPROBE_DEFER > > > > error. > > > > > This is a nice cleanup patch... But the subject might be a bit > > > misleading as it says "Fix". So I would expect a Fixes tag which > > > I'm not sure it's really worth it here. Yes, the code was pretty > > > much > > > doing clk_get_optional() "by hand" but I think it was still > > > functional. > > > So to me, this is more an improvement rather than a fix... > > > > Actually it is a fix, but not critical since no-one complains aloud > > so far. > > The problematic part is logs exhausting if repetitive deferred probe > > happens. > > Still not really agree with it... In the commit message you state that > errors are not properly handled and so let's use > 'devm_clk_get_optional()'. I don't think that is true because If im not > missing nothing there's no fundamental change between the previous code > and using 'devm_clk_get_optional()'. So to me this is an enhancement > because we were doing something "by hand" when we have an API for it. > > That said, introducing dev_err_probe() indeed stops possibly annoying > error messages for EPROBE_DEFER (and that could be seen as a fix, not > really devm_clk_get_optional()). I honestly still don't see it as fix > but we are also not adding a Fixes tag so I don't really care :). > > (But I still think the commit message is a bit misleading) Yes, the commit message can be amended. I won't split these two since the fix part is not critical (nobody so far complained aloud about it). That's why I prefer to set the main point of the change as switching to devm_clk_get_optional() than deferred probe messages.
On Thu, Apr 14, 2022 at 03:48:42PM +0300, Andy Shevchenko wrote: > On Thu, Apr 14, 2022 at 09:07:29AM +0200, Nuno Sá wrote: > > On Wed, 2022-04-13 at 19:58 +0300, Andy Shevchenko wrote: ... > > (But I still think the commit message is a bit misleading) > > Yes, the commit message can be amended. I won't split these two since > the fix part is not critical (nobody so far complained aloud about it). > That's why I prefer to set the main point of the change as switching to > devm_clk_get_optional() than deferred probe messages. That said, I will amend it for v2. Thank you for review!
diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c index 287914016f28..fe520194a837 100644 --- a/drivers/iio/imu/adis16480.c +++ b/drivers/iio/imu/adis16480.c @@ -1362,31 +1362,25 @@ static int adis16480_get_ext_clocks(struct adis16480 *st) { struct device *dev = &st->adis.spi->dev; - st->clk_mode = ADIS16480_CLK_INT; - st->ext_clk = devm_clk_get(dev, "sync"); - if (!IS_ERR_OR_NULL(st->ext_clk)) { + st->ext_clk = devm_clk_get_optional(dev, "sync"); + if (IS_ERR(st->ext_clk)) + return dev_err_probe(dev, PTR_ERR(st->ext_clk), "failed to get ext clk\n"); + if (st->ext_clk) { st->clk_mode = ADIS16480_CLK_SYNC; return 0; } - if (PTR_ERR(st->ext_clk) != -ENOENT) { - dev_err(dev, "failed to get ext clk\n"); - return PTR_ERR(st->ext_clk); - } - if (st->chip_info->has_pps_clk_mode) { - st->ext_clk = devm_clk_get(dev, "pps"); - if (!IS_ERR_OR_NULL(st->ext_clk)) { + st->ext_clk = devm_clk_get_optional(dev, "pps"); + if (IS_ERR(st->ext_clk)) + return dev_err_probe(dev, PTR_ERR(st->ext_clk), "failed to get ext clk\n"); + if (st->ext_clk) { st->clk_mode = ADIS16480_CLK_PPS; return 0; } - - if (PTR_ERR(st->ext_clk) != -ENOENT) { - dev_err(dev, "failed to get ext clk\n"); - return PTR_ERR(st->ext_clk); - } } + st->clk_mode = ADIS16480_CLK_INT; return 0; } @@ -1447,7 +1441,7 @@ static int adis16480_probe(struct spi_device *spi) if (ret) return ret; - if (!IS_ERR_OR_NULL(st->ext_clk)) { + if (st->ext_clk) { ret = adis16480_ext_clk_config(st, true); if (ret) return ret;
The extended clocks are optional and may not be present for some SoCs supported by this driver. Nevertheless, in case the clock is provided but some error happens during its getting, that error should be handled properly. Use devm_clk_get_optional() API for that. Also report possible errors using dev_err_probe() to handle properly -EPROBE_DEFER error. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/iio/imu/adis16480.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)