Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache

[Lothar Rubusch <l.rubusch@gmail.com>]

On Sun, Jun 8, 2025 at 5:38 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sun, 8 Jun 2025 16:22:15 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
>
> > On Sun,  1 Jun 2025 17:21:31 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >
> > > Setup regmap cache to cache register configuration. This is a preparatory
> > > step for follow up patches. Using cached settings will help at inerrupt
> > > handling, to generate activity and inactivity events.
> >
> > The regmap cache will reduce traffic to the device for things like reading
> > back sampling frequency, so no need to justify this patch with 'future'
> > stuff.  Justify it with current.   I've applied with the description of
> > simply
> >
> > "Setup regmap cache to cache register configuration, reducing bus traffic
> > for repeated accesses to non volatile registers."
> >
> Dropped again.  The is_volatile should include all volatile registers
> not just ones we happen to be using so far.
>

I see among the patches, REG_INT_SOURCE is added later. For a v5 then
I'll prepare a patch which sets up all registers - including
REG_INT_SOURCE right away. Correct?

Then it should be added the following line:
bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
{
    switch (reg) {
    case ADXL313_REG_DATA_AXIS(0):
    case ADXL313_REG_DATA_AXIS(1):
    case ADXL313_REG_DATA_AXIS(2):
    case ADXL313_REG_DATA_AXIS(3):
    case ADXL313_REG_DATA_AXIS(4):
    case ADXL313_REG_DATA_AXIS(5):
    case ADXL313_REG_FIFO_STATUS:
+    case ADXL313_REG_INT_SOURCE:
        return true;
    default:
        return false;
    }
}

> You added debug accesses in previous patch which will not take the volatile
> nature into account unless the register is in that switch statement.

This is not quite clear to me. What am I missing here?

When I try to find iio drivers using "debugfs" and having a
"volatile_reg" called specification (using either ranges or by a
function), I could only identify the following drivers:
./drivers/iio/accel/msa311.c
./drivers/iio/adc/ad7380.c
./drivers/iio/adc/ina2xx-adc.c
./drivers/iio/imu/bno055/bno055.c
./drivers/iio/light/gp2ap020a00f.c

I tried to find if there is a special declaration of debug registers
in the volatile_reg list, but could not find any.

Most interesting here was:
./drivers/iio/adc/ad7380.c

It seems to claim a kind of a "direct" access specifier. Should I use
similar calls to `iio_device_claim_direct()` and
`iio_device_release_direct()` here?

 999
1000 static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev,
u32 reg,
1001                                      u32 writeval, u32 *readval)
1002 {
1003         struct ad7380_state *st = iio_priv(indio_dev);
1004         int ret;
1005
1006         if (!iio_device_claim_direct(indio_dev))
1007                 return -EBUSY;
1008
1009         if (readval)
1010                 ret = regmap_read(st->regmap, reg, readval);
1011         else
1012                 ret = regmap_write(st->regmap, reg, writeval);
1013
1014         iio_device_release_direct(indio_dev);
1015
1016         return ret;
1017 }
1018

>
> Put the all in from the start.
>

I guess, in the ADXL313 I'm doing the same approach as for the
ADXL345. If it's wrong / incomplete here, it will need to be fixed in
the ADXL345 as well. Or did I understand something wrong?

