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

[Jonathan Cameron]

On Fri, 24 Jul 2026 03:45:17 +0900
Sang-Heon Jeon <ekffu200098@xxxxxxxxx> 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 <ekffu200098@xxxxxxxxx>
> ---
>  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!


>  }
>  
>  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.

>  }
>  
>  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!

Jonathan

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