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 <[email protected]>
---
drivers/iio/imu/adis16480.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)
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;
--
2.35.1
> From: Andy Shevchenko <[email protected]>
> Sent: Wednesday, April 13, 2022 4:41 PM
> To: Sa, Nuno <[email protected]>; Andy Shevchenko
> <[email protected]>; [email protected];
> [email protected]
> Cc: Lars-Peter Clausen <[email protected]>; Hennerich, Michael
> <[email protected]>; Jonathan Cameron
> <[email protected]>
> 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
> <[email protected]>
> ---
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? <[email protected]>
On Wed, Apr 13, 2022 at 03:38:47PM +0000, Sa, Nuno wrote:
> > From: Andy Shevchenko <[email protected]>
> > 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? <[email protected]>
Thanks!
--
With Best Regards,
Andy Shevchenko
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!
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> > > > 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.
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> > > 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á
>
>