Re: [PATCH v2 07/16] iio: adc: ad7768-1: convert driver to use regmap

[David Lechner <dlechner@baylibre.com>]

On 1/27/25 9:12 AM, Jonathan Santos wrote:
> Convert the AD7768-1 driver to use the regmap API for register
> access. This change simplifies and standardizes register interactions,
> reducing code duplication and improving maintainability.
> 
> Signed-off-by: Jonathan Santos <Jonathan.Santos@analog.com>
> ---
> v2 Changes:
> * New patch in v2.
> ---
>  drivers/iio/adc/ad7768-1.c | 82 +++++++++++++++++++++++++++-----------
>  1 file changed, 58 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7768-1.c b/drivers/iio/adc/ad7768-1.c
> index 95ba89435652..fb8d6fae5f8a 100644
> --- a/drivers/iio/adc/ad7768-1.c
> +++ b/drivers/iio/adc/ad7768-1.c
> @@ -12,6 +12,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/sysfs.h>
>  #include <linux/spi/spi.h>
> @@ -153,6 +154,7 @@ static const struct iio_chan_spec ad7768_channels[] = {
>  
>  struct ad7768_state {
>  	struct spi_device *spi;
> +	struct regmap *regmap;
>  	struct regulator *vref;
>  	struct mutex lock;
>  	struct clk *mclk;
> @@ -176,12 +178,17 @@ struct ad7768_state {
>  	} data __aligned(IIO_DMA_MINALIGN);
>  };
>  
> -static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
> -			       unsigned int len)
> +static int ad7768_spi_reg_read(void *context, unsigned int addr,
> +			       unsigned int *val)
>  {
> -	unsigned int shift;
> +	struct iio_dev *dev = context;
> +	struct ad7768_state *st;
> +	unsigned int shift, len;
>  	int ret;
>  
> +	st = iio_priv(dev);

This can be combined with the variable declaration.

> +	/* Regular value size is 1 Byte, but 3 Bytes for ADC data */

Probably not currently needed but COEFF_DATA register is also 3 bytes.

> +	len = (addr == AD7768_REG_ADC_DATA) ? 3 : 1;
>  	shift = 32 - (8 * len);
>  	st->data.d8[0] = AD7768_RD_FLAG_MSK(addr);
>  
> @@ -190,13 +197,19 @@ static int ad7768_spi_reg_read(struct ad7768_state *st, unsigned int addr,
>  	if (ret < 0)
>  		return ret;
>  
> -	return (be32_to_cpu(st->data.d32) >> shift);
> +	*val = be32_to_cpu(st->data.d32) >> shift;
> +
> +	return 0;
>  }
>  
> -static int ad7768_spi_reg_write(struct ad7768_state *st,
> +static int ad7768_spi_reg_write(void *context,
>  				unsigned int addr,
>  				unsigned int val)
>  {
> +	struct iio_dev *dev = context;
> +	struct ad7768_state *st;
> +
> +	st = iio_priv(dev);
>  	st->data.d8[0] = AD7768_WR_FLAG_MSK(addr);
>  	st->data.d8[1] = val & 0xFF;
>  
> @@ -206,16 +219,16 @@ static int ad7768_spi_reg_write(struct ad7768_state *st,
>  static int ad7768_set_mode(struct ad7768_state *st,
>  			   enum ad7768_conv_mode mode)
>  {
> -	int regval;
> +	int regval, ret;
>  
> -	regval = ad7768_spi_reg_read(st, AD7768_REG_CONVERSION, 1);
> -	if (regval < 0)
> -		return regval;
> +	ret = regmap_read(st->regmap, AD7768_REG_CONVERSION, &regval);
> +	if (ret)
> +		return ret;
>  
>  	regval &= ~AD7768_CONV_MODE_MSK;
>  	regval |= AD7768_CONV_MODE(mode);
>  
> -	return ad7768_spi_reg_write(st, AD7768_REG_CONVERSION, regval);
> +	return regmap_write(st->regmap, AD7768_REG_CONVERSION, regval);
>  }
>  
>  static int ad7768_scan_direct(struct iio_dev *indio_dev)
> @@ -234,9 +247,10 @@ static int ad7768_scan_direct(struct iio_dev *indio_dev)
>  	if (!ret)
>  		return -ETIMEDOUT;
>  
> -	readval = ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, 3);
> -	if (readval < 0)
> -		return readval;
> +	ret = regmap_read(st->regmap, AD7768_REG_ADC_DATA, &readval);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Any SPI configuration of the AD7768-1 can only be
>  	 * performed in continuous conversion mode.
> @@ -258,13 +272,11 @@ static int ad7768_reg_access(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&st->lock);
>  	if (readval) {
> -		ret = ad7768_spi_reg_read(st, reg, 1);
> -		if (ret < 0)
> +		ret = regmap_read(st->regmap, reg, readval);
> +		if (ret)
>  			goto err_unlock;

Can drop the if and goto.

> -		*readval = ret;
> -		ret = 0;
>  	} else {
> -		ret = ad7768_spi_reg_write(st, reg, writeval);
> +		ret = regmap_write(st->regmap, reg, writeval);
>  	}
>  err_unlock:
>  	mutex_unlock(&st->lock);
> @@ -283,7 +295,7 @@ static int ad7768_set_dig_fil(struct ad7768_state *st,
>  	else
>  		mode = AD7768_DIG_FIL_DEC_RATE(dec_rate);
>  
> -	ret = ad7768_spi_reg_write(st, AD7768_REG_DIGITAL_FILTER, mode);
> +	ret = regmap_write(st->regmap, AD7768_REG_DIGITAL_FILTER, mode);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -320,7 +332,7 @@ static int ad7768_set_freq(struct ad7768_state *st,
>  	 */
>  	pwr_mode = AD7768_PWR_MCLK_DIV(ad7768_clk_config[idx].mclk_div) |
>  		   AD7768_PWR_PWRMODE(ad7768_clk_config[idx].pwrmode);
> -	ret = ad7768_spi_reg_write(st, AD7768_REG_POWER_CLOCK, pwr_mode);
> +	ret = regmap_write(st->regmap, AD7768_REG_POWER_CLOCK, pwr_mode);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -447,11 +459,11 @@ static int ad7768_setup(struct ad7768_state *st)
>  	 * to 10. When the sequence is detected, the reset occurs.
>  	 * See the datasheet, page 70.
>  	 */
> -	ret = ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x3);
> +	ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x3);
>  	if (ret)
>  		return ret;
>  
> -	ret = ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x2);
> +	ret = regmap_write(st->regmap, AD7768_REG_SYNC_RESET, 0x2);
>  	if (ret)
>  		return ret;
>  
> @@ -509,18 +521,19 @@ static int ad7768_buffer_postenable(struct iio_dev *indio_dev)
>  	 * continuous read mode. Subsequent data reads do not require an
>  	 * initial 8-bit write to query the ADC_DATA register.
>  	 */
> -	return ad7768_spi_reg_write(st, AD7768_REG_INTERFACE_FORMAT, 0x01);
> +	return regmap_write(st->regmap, AD7768_REG_INTERFACE_FORMAT, 0x01);
>  }
>  
>  static int ad7768_buffer_predisable(struct iio_dev *indio_dev)
>  {
>  	struct ad7768_state *st = iio_priv(indio_dev);
> +	unsigned int regval;

Intention could be more clear by calling this "unused". Otherwise, it can look
like a bug if you don't fully understand what the comment below means.

>  
>  	/*
>  	 * To exit continuous read mode, perform a single read of the ADC_DATA
>  	 * reg (0x2C), which allows further configuration of the device.
>  	 */
> -	return ad7768_spi_reg_read(st, AD7768_REG_ADC_DATA, 3);
> +	return regmap_read(st->regmap, AD7768_REG_ADC_DATA, &regval);
>  }
>  
>  static const struct iio_buffer_setup_ops ad7768_buffer_ops = {
> @@ -563,6 +576,20 @@ static int ad7768_set_channel_label(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> +static const struct regmap_bus ad7768_regmap_bus = {
> +	.reg_write = ad7768_spi_reg_write,
> +	.reg_read = ad7768_spi_reg_read,
> +	.reg_format_endian_default = REGMAP_ENDIAN_BIG,
> +	.val_format_endian_default = REGMAP_ENDIAN_BIG,

The bus read function is calling be32_to_cpu(), so we probably want to remove
that or change the default here.

> +};
> +
> +static const struct regmap_config ad7768_regmap_config = {
> +	.name = "ad7768-1",
> +	.reg_bits = 8,
> +	.val_bits = 8,

Should this be 24 since the largest registers are 24-bit?

Another option could be to just use a regular spi_*() API for that register
instead of regmap_*() and avoid trying to do something that regmap doesn't
really handle.

Or we could possibly use regmap_bulk_read(), but that feels a bit hacky too
since it isn't actually how that function was intended to be used.

> +	.max_register = AD7768_REG_MCLK_COUNTER,
> +};
> +
>  static int ad7768_probe(struct spi_device *spi)
>  {
>  	struct ad7768_state *st;
> @@ -592,6 +619,13 @@ static int ad7768_probe(struct spi_device *spi)
>  
>  	st->spi = spi;
>  
> +	st->regmap = devm_regmap_init(&spi->dev,
> +				      &ad7768_regmap_bus, indio_dev,
> +				      &ad7768_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
> +				     "Failed to initialize regmap");
> +
>  	st->vref = devm_regulator_get(&spi->dev, "vref");
>  	if (IS_ERR(st->vref))
>  		return PTR_ERR(st->vref);