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

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2025-01-28 at 11:46 -0300, Jonathan Santos wrote:
> On 01/28, Nuno Sá wrote:
> > On Mon, 2025-01-27 at 19:29 -0600, David Lechner wrote:
> > > 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.
> > > 
> > 
> > Hmm I might be missing something but looking at the register map, It seems
> > we do
> > have 8bit registers? We do have values that span multiple registers (3 for
> > the
> > 24bit values) and regmap_bulk_read() should actually fit right? I mean,
> > looking
> > at the docs:
> > 
> > "regmap_bulk_read() - Read multiple sequential registers from the device"
> > 
> 
> Isn't regmap_bulk_*() for reading a value spread in sequential registers,
> like the offset calibration (registers 0x22, 0x23 and 0x24, 8 bits value
> for each reg)? For the ADC data (0x2C) we have a 24 bits value in only one
> register, so I beleive this does not apply.
> 

Ah got it. I failed to see that. Yeah, in that case you're right... it won't
work out of the box.

> > But I do agree that what we have right now does not make much sense. If we
> > need
> > to do
> > 
> > len = (addr == AD7768_REG_ADC_DATA) ? 3 : 1;
> > 
> > for supporting regmap, then I have to question using it. Also note that we
> > have
> > things like gain and offset that are also 3 bytes which means that our
> > custom
> > read would need to become more questionable if we add support for it.
> > 
> 
> For those cases the regmap_bulk_*() works.
> 
> > Jonathan, did you tried to use plain regmap (without the custom bus)?
> > Assuming
> > bulk reads work, I'm not seeing an apparent reason for the custom bus... I
> > would
> > also suspect that if bulk reads don't work out of the box, providing a
> > regmap
> > cache would make it work but relying on implementation details is not a very
> > good practice.
> > 
> 
> Yes, i tried and only works for the register with 8-bits value. David
> suggested using regular spi_*() functions for the unsual registers with
> 24-bits value, such as the ADC data (0x2C). That is the only way of
> having the default spi bus interface using regmap. Otherwise we should
> drop the regmap. 

Yeah, might be better to do plain spi for the unusual registers rather than the
custom bus. But no strong feelings on my side...

- Nuno Sá
>