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

Gabriel Shahrouzi
2025-04-22T16:26:25Z

On Tue, Apr 22, 2025 at 6:08 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> 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.
Ah, I forgot to respond to this part — makes sense. I was initially
trying to mirror the original author’s style and build on top of it,
but I see now that all the changes should be folded into a single
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
>