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

[Nuno Sá]

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á 
>