Message-ID: <648eedbee0e7702eda10034531de4611597cd9f2.camel@gmail.com>
Date: Tue, 28 Jan 2025 13:25:28 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: 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
Cc: 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, 	jonath4nns@...il.com,
 marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 07/16] iio: adc: ad7768-1: convert driver to use
 regmap

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"

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.

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.

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;
2) Or do what David suggests and use normal spi_*() and forget about regmap.

Either way is fine to me. 

- Nuno Sá
> 

Powered by blists - more mailing lists