Received-SPF: pass (google.com: domain of linux-kernel+bounces-111052-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <21b8933dc87ce76eb15abd3dbcb6f025f282eedf.camel@gmail.com>
Subject: Re: [PATCH v2 1/3] iio: accel: adxl345: Update adxl345
From: Nuno =?ISO-8859-1?Q?S=E1?= <noname.nuno@gmail.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Lothar Rubusch
	 <l.rubusch@gmail.com>, lars@metafoo.de, Michael.Hennerich@analog.com, 
	jic23@kernel.org, robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org, 
	conor+dt@kernel.org
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, eraretuya@gmail.com
Date: Fri, 22 Mar 2024 08:18:47 +0100
In-Reply-To: <51e3683f-be53-4bb7-a994-ffd05744a745@linaro.org>
References: <20240322003713.6918-1-l.rubusch@gmail.com>
	 <20240322003713.6918-2-l.rubusch@gmail.com>
	 <51e3683f-be53-4bb7-a994-ffd05744a745@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.50.4 (3.50.4-1.fc39) 
Precedence: bulk
MIME-Version: 1.0

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.
>=20
> Why?
>=20
> > Let probe call a separate setup function. Provide
>=20
> Why?
>=20
> > possibility for an SPI/I2C specific setup to be passed
> > as function pointer to core.
>=20
> Why?
>=20
> Your commit message *MUST* explain why you are doing things.
>=20
> >=20
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> > =C2=A0drivers/iio/accel/adxl345.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=
 44 +++++++++++-
> > =C2=A0drivers/iio/accel/adxl345_core.c | 117 +++++++++++++++++---------=
-----
> > =C2=A0drivers/iio/accel/adxl345_i2c.c=C2=A0 |=C2=A0 30 ++++----
> > =C2=A0drivers/iio/accel/adxl345_spi.c=C2=A0 |=C2=A0 28 ++++----
> > =C2=A04 files changed, 134 insertions(+), 85 deletions(-)
> >=20
> > 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 @@
> > =C2=A0#ifndef _ADXL345_H_
> > =C2=A0#define _ADXL345_H_
> > =C2=A0
> > +#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 resolutio=
n */
> > +#define ADXL345_DATA_FORMAT_SPI=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0 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
> > +
> > =C2=A0/*
> > =C2=A0 * In full-resolution mode, scale factor is maintained at ~4 mg/L=
SB
> > =C2=A0 * in all g ranges.
> > @@ -23,11 +56,20 @@
> > =C2=A0 */
> > =C2=A0#define ADXL375_USCALE	480000
> > =C2=A0
> > +enum adxl345_device_type {
> > +	ADXL345,
> > +	ADXL375,
> > +};
> > +
> > =C2=A0struct adxl345_chip_info {
> > =C2=A0	const char *name;
> > =C2=A0	int uscale;
> > =C2=A0};
> > =C2=A0
> > -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,
> > +		=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 const struct adxl345_chip_info =
*chip_info,
> > +		=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 int (*setup)(struct device*, st=
ruct regmap*));
>=20
> Last setup argument is entirely unused. Drop this change, it's not
> related to this patchset. Neither explained.
>=20

Yeah, you need to make it explicit in the message that this change is in pr=
eparation
for a future change (adding the 3-wire spi mode). Otherwise it's natural fo=
r
reviewers to make questions about it... Maybe another one that could live i=
n it#s own
patch.

- Nuno S=C3=A1=20
>=20