Message-ID: <20251206161757.32e4cf0d@jic23-huawei>
Date: Sat, 6 Dec 2025 16:17:57 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Tomas Borquez <tomasborquez13@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Greg
 Kroah-Hartman <gregkh@...uxfoundation.org>, David Lechner
 <dlechner@...libre.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-staging@...ts.linux.dev
Subject: Re: [RFC PATCH 2/3] staging: iio: ad9832: convert to iio channels

On Fri,  5 Dec 2025 17:27:42 -0300
Tomas Borquez <tomasborquez13@...il.com> wrote:

> Replace the custom frequency and phase sysfs attributes with IIO channels
> using read_raw()/write_raw() callbacks, as well as removing the dds.h
> header.
> 
> Changes:
> - Add iio_chan_spec definitions for 2 frequency and 4 phase channels.
> - Implement read_raw/write_raw for IIO_CHAN_INFO_FREQUENCY/PHASE.
> - Cache frequency and phase values in driver state for readback.
> - Remove dependency on dds.h macros for sysfs.
> - Use guard(mutex) for cleaner locking.
> - Add input validation and consistent error messages.
> 
Hi Tomas,

> NOTE: This changes the userspace ABI, see cover letter.
Given I responded there on ABI, I'll just ignore that aspect for this review.

Code is pretty clean, but there are a few things that belong in other
patches rather than being mixed in here.
For kernel code we are pretty strict on one patch, one type of change.

> Signed-off-by: Tomas Borquez <tomasborquez13@...il.com>
> ---
>  drivers/staging/iio/frequency/ad9832.c | 232 ++++++++++++++++++-------
>  1 file changed, 168 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index e2ad3e5a7a..79d26009d1 100644
> --- a/drivers/staging/iio/frequency/ad9832.c
> +++ b/drivers/staging/iio/frequency/ad9832.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/device.h>
>  #include <linux/err.h>
> @@ -23,10 +24,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> -#include "dds.h"
> -
>  /* Registers */
> -
Trivial but don't make unrelated white space changes in a patch
doing anything else - they add noise and hurt reviewer speed.
Put them in a patch on their own.

>  #define AD9832_FREQ0LL		0x0
>  #define AD9832_FREQ0HL		0x1
>  #define AD9832_FREQ0LM		0x2
> @@ -50,7 +48,6 @@
>  #define AD9832_OUTPUT_EN	0x13
>  
>  /* Command Control Bits */
> -

>  	} __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
> +static unsigned long ad9832_calc_freqreg(unsigned long mclk, u32 fout)
>  {
>  	unsigned long long freqreg = (u64)fout *
>  				     (u64)((u64)1L << AD9832_FREQ_BITS);
> @@ -124,12 +124,24 @@ static unsigned long ad9832_calc_freqreg(unsigned long mclk, unsigned long fout)
>  }
>  
>  static int ad9832_write_frequency(struct ad9832_state *st,
> -				  unsigned int addr, unsigned long fout)
> +				  unsigned int addr, u32 fout)
>  {
>  	unsigned long clk_freq;
>  	unsigned long regval;
>  	u8 regval_bytes[4];
>  	u16 freq_cmd;
> +	int ret, idx;
> +
> +	switch (addr) {
> +	case AD9832_FREQ0HM:
> +		idx = 0;
> +		break;
> +	case AD9832_FREQ1HM:
> +		idx = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	clk_freq = clk_get_rate(st->mclk);
>  
> @@ -147,14 +159,37 @@ static int ad9832_write_frequency(struct ad9832_state *st,
>  			FIELD_PREP(AD9832_DAT_MSK, regval_bytes[i]));
>  	}
>  
> -	return spi_sync(st->spi, &st->freq_msg);
> +	ret = spi_sync(st->spi, &st->freq_msg);
> +	if (ret)
> +		return ret;
> +
> +	st->freq[idx] = fout;

I'd put a blank line here.  Generally slightly helps readability before
simple return statements like this.

> +	return 0;
>  }
>  
>  static int ad9832_write_phase(struct ad9832_state *st,
> -			      unsigned long addr, unsigned long phase)
> +			      unsigned long addr, u32 phase)
>  {
>  	u8 phase_bytes[2];
>  	u16 phase_cmd;
> +	int ret, idx;
> +
> +	switch (addr) {
> +	case AD9832_PHASE0H:
> +		idx = 0;
> +		break;
> +	case AD9832_PHASE1H:
> +		idx = 1;
> +		break;
> +	case AD9832_PHASE2H:
> +		idx = 2;
> +		break;
> +	case AD9832_PHASE3H:
> +		idx = 3;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>  
>  	if (phase >= BIT(AD9832_PHASE_BITS))
>  		return -EINVAL;
> @@ -169,10 +204,77 @@ static int ad9832_write_phase(struct ad9832_state *st,
>  			FIELD_PREP(AD9832_DAT_MSK, phase_bytes[i]));
>  	}
>  
> -	return spi_sync(st->spi, &st->phase_msg);
> +	ret = spi_sync(st->spi, &st->phase_msg);
> +	if (ret)
> +		return ret;
> +
> +	st->phase[idx] = phase;
> +	return 0;
>  }
>  
> -static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> +static int ad9832_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad9832_state *st = iio_priv(indio_dev);
> +
> +	if (val < 0)
> +		return -EINVAL;

