Re: [PATCH 15/36] iio: remove conditional return with no effect

Hello,

On Fri, Jul 24, 2026 at 9:38 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 24 Jul 2026 03:45:17 +0900
> Sang-Heon Jeon <[email protected]> wrote:
>
> > Both branches of the check return the same value, so the check has
> > no effect. Remove it and return the value directly.
> >
> > This is the result of running the Coccinelle script from
> > scripts/coccinelle/misc/cond_return_no_effect.cocci.
> >
> > Signed-off-by: Sang-Heon Jeon <[email protected]>
> > ---
> >  drivers/iio/light/isl29028.c      |  6 +-----
> >  drivers/iio/light/tsl2583.c       | 14 +++-----------
> >  drivers/iio/magnetometer/ak8974.c |  6 +-----
> >  3 files changed, 5 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/iio/light/isl29028.c b/drivers/iio/light/isl29028.c
> > index b88e7c4eae3e..ea38d797082b 100644
> > --- a/drivers/iio/light/isl29028.c
> > +++ b/drivers/iio/light/isl29028.c
> > @@ -405,11 +405,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
> >       if (ret < 0)
> >               return ret;
> >
> > -     ret = isl29028_set_pm_runtime_busy(chip, false);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     return ret;
> > +     return isl29028_set_pm_runtime_busy(chip, false);
>
> That should have been a return 0.  The call to pm_runtime_put_auto_suspend()
> can return positive (in theory anyway).  The intent here was probably to eat
> that positive and ensure we return 0 or negative only from this function
>
> Please split up a fix for this as a separate patch.
>
> Even better if you have time would be to do that and then a second patch to
> get rid of this helper that adds no obvious value.  Smells like code
> that evolved into a slightly silly form!
>

I looked into it briefly and I agree with your analysis. Not eating
positive looks like a mistake to me too. Removing unnecessary helper
this time also sounds like a good idea.

> >  }
> >
> >  static int isl29028_read_raw(struct iio_dev *indio_dev,
> > diff --git a/drivers/iio/light/tsl2583.c b/drivers/iio/light/tsl2583.c
> > index a0dd122af2cf..a38a0130d854 100644
> > --- a/drivers/iio/light/tsl2583.c
> > +++ b/drivers/iio/light/tsl2583.c
> > @@ -456,12 +456,8 @@ static int tsl2583_chip_init_and_power_on(struct iio_dev *indio_dev)
> >
> >       usleep_range(3000, 3500);
> >
> > -     ret = tsl2583_set_power_state(chip, TSL2583_CNTL_PWR_ON |
> > -                                         TSL2583_CNTL_ADC_ENBL);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     return ret;
> > +     return tsl2583_set_power_state(chip,
> > +                                    TSL2583_CNTL_PWR_ON | TSL2583_CNTL_ADC_ENBL);
> >  }
> >
> >  /* Sysfs Interface Functions */
> > @@ -790,11 +786,7 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
> >               return ret;
> >       }
> >
> > -     ret = tsl2583_set_pm_runtime_busy(chip, false);
> > -     if (ret < 0)
> > -             return ret;
> > -
> > -     return ret;
> > +     return tsl2583_set_pm_runtime_busy(chip, false);
> Similar to case above.   It should have been eating the positive return
> by return 0;  Also the same suggested follow up cleanup applies in this driver.

Ditto. Nice catch :)

> >  }
> >
> >  static const struct iio_info tsl2583_info = {
> > diff --git a/drivers/iio/magnetometer/ak8974.c b/drivers/iio/magnetometer/ak8974.c
> > index c7fdb7c2f543..c988d0e40cea 100644
> > --- a/drivers/iio/magnetometer/ak8974.c
> > +++ b/drivers/iio/magnetometer/ak8974.c
> > @@ -379,11 +379,7 @@ static int ak8974_getresult(struct ak8974 *ak8974, __le16 *result)
> >               return -ERANGE;
> >       }
> >
> > -     ret = regmap_bulk_read(ak8974->map, AK8974_DATA_X, result, 6);
> > -     if (ret)
> > -             return ret;
> > -
> > -     return ret;
> > +     return regmap_bulk_read(ak8974->map, AK8974_DATA_X, result, 6);
> This one I agree with.
>
> Thanks for looking at these.  I suspect you may want to take a closer
> look at any other changes you've scripted that are similarl
>
> When code is doing something silly it might just be that, or it might
> be intended to do something not so silly!

I agree. This patch was generated by the script, and I only checked
whether it is a false positive or breaks the existing logic. Fixing
mistakes and improving code itself is always a good thing. I'll drop
this patch from the series and send it separately to IIO, including
the fixes and cleanups discussed here.

Thanks for your detailed review.

> Jonathan
>
> >  }
> >
> >  static irqreturn_t ak8974_drdy_irq(int irq, void *d)
>

Best Regards,
Sang-Heon Jeon.