| Message ID | 0968f9cfc55c5ac80492a88bbe95fc8ff7208fa5.1737985435.git.Jonathan.Santos@analog.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add features, improvements, and fixes | expand |
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@analog.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, ®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. > + .max_register = AD7768_REG_MCLK_COUNTER, > +}; > + > static int ad7768_probe(struct spi_device *spi) > { > struct ad7768_state *st; > @@ -592,6 +619,13 @@ static int ad7768_probe(struct spi_device *spi) > > st->spi = spi; > > + st->regmap = devm_regmap_init(&spi->dev, > + &ad7768_regmap_bus, indio_dev, > + &ad7768_regmap_config); > + if (IS_ERR(st->regmap)) > + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap), > + "Failed to initialize regmap"); > + > st->vref = devm_regulator_get(&spi->dev, "vref"); > if (IS_ERR(st->vref)) > return PTR_ERR(st->vref);
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@analog.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, ®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" 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á >
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@analog.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, ®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á > > > >
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@analog.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, ®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... - Nuno Sá >
On Tue, 28 Jan 2025 15:09:36 +0000 Nuno Sá <noname.nuno@gmail.com> 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@analog.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, ®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 > > - Nuno Sá > > > > >
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); + /* Regular value size is 1 Byte, but 3 Bytes for ADC data */ + 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; - *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; /* * 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, +}; + +static const struct regmap_config ad7768_regmap_config = { + .name = "ad7768-1", + .reg_bits = 8, + .val_bits = 8, + .max_register = AD7768_REG_MCLK_COUNTER, +}; + static int ad7768_probe(struct spi_device *spi) { struct ad7768_state *st; @@ -592,6 +619,13 @@ static int ad7768_probe(struct spi_device *spi) st->spi = spi; + st->regmap = devm_regmap_init(&spi->dev, + &ad7768_regmap_bus, indio_dev, + &ad7768_regmap_config); + if (IS_ERR(st->regmap)) + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap), + "Failed to initialize regmap"); + st->vref = devm_regulator_get(&spi->dev, "vref"); if (IS_ERR(st->vref)) return PTR_ERR(st->vref);
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@analog.com> --- v2 Changes: * New patch in v2. --- drivers/iio/adc/ad7768-1.c | 82 +++++++++++++++++++++++++++----------- 1 file changed, 58 insertions(+), 24 deletions(-)