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.
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,
},
};
--
2.39.5On 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."
>
> 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,
> },
> };
>
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.
You added debug accesses in previous patch which will not take the volatile
nature into account unless the register is in that switch statement.
Put the all in from the start.
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,
> > },
> > };
> >
>
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,
> > > },
> > > };
> > >
> >
>
On Wed, 11 Jun 2025 15:48:25 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:
> 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?
>
Yes + any others that we aren't using at all yet.
> 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
It only matters if regcache is involved. If you don't mark
all the registers volatile + provide debugfs access to them
then only the first read will reach the device. The result
of that will be stored in cache and served up for future
use of the debug interface (rather than the updated value).
>
> 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?
Generally we only do that if simply accessing the register is enough
to break comms if done incorrectly. That's normally only on devices
where a mode switch is involved where a device transitions from
register access mode to streaming mode and we don't want a simple
debug read to flip it back again (as that would be a major state
change and rather defeat the point of debug access).
Note sure if that's true for that particular part or not though
(I didn't look).
>
> 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?
No there is generally no need to prevent debug access just because
buffered mode is in use. It is possible for someone foolishly
misusing the write to break things of course, but if we remove
the foot gun then the debug interfaces aren't useful in what is
normally our most high performance mode and one we may well be in
when we want to poke the state.
I'm not personally a big fan of any debug interfaces at all in
production drivers, but we've had them a long time so that ship
sailed.
Jonathan
On Sun, Jun 1, 2025 at 8:21 PM 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 interrupt interrupt > handling, to generate activity and inactivity events. -- With Best Regards, Andy Shevchenko
© 2016 - 2026 Red Hat, Inc.