[v3] Fix and refactor output disable logic

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

Posted by Gabriel Shahrouzi 9 months, 3 weeks ago

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;
 
 	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;
 }
 
 static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
-- 
2.43.0

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

Posted by Jonathan Cameron 9 months, 3 weeks ago

On Sun, 20 Apr 2025 13:54:19 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> 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);
That could have been done in the previous patch as you are changing the ABI anyway.
>  	if (ret)
> -		goto error_ret;
> +		return ret;
>  
>  	mutex_lock(&st->lock);
> +
> +	cur = !!(st->ctrl_src & AD9832_SLEEP);

Worth thinking about whether this driver will ever be converted to regmap with regcache.
If that's a reasonable thing to do long term this sort of optimization adds nothing
useful as we'll skip the right anwyay if the driver is already in the appropriate state.

This isn't a high performance path, so I'd not bother with the check for now.

> +	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);
If this fails, where doe ret end up? Looks to me like we now don't report the error
Anyhow, see guard() comment in previous patch.  These days we can avoid a lot of
this complexity by using that to allow direct returns where we first see the error.

> -	mutex_unlock(&st->lock);
>  
> -error_ret:
> -	return ret ? ret : len;
> +unlock:
> +	mutex_unlock(&st->lock);
> +	return len;
>  }
>  
>  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,

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

Posted by Gabriel Shahrouzi 9 months, 2 weeks ago

On Mon, Apr 21, 2025 at 7:40 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 20 Apr 2025 13:54:19 -0400
> Gabriel Shahrouzi <gshahrouzi@gmail.com> 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);
> That could have been done in the previous patch as you are changing the ABI anyway.
Got it.
> >       if (ret)
> > -             goto error_ret;
> > +             return ret;
> >
> >       mutex_lock(&st->lock);
> > +
> > +     cur = !!(st->ctrl_src & AD9832_SLEEP);
>
> Worth thinking about whether this driver will ever be converted to regmap with regcache.
> If that's a reasonable thing to do long term this sort of optimization adds nothing
> useful as we'll skip the right anwyay if the driver is already in the appropriate state.
Got it.
>
> This isn't a high performance path, so I'd not bother with the check for now.
Got it.
>
> > +     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);
> If this fails, where doe ret end up? Looks to me like we now don't report the error
> Anyhow, see guard() comment in previous patch.  These days we can avoid a lot of
> this complexity by using that to allow direct returns where we first see the error.
Ah right, fixed that for the new version and removed the complexity by
directly returning.
>
> > -     mutex_unlock(&st->lock);
> >
> > -error_ret:
> > -     return ret ? ret : len;
> > +unlock:
> > +     mutex_unlock(&st->lock);
> > +     return len;
> >  }
> >
> >  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>

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

Posted by Jonathan Cameron 9 months, 2 weeks ago

On Thu, 24 Apr 2025 18:29:57 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:

> On Mon, Apr 21, 2025 at 7:40 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 20 Apr 2025 13:54:19 -0400
> > Gabriel Shahrouzi <gshahrouzi@gmail.com> 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.
> > >
Hi Garbiel,

Quick process comment given you are sending quite a few emails (which is great!)

Please don't bother replying to say you are doing something suggested.
Focus any reply (including cropping irrelevant context) on what you want to
discuss.  If there is nothing to discuss your reply is effectively the change
log of the next version of the series.

Even a simple email like this one takes a little time for reviewers to look
at and they gain no extra information from doing so.

Thanks,

Jonathan