Check val2 as well.  Should be zero.


> +
> +	guard(mutex)(&st->lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		return ad9832_write_frequency(st, chan->address, val);
> +	case IIO_CHAN_INFO_PHASE:
> +		return ad9832_write_phase(st, chan->address, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static ssize_t ad9832_store(struct device *dev,
> +			    struct device_attribute *attr,
>  			    const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> @@ -183,20 +285,10 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  
>  	ret = kstrtoul(buf, 10, &val);
>  	if (ret)
> -		goto error_ret;
> +		return ret;
>  
> -	mutex_lock(&st->lock);
> -	switch ((u32)this_attr->address) {
> -	case AD9832_FREQ0HM:
> -	case AD9832_FREQ1HM:
> -		ret = ad9832_write_frequency(st, this_attr->address, val);
> -		break;
> -	case AD9832_PHASE0H:
> -	case AD9832_PHASE1H:
> -	case AD9832_PHASE2H:
> -	case AD9832_PHASE3H:
> -		ret = ad9832_write_phase(st, this_attr->address, val);
> -		break;
> +	guard(mutex)(&st->lock);

Ideally do guard() changes in a separate patch as seems unrelated to the
other stuff going on here. I'd suggest making that change first.

> +	switch (this_attr->address) {
>  	case AD9832_PINCTRL_EN:
>  		st->ctrl_ss &= ~AD9832_SELSRC;
>  		st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
> @@ -206,13 +298,13 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  		ret = spi_sync(st->spi, &st->msg);
>  		break;
>  	case AD9832_FREQ_SYM:
> -		if (val == 1 || val == 0) {
> -			st->ctrl_fp &= ~AD9832_FREQ;
> -			st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 1 : 0);
> -		} else {
> +		if (val != 1 && val != 0) {
>  			ret = -EINVAL;

With guard, should be able to directly return in error cases
simplifying the flow and helping code readability.  That's one of the
nicest things guard() enables.

>  			break;
>  		}
> +
> +		st->ctrl_fp &= ~AD9832_FREQ;
> +		st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val);
>  		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
>  						  st->ctrl_fp);
>  		ret = spi_sync(st->spi, &st->msg);
> @@ -243,47 +335,56 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
>  	default:
>  		ret = -ENODEV;
>  	}
> -	mutex_unlock(&st->lock);
>  
> -error_ret:
>  	return ret ? ret : len;
>  }

>  
> @@ -309,15 +412,15 @@ static int ad9832_probe(struct spi_device *spi)
>  
>  	ret = devm_regulator_get_enable(&spi->dev, "avdd");
>  	if (ret)
> -		return dev_err_probe(&spi->dev, ret, "failed to enable specified AVDD voltage\n");
> +		return dev_err_probe(&spi->dev, ret, "failed to enable AVDD supply\n");
>  
>  	ret = devm_regulator_get_enable(&spi->dev, "dvdd");
>  	if (ret)
> -		return dev_err_probe(&spi->dev, ret, "Failed to enable specified DVDD supply\n");
> +		return dev_err_probe(&spi->dev, ret, "failed to enable DVDD supply\n");
>  
>  	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
>  	if (IS_ERR(st->mclk))
> -		return PTR_ERR(st->mclk);
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->mclk), "failed to enable MCLK\n");
This is an unrelated change.  Do it in a separate patch for just the dev_err_probe()
usage.

>  

Powered by blists - more mailing lists