Re: [PATCH v2 3/3] staging: iio: ad9832: Add minor improvements to ad9832_write_powerdown

Dan Carpenter
2025-04-22T10:08:30Z

On Sun, Apr 20, 2025 at 01:47:25PM -0400, Gabriel Shahrouzi wrote:
> Minimize size of type that needs to be used by replacing unsigned long
> with bool. Avoid redundancy by checking if cached power state is the
> same as the one requested.
>
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> ---
> drivers/staging/iio/frequency/ad9832.c | 18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index a8fc20379efed..2ab6026d56b5c 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -173,13 +173,19 @@ static ssize_t ad9832_write_powerdown(struct device *dev, struct device_attribut
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct ad9832_state *st = iio_priv(indio_dev);
> int ret;
> - unsigned long val;
> + bool val;
> + bool cur;
>
> - ret = kstrtoul(buf, 10, &val);
> + ret = kstrtobool(buf, &val);
> if (ret)
> - goto error_ret;
> + return ret;

Fold this whole thing into patch 2. Don't write something and then fix
it in the next patch.

>
> mutex_lock(&st->lock);
> +
> + cur = !!(st->ctrl_src & AD9832_SLEEP);
> + if (val == cur)
> + goto unlock;
> +
> if (val)
> st->ctrl_src |= AD9832_SLEEP;
> else
> @@ -189,10 +195,10 @@ static ssize_t ad9832_write_powerdown(struct device *dev, struct device_attribut
> st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
> st->ctrl_src);
> ret = spi_sync(st->spi, &st->msg);
> - mutex_unlock(&st->lock);
>
> -error_ret:
> - return ret ? ret : len;
> +unlock:
> + mutex_unlock(&st->lock);
> + return len;

This should still be:

return ret ? ret : len;

regards,
dan carpenter