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

[Jonathan Cameron]

On Wed, 11 Jun 2025 15:48:25 +0200
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> On Sun, Jun 8, 2025 at 5:38 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Sun, 8 Jun 2025 16:22:15 +0100
> > Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >  
> > > On Sun,  1 Jun 2025 17:21:31 +0000
> > > Lothar Rubusch <l.rubusch@xxxxxxxxx> 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