On 01/30, Jonathan Cameron wrote:
> On Tue, 28 Jan 2025 15:09:36 +0000
> Nuno Sá <noname.nuno@xxxxxxxxx> 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@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.
> > >
> >
> > 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
>
Yes, it makes sense a regmap for each register size, if everyone agrees.
I will try this here.
> >
> > - Nuno Sá
> > >
> >
> >
> >
>
