On Tue, 28 Jan 2025 15:09:36 +0000
Nuno Sá [off-list ref] wrote:

On Tue, 2025-01-28 at 11:46 -0300, Jonathan Santos wrote:

quoted

On 01/28, Nuno Sá wrote:  

quoted

On Mon, 2025-01-27 at 19:29 -0600, David Lechner wrote:  

quoted

On 1/27/25 9:12 AM, Jonathan Santos wrote:  

quoted

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

quoted

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

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

quoted

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

quoted

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

quoted

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

quoted

+};
+
+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.

quoted

quoted

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.
  

quoted

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á

quoted