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@xxxxxxxxxx>
> > > ---
> > > 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, ®val);
> > > + 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, ®val);
> > > }
> > >
> > > 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á
> >
>
>
