Message-ID: <Z5jtx6QaSMxBXoyk@JSANTO12-L01.ad.analog.com>
Date: Tue, 28 Jan 2025 11:46:31 -0300
From: Jonathan Santos <jonath4nns@...il.com>
To: Nuno Sá <noname.nuno@...il.com>
Cc: David Lechner <dlechner@...libre.com>,
	Jonathan Santos <Jonathan.Santos@...log.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, lars@...afoo.de,
	Michael.Hennerich@...log.com, marcelo.schmitt@...log.com,
	jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 07/16] iio: adc: ad7768-1: convert driver to use regmap

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@...log.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.

> 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. 

> Anyways, I would try would normal regmap and if bulk reads don't work I would
> either:
> 
> 1) Just do three regmap_reads() for 3byte values;
Also does not work.
> 2) Or do what David suggests and use normal spi_*() and forget about regmap.
> 
Either that or using spi_*() only for ADC data and regmap for the rest
of the registers.
> Either way is fine to me. 
> 
> - Nuno Sá
> > 
> 
> 

Powered by blists - more mailing lists