Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345

From: Lothar Rubusch
Date: Sat Mar 23 2024 - 08:17:46 EST


(...)
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > ---
> > drivers/iio/accel/adxl345.h | 44 +++++++++++-
> > drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++--------------
> > drivers/iio/accel/adxl345_i2c.c | 30 ++++----
> > drivers/iio/accel/adxl345_spi.c | 28 ++++----
> > 4 files changed, 134 insertions(+), 85 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 284bd387c..01493c999 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -8,6 +8,39 @@
> > #ifndef _ADXL345_H_
> > #define _ADXL345_H_
> >
> > +#include <linux/iio/iio.h>
> > +
> > +/* ADXL345 register definitions */
> > +#define ADXL345_REG_DEVID 0x00
> > +#define ADXL345_REG_OFSX 0x1E
> > +#define ADXL345_REG_OFSY 0x1F
> > +#define ADXL345_REG_OFSZ 0x20
> > +#define ADXL345_REG_OFS_AXIS(index) (ADXL345_REG_OFSX + (index))
> > +#define ADXL345_REG_BW_RATE 0x2C
> > +#define ADXL345_REG_POWER_CTL 0x2D
> > +#define ADXL345_REG_DATA_FORMAT 0x31
> > +#define ADXL345_REG_DATAX0 0x32
> > +#define ADXL345_REG_DATAY0 0x34
> > +#define ADXL345_REG_DATAZ0 0x36
> > +#define ADXL345_REG_DATA_AXIS(index) \
> > + (ADXL345_REG_DATAX0 + (index) * sizeof(__le16))
> > +
> > +#define ADXL345_BW_RATE GENMASK(3, 0)
> > +#define ADXL345_BASE_RATE_NANO_HZ 97656250LL
> > +
> > +#define ADXL345_POWER_CTL_MEASURE BIT(3)
> > +#define ADXL345_POWER_CTL_STANDBY 0x00
> > +
> > +#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> > +#define ADXL345_DATA_FORMAT_SPI BIT(6) /* spi-3wire */
> > +#define ADXL345_DATA_FORMAT_2G 0
> > +#define ADXL345_DATA_FORMAT_4G 1
> > +#define ADXL345_DATA_FORMAT_8G 2
> > +#define ADXL345_DATA_FORMAT_16G 3
> > +#define ADXL345_DATA_FORMAT_MSK ~((u8) BIT(6)) /* ignore spi-3wire */
> > +
> > +#define ADXL345_DEVID 0xE5
> > +
(...)

I think I see your point. My patch has more noise and lacks a logic
structure in proceding.
I will resubmit, but may I ask one question in particular. I moved the
entire list of register
defines from the adxl345_core.c to the common adxl345.h.
For setting spi-3wire with my approach, only two of those defines are
needed. I think it is
nicer for readability to keep the defines together, though, in a
commonly shared header.
Nevertheless most of the defines are just used locally in the .._core.c
Should I move them for refactory?
I feel there is no reason to move them. On the other hand I see many
drivers keep them in a common header. Hence, is there a best practice
which justifies moving them to a header?