On Fri, 2024-03-22 at 06:53 +0100, Krzysztof Kozlowski wrote:
> On 22/03/2024 01:37, Lothar Rubusch wrote:
> > Move driver wide constants and fields into the header.
>
> Why?
>
> > Let probe call a separate setup function. Provide
>
> Why?
>
> > possibility for an SPI/I2C specific setup to be passed
> > as function pointer to core.
>
> Why?
>
> Your commit message *MUST* explain why you are doing things.
>
> >
> > 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
> > +
> > /*
> > * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> > * in all g ranges.
> > @@ -23,11 +56,20 @@
> > */
> > #define ADXL375_USCALE 480000
> >
> > +enum adxl345_device_type {
> > + ADXL345,
> > + ADXL375,
> > +};
> > +
> > struct adxl345_chip_info {
> > const char *name;
> > int uscale;
> > };
> >
> > -int adxl345_core_probe(struct device *dev, struct regmap *regmap);
> > +extern const struct adxl345_chip_info adxl3x5_chip_info[];
> > +
> > +int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > + const struct adxl345_chip_info *chip_info,
> > + int (*setup)(struct device*, struct regmap*));
>
> Last setup argument is entirely unused. Drop this change, it's not
> related to this patchset. Neither explained.
>
Yeah, you need to make it explicit in the message that this change is in preparation
for a future change (adding the 3-wire spi mode). Otherwise it's natural for
reviewers to make questions about it... Maybe another one that could live in it#s own
patch.
- Nuno Sá
>
