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

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 28 Jan 2025 15:09:36 +0000
Nuno Sá <noname.nuno@gmail.com> wrote:

> 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...
I'm not keen on a mix or on a regmap that handles the size under the hood.
I think this quirk should be obvious.

Can we do two regmap, one for each of the register size?  We have other
drivers taking that approach and I'm not sure if it was ruled out for
this one.

Jonathan

> 
> - Nuno Sá
> >   
> 
> 
>