[PATCH v3 0/3] Fix and refactor output disable logic

[PATCH v3 0/3] Fix and refactor output disable logic

[Gabriel Shahrouzi]

Patch 1 includes the initial fix.

Patch 2 refactors the code to use the out_altvoltage_powerdown ABI.

Patch 3 adds small improvements by minimizing the size of types and
doing a redundancy check.

Not sure whether to include a read function for powerdown as well since
all the other attributes only had write permissions. I can also do this
for the other attributes to help modernize the driver.

Changes in v3:
	- Include version log in cover letter.
Changes in v2:
	- Refactor and make small improvements ontop of the initial fix.

Gabriel Shahrouzi (3):
  iio: frequency: Use SLEEP bit instead of RESET to disable output
  staging: iio: ad9832: Refactor powerdown control
  staging: iio: ad9832: Add minor improvements to ad9832_write_powerdown

 drivers/staging/iio/frequency/ad9832.c | 50 ++++++++++++++++++--------
 1 file changed, 36 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] iio: frequency: Use SLEEP bit instead of RESET to disable output

[Gabriel Shahrouzi]

According to the AD9832 datasheet (Table 10, D12 description), setting
the RESET bit forces the phase accumulator to zero, which corresponds to
a full-scale DC output, rather than disabling the output signal.

The correct way to disable the output and enter a low-power state is to
set the AD9832_SLEEP bit (Table 10, D13 description), which powers down
the internal DAC current sources and disables internal clocks.

Fixes: ea707584bac1 ("Staging: IIO: DDS: AD9832 / AD9835 driver")
Cc: stable@vger.kernel.org
Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index db42810c7664b..0872ff4ec4896 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -232,7 +232,7 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 			st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
 					AD9832_CLR);
 		else
-			st->ctrl_src |= AD9832_RESET;
+			st->ctrl_src |= AD9832_SLEEP;
 
 		st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
 					st->ctrl_src);
-- 
2.43.0

[PATCH v3 2/3] staging: iio: ad9832: Refactor powerdown control

[Gabriel Shahrouzi]

Replace custom implementation with out_altvoltage_powerdown ABI. The
attribute's logic is inverted (1 now enables powerdown) to match the
standard. Modernize driver by using the standard IIO interface.

Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
---
 drivers/staging/iio/frequency/ad9832.c | 44 ++++++++++++++++++--------
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 0872ff4ec4896..a8fc20379efed 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -167,6 +167,34 @@ static int ad9832_write_phase(struct ad9832_state *st,
 	return spi_sync(st->spi, &st->phase_msg);
 }
 
+static ssize_t ad9832_write_powerdown(struct device *dev, struct device_attribute *attr,
+				      const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct ad9832_state *st = iio_priv(indio_dev);
+	int ret;
+	unsigned long val;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		goto error_ret;
+
+	mutex_lock(&st->lock);
+	if (val)
+		st->ctrl_src |= AD9832_SLEEP;
+	else
+		st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
+				 AD9832_CLR);
+
+	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;
+}
+
 static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 			    const char *buf, size_t len)
 {
@@ -227,17 +255,6 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 					st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
 		break;
-	case AD9832_OUTPUT_EN:
-		if (val)
-			st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
-					AD9832_CLR);
-		else
-			st->ctrl_src |= AD9832_SLEEP;
-
-		st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
-					st->ctrl_src);
-		ret = spi_sync(st->spi, &st->msg);
-		break;
 	default:
 		ret = -ENODEV;
 	}
@@ -266,8 +283,7 @@ static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
 
 static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
 				ad9832_write, AD9832_PINCTRL_EN);
-static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
-				ad9832_write, AD9832_OUTPUT_EN);
+static IIO_DEVICE_ATTR(out_altvoltage_powerdown, 0200, NULL, ad9832_write_powerdown, 0);
 
 static struct attribute *ad9832_attributes[] = {
 	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
@@ -281,7 +297,7 @@ static struct attribute *ad9832_attributes[] = {
 	&iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
 	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
-	&iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
+	&iio_dev_attr_out_altvoltage_powerdown.dev_attr.attr,
 	NULL,
 };
 
-- 
2.43.0

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

[Gabriel Shahrouzi]

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 0/3] Fix and refactor output disable logic

[Jonathan Cameron]

On Sun, 20 Apr 2025 13:54:16 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:

> Patch 1 includes the initial fix.
> 
> Patch 2 refactors the code to use the out_altvoltage_powerdown ABI.
> 
> Patch 3 adds small improvements by minimizing the size of types and
> doing a redundancy check.
> 
> Not sure whether to include a read function for powerdown as well since
> all the other attributes only had write permissions. I can also do this
> for the other attributes to help modernize the driver.
> 
> Changes in v3:
> 	- Include version log in cover letter.
Just post it in reply to that v2!

Note though that this needs a rebase as I mentioned in the thread wrt to
the original fix. I'll take a quick look though to see if I can spot
anything else for v4.

> Changes in v2:
> 	- Refactor and make small improvements ontop of the initial fix.
> 
> Gabriel Shahrouzi (3):
>   iio: frequency: Use SLEEP bit instead of RESET to disable output
>   staging: iio: ad9832: Refactor powerdown control
>   staging: iio: ad9832: Add minor improvements to ad9832_write_powerdown
> 
>  drivers/staging/iio/frequency/ad9832.c | 50 ++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 14 deletions(-)
> 

Re: [PATCH v3 2/3] staging: iio: ad9832: Refactor powerdown control

[Jonathan Cameron]

On Sun, 20 Apr 2025 13:54:18 -0400
Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:

> Replace custom implementation with out_altvoltage_powerdown ABI. The
> attribute's logic is inverted (1 now enables powerdown) to match the
> standard. Modernize driver by using the standard IIO interface.
> 
> Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> ---
>  drivers/staging/iio/frequency/ad9832.c | 44 ++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 0872ff4ec4896..a8fc20379efed 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -167,6 +167,34 @@ static int ad9832_write_phase(struct ad9832_state *st,
>  	return spi_sync(st->spi, &st->phase_msg);
>  }
>  
> +static ssize_t ad9832_write_powerdown(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct ad9832_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned long val;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret)
> +		goto error_ret;
> +
> +	mutex_lock(&st->lock);

Look at how guard(mutex)(&st->lock);
can be used in this driver to simplify things considerably.
May make sense to do that before introducing this new code.

> +	if (val)
> +		st->ctrl_src |= AD9832_SLEEP;
> +	else
> +		st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> +				 AD9832_CLR);
> +
> +	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;
> +}
> +
>  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  			    const char *buf, size_t len)
>  {
> @@ -227,17 +255,6 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  					st->ctrl_fp);
>  		ret = spi_sync(st->spi, &st->msg);
>  		break;
> -	case AD9832_OUTPUT_EN:
> -		if (val)
> -			st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> -					AD9832_CLR);
> -		else
> -			st->ctrl_src |= AD9832_SLEEP;
> -
> -		st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
> -					st->ctrl_src);
> -		ret = spi_sync(st->spi, &st->msg);
> -		break;
>  	default:
>  		ret = -ENODEV;
>  	}
> @@ -266,8 +283,7 @@ static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
>  
>  static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
>  				ad9832_write, AD9832_PINCTRL_EN);
> -static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
> -				ad9832_write, AD9832_OUTPUT_EN);
> +static IIO_DEVICE_ATTR(out_altvoltage_powerdown, 0200, NULL, ad9832_write_powerdown, 0);

Take a look at the use of extended attributes used for this like we see
in ad5064.c
That will need an actual channel though so is a more significant rework.

>  
>  static struct attribute *ad9832_attributes[] = {
>  	&iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> @@ -281,7 +297,7 @@ static struct attribute *ad9832_attributes[] = {
>  	&iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
>  	&iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
> -	&iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
> +	&iio_dev_attr_out_altvoltage_powerdown.dev_attr.attr,
>  	NULL,
>  };
>  

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

[Jonathan Cameron]

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 0/3] Fix and refactor output disable logic

[Gabriel Shahrouzi]

