Re: [PATCH v9 05/11] iio: accel: adxl345: simplify reading the FIFO

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 16 Jun 2025 00:14:47 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Hi Jonathan,
> 
> On Sat, Jun 14, 2025 at 3:47 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Tue, 10 Jun 2025 21:59:27 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Bulk reading from the FIFO can be simplified. Remove unnecessary
> > > variables and simplify reading sets of x-, y- and z-axis measurements.
> > >
> > > This is a refactoring change and should not impact functionality.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345_core.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 18c625d323ba..dcfbfe4cac0f 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -884,15 +884,13 @@ static int adxl345_get_samples(struct adxl345_state *st)
> > >   */
> > >  static int adxl345_fifo_transfer(struct adxl345_state *st, int samples)
> > >  {
> > > -     size_t count;
> > >       int i, ret = 0;
> > >
> > > -     /* count is the 3x the fifo_buf element size, hence 6B */
> > > -     count = sizeof(st->fifo_buf[0]) * ADXL345_DIRS;
> > >       for (i = 0; i < samples; i++) {
> > >               /* read 3x 2 byte elements from base address into next fifo_buf position */
> > >               ret = regmap_bulk_read(st->regmap, ADXL345_REG_XYZ_BASE,
> > > -                                    st->fifo_buf + (i * count / 2), count);
> > > +                                    st->fifo_buf + (i * ADXL345_DIRS),
> > > +                                    2 * ADXL345_DIRS);  
> >                                        sizeof(st->fifo_buf[0]) * ADXL345_DIRS);
> >
> > As it's not locally obvious where that 2 comes from.  
> 
> Hm, I left the comment some lines above: "read 3x 2 byte elements from
> base address into next fifo_buf position" - so, the 2 comes from the 2
> bytes. 3x because of the 3-axis sensor, so 3 axis times 2 bytes. How
> can I improve it? The patch is just a simplification, removing that
> unnecessary count variable.

 sizeof(st->fifo_buf[0]) * ADXL345_DIRS);

improves it by making it clear where the 2 comes from.
Getting rid of count is good, but that remaining 2 * 
should be replaced by using sizeof() the buffer element.

The comment would then be unnecessary as we are sizing it directly from
the fifo storage.  Fine to leave it if you like though.

Jonathan


> 
> >  
> > >               if (ret)
> > >                       return ret;
> > >  
> >