| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFXKEHY5SmtTrxy-8AWxGNqkPUAZjitgYDg2pR7acTAt-tFWdQ@mail.gmail.com>
Date: Wed, 11 Jun 2025 15:48:25 +0200
From: Lothar Rubusch <l.rubusch@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org, corbet@....net,
lucas.p.stankus@...il.com, lars@...afoo.de, Michael.Hennerich@...log.com,
bagasdotme@...il.com, linux-iio@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 03/11] iio: accel: adxl313: make use of regmap cache
On Sun, Jun 8, 2025 at 5:38 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sun, 8 Jun 2025 16:22:15 +0100
> Jonathan Cameron <jic23@...nel.org> wrote:
>
> > On Sun, 1 Jun 2025 17:21:31 +0000
> > Lothar Rubusch <l.rubusch@...il.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@...il.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,
> > > },
> > > };
> > >
> >
>
Powered by blists - more mailing lists