On Mon, Apr 21, 2025 at 7:33 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 20 Apr 2025 13:54:16 -0400
> Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:
>
> > Patch 1 includes the initial fix.
> >
> > Patch 2 refactors the code to use the out_altvoltage_powerdown ABI.
> >
> > Patch 3 adds small improvements by minimizing the size of types and
> > doing a redundancy check.
> >
> > Not sure whether to include a read function for powerdown as well since
> > all the other attributes only had write permissions. I can also do this
> > for the other attributes to help modernize the driver.
> >
> > Changes in v3:
> >       - Include version log in cover letter.
> Just post it in reply to that v2!
Got it.
>
> Note though that this needs a rebase as I mentioned in the thread wrt to
> the original fix. I'll take a quick look though to see if I can spot
> anything else for v4.
Got it.
>
> > Changes in v2:
> >       - Refactor and make small improvements ontop of the initial fix.
> >
> > Gabriel Shahrouzi (3):
> >   iio: frequency: Use SLEEP bit instead of RESET to disable output
> >   staging: iio: ad9832: Refactor powerdown control
> >   staging: iio: ad9832: Add minor improvements to ad9832_write_powerdown
> >
> >  drivers/staging/iio/frequency/ad9832.c | 50 ++++++++++++++++++--------
> >  1 file changed, 36 insertions(+), 14 deletions(-)
> >
>

Re: [PATCH v3 0/3] Fix and refactor output disable logic

[Dan Carpenter]

On Sun, Apr 20, 2025 at 01:54:16PM -0400, Gabriel Shahrouzi wrote:
> Patch 1 includes the initial fix.
> 
> Patch 2 refactors the code to use the out_altvoltage_powerdown ABI.
> 
> Patch 3 adds small improvements by minimizing the size of types and
> doing a redundancy check.
> 
> Not sure whether to include a read function for powerdown as well since
> all the other attributes only had write permissions. I can also do this
> for the other attributes to help modernize the driver.
> 
> Changes in v3:
> 	- Include version log in cover letter.

Please don't resend patches the same day.  Give us a chance to review
it otherwise it gets split across multiple threads.

regards,
dan carpenter

Re: [PATCH v3 0/3] Fix and refactor output disable logic

[Gabriel Shahrouzi]

On Tue, Apr 22, 2025 at 6:09 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Sun, Apr 20, 2025 at 01:54:16PM -0400, Gabriel Shahrouzi wrote:
> > Patch 1 includes the initial fix.
> >
> > Patch 2 refactors the code to use the out_altvoltage_powerdown ABI.
> >
> > Patch 3 adds small improvements by minimizing the size of types and
> > doing a redundancy check.
> >
> > Not sure whether to include a read function for powerdown as well since
> > all the other attributes only had write permissions. I can also do this
> > for the other attributes to help modernize the driver.
> >
> > Changes in v3:
> >       - Include version log in cover letter.
>
> Please don't resend patches the same day.  Give us a chance to review
> it otherwise it gets split across multiple threads.
Noted, thanks.
>
> regards,
> dan carpenter
>

Re: [PATCH v3 2/3] staging: iio: ad9832: Refactor powerdown control

[Gabriel Shahrouzi]