> Jonathan
>
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl313.h      |  2 ++
> > >  drivers/iio/accel/adxl313_core.c | 17 +++++++++++++++++
> > >  drivers/iio/accel/adxl313_i2c.c  |  6 ++++++
> > >  drivers/iio/accel/adxl313_spi.c  |  6 ++++++
> > >  4 files changed, 31 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> > > index 72f624af4686..fc937bdf83b6 100644
> > > --- a/drivers/iio/accel/adxl313.h
> > > +++ b/drivers/iio/accel/adxl313.h
> > > @@ -54,6 +54,8 @@ extern const struct regmap_access_table adxl312_writable_regs_table;
> > >  extern const struct regmap_access_table adxl313_writable_regs_table;
> > >  extern const struct regmap_access_table adxl314_writable_regs_table;
> > >
> > > +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg);
> > > +
> > >  enum adxl313_device_type {
> > >     ADXL312,
> > >     ADXL313,
> > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > > index 06a771bb4726..0c893c286017 100644
> > > --- a/drivers/iio/accel/adxl313_core.c
> > > +++ b/drivers/iio/accel/adxl313_core.c
> > > @@ -46,6 +46,23 @@ const struct regmap_access_table adxl314_readable_regs_table = {
> > >  };
> > >  EXPORT_SYMBOL_NS_GPL(adxl314_readable_regs_table, IIO_ADXL313);
> > >
> > > +bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> > > +{
> > > +   switch (reg) {
> > > +   case ADXL313_REG_DATA_AXIS(0):
> > > +   case ADXL313_REG_DATA_AXIS(1):
> > > +   case ADXL313_REG_DATA_AXIS(2):
> > > +   case ADXL313_REG_DATA_AXIS(3):
> > > +   case ADXL313_REG_DATA_AXIS(4):
> > > +   case ADXL313_REG_DATA_AXIS(5):
> > > +   case ADXL313_REG_FIFO_STATUS:
> > > +           return true;
> > > +   default:
> > > +           return false;
> > > +   }
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(adxl313_is_volatile_reg, "IIO_ADXL313");
> > > +
> > >  static int adxl312_check_id(struct device *dev,
> > >                         struct adxl313_data *data)
> > >  {
> > > diff --git a/drivers/iio/accel/adxl313_i2c.c b/drivers/iio/accel/adxl313_i2c.c
> > > index a4cf0cf2c5aa..e8636e8ab14f 100644
> > > --- a/drivers/iio/accel/adxl313_i2c.c
> > > +++ b/drivers/iio/accel/adxl313_i2c.c
> > > @@ -21,6 +21,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> > >             .rd_table       = &adxl312_readable_regs_table,
> > >             .wr_table       = &adxl312_writable_regs_table,
> > >             .max_register   = 0x39,
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL313] = {
> > >             .reg_bits       = 8,
> > > @@ -28,6 +30,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> > >             .rd_table       = &adxl313_readable_regs_table,
> > >             .wr_table       = &adxl313_writable_regs_table,
> > >             .max_register   = 0x39,
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL314] = {
> > >             .reg_bits       = 8,
> > > @@ -35,6 +39,8 @@ static const struct regmap_config adxl31x_i2c_regmap_config[] = {
> > >             .rd_table       = &adxl314_readable_regs_table,
> > >             .wr_table       = &adxl314_writable_regs_table,
> > >             .max_register   = 0x39,
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >  };
> > >
> > > diff --git a/drivers/iio/accel/adxl313_spi.c b/drivers/iio/accel/adxl313_spi.c
> > > index 9a16b40bff34..68e323e81aeb 100644
> > > --- a/drivers/iio/accel/adxl313_spi.c
> > > +++ b/drivers/iio/accel/adxl313_spi.c
> > > @@ -24,6 +24,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> > >             .max_register   = 0x39,
> > >             /* Setting bits 7 and 6 enables multiple-byte read */
> > >             .read_flag_mask = BIT(7) | BIT(6),
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL313] = {
> > >             .reg_bits       = 8,
> > > @@ -33,6 +35,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> > >             .max_register   = 0x39,
> > >             /* Setting bits 7 and 6 enables multiple-byte read */
> > >             .read_flag_mask = BIT(7) | BIT(6),
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >     [ADXL314] = {
> > >             .reg_bits       = 8,
> > > @@ -42,6 +46,8 @@ static const struct regmap_config adxl31x_spi_regmap_config[] = {
> > >             .max_register   = 0x39,
> > >             /* Setting bits 7 and 6 enables multiple-byte read */
> > >             .read_flag_mask = BIT(7) | BIT(6),
> > > +           .volatile_reg   = adxl313_is_volatile_reg,
> > > +           .cache_type     = REGCACHE_MAPLE,
> > >     },
> > >  };
> > >
> >
>