1. Patchew
  2. linux
  3. [PATCH v3 0/3] Fix and refactor output disable logic
  4. View patch

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

Gabriel Shahrouzi posted 3 patches 9 months, 3 weeks ago
[PATCH v3 2/3] staging: iio: ad9832: Refactor powerdown control
Posted by Gabriel Shahrouzi 9 months, 3 weeks ago 
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
Re: [PATCH v3 2/3] staging: iio: ad9832: Refactor powerdown control
Posted by Jonathan Cameron 9 months, 3 weeks ago 
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 2/3] staging: iio: ad9832: Refactor powerdown control
Posted by Gabriel Shahrouzi 9 months, 2 weeks ago 
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 2/3] staging: iio: ad9832: Refactor powerdown control
Posted by Jonathan Cameron 9 months, 2 weeks ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.