On Mon, Apr 21, 2025 at 7:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 20 Apr 2025 13:54:18 -0400
> Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:
>
> > Replace custom implementation with out_altvoltage_powerdown ABI. The
> > attribute's logic is inverted (1 now enables powerdown) to match the
> > standard. Modernize driver by using the standard IIO interface.
> >
> > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > ---
> >  drivers/staging/iio/frequency/ad9832.c | 44 ++++++++++++++++++--------
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > index 0872ff4ec4896..a8fc20379efed 100644
> > --- a/drivers/staging/iio/frequency/ad9832.c
> > +++ b/drivers/staging/iio/frequency/ad9832.c
> > @@ -167,6 +167,34 @@ static int ad9832_write_phase(struct ad9832_state *st,
> >       return spi_sync(st->spi, &st->phase_msg);
> >  }
> >
> > +static ssize_t ad9832_write_powerdown(struct device *dev, struct device_attribute *attr,
> > +                                   const char *buf, size_t len)
> > +{
> > +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +     struct ad9832_state *st = iio_priv(indio_dev);
> > +     int ret;
> > +     unsigned long val;
> > +
> > +     ret = kstrtoul(buf, 10, &val);
> > +     if (ret)
> > +             goto error_ret;
> > +
> > +     mutex_lock(&st->lock);
>
> Look at how guard(mutex)(&st->lock);
> can be used in this driver to simplify things considerably.
Noted, added into new version.
> May make sense to do that before introducing this new code.
Not sure whether to have made it its own patch or not. I grouped it
together with the new code since it also uses locking.
>
> > +     if (val)
> > +             st->ctrl_src |= AD9832_SLEEP;
> > +     else
> > +             st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> > +                              AD9832_CLR);
> > +
> > +     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;
> > +}
> > +
> >  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> >                           const char *buf, size_t len)
> >  {
> > @@ -227,17 +255,6 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> >                                       st->ctrl_fp);
> >               ret = spi_sync(st->spi, &st->msg);
> >               break;
> > -     case AD9832_OUTPUT_EN:
> > -             if (val)
> > -                     st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> > -                                     AD9832_CLR);
> > -             else
> > -                     st->ctrl_src |= AD9832_SLEEP;
> > -
> > -             st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
> > -                                     st->ctrl_src);
> > -             ret = spi_sync(st->spi, &st->msg);
> > -             break;
> >       default:
> >               ret = -ENODEV;
> >       }
> > @@ -266,8 +283,7 @@ static IIO_CONST_ATTR_PHASE_SCALE(0, "0.0015339808"); /* 2PI/2^12 rad*/
> >
> >  static IIO_DEV_ATTR_PINCONTROL_EN(0, 0200, NULL,
> >                               ad9832_write, AD9832_PINCTRL_EN);
> > -static IIO_DEV_ATTR_OUT_ENABLE(0, 0200, NULL,
> > -                             ad9832_write, AD9832_OUTPUT_EN);
> > +static IIO_DEVICE_ATTR(out_altvoltage_powerdown, 0200, NULL, ad9832_write_powerdown, 0);
>
> Take a look at the use of extended attributes used for this like we see
> in ad5064.c
> That will need an actual channel though so is a more significant rework.
Got it.
>
> >
> >  static struct attribute *ad9832_attributes[] = {
> >       &iio_dev_attr_out_altvoltage0_frequency0.dev_attr.attr,
> > @@ -281,7 +297,7 @@ static struct attribute *ad9832_attributes[] = {
> >       &iio_dev_attr_out_altvoltage0_pincontrol_en.dev_attr.attr,
> >       &iio_dev_attr_out_altvoltage0_frequencysymbol.dev_attr.attr,
> >       &iio_dev_attr_out_altvoltage0_phasesymbol.dev_attr.attr,
> > -     &iio_dev_attr_out_altvoltage0_out_enable.dev_attr.attr,
> > +     &iio_dev_attr_out_altvoltage_powerdown.dev_attr.attr,
> >       NULL,
> >  };
> >
>

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

[Gabriel Shahrouzi]

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 2/3] staging: iio: ad9832: Refactor powerdown control

[Jonathan Cameron]

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

> On Mon, Apr 21, 2025 at 7:37 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sun, 20 Apr 2025 13:54:18 -0400
> > Gabriel Shahrouzi <gshahrouzi@gmail.com> wrote:
> >  
> > > Replace custom implementation with out_altvoltage_powerdown ABI. The
> > > attribute's logic is inverted (1 now enables powerdown) to match the
> > > standard. Modernize driver by using the standard IIO interface.
> > >
> > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@gmail.com>
> > > ---
> > >  drivers/staging/iio/frequency/ad9832.c | 44 ++++++++++++++++++--------
> > >  1 file changed, 30 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> > > index 0872ff4ec4896..a8fc20379efed 100644
> > > --- a/drivers/staging/iio/frequency/ad9832.c
> > > +++ b/drivers/staging/iio/frequency/ad9832.c
> > > @@ -167,6 +167,34 @@ static int ad9832_write_phase(struct ad9832_state *st,
> > >       return spi_sync(st->spi, &st->phase_msg);
> > >  }
> > >
> > > +static ssize_t ad9832_write_powerdown(struct device *dev, struct device_attribute *attr,
> > > +                                   const char *buf, size_t len)
> > > +{
> > > +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > +     struct ad9832_state *st = iio_priv(indio_dev);
> > > +     int ret;
> > > +     unsigned long val;
> > > +
> > > +     ret = kstrtoul(buf, 10, &val);
> > > +     if (ret)
> > > +             goto error_ret;
> > > +
> > > +     mutex_lock(&st->lock);  
> >
> > Look at how guard(mutex)(&st->lock);
> > can be used in this driver to simplify things considerably.  
> Noted, added into new version.
> > May make sense to do that before introducing this new code.  
> Not sure whether to have made it its own patch or not. I grouped it
> together with the new code since it also uses locking.
For new code it is fine to do it at the same time.

If there are other places it makes sense in the driver, separate
patch covering all those.

Jonathan

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

[Jonathan Cameron]

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