[PATCH v2 0/3] iio: adxl345: add spi-3wire

[PATCH v2 0/3] iio: adxl345: add spi-3wire

[Lothar Rubusch]

Move driver wide constants and fields into the header. Reduce
redundant info struct definitions. Allow to pass a function
pointer from SPI/I2C specific probe, and smaller refactorings.
Apply regmap_update_bits() in the core file replaces the
regmap_write() to format_data.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
V1 -> V2: split into spi-3wire and refactoring

Lothar Rubusch (3):
  iio: accel: adxl345: Update adxl345
  iio: accel: adxl345: Add spi-3wire feature
  dt-bindings: iio: accel: adxl345: Add spi-3wire

 .../bindings/iio/accel/adi,adxl345.yaml       |   2 +
 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               |  38 +++---
 5 files changed, 146 insertions(+), 85 deletions(-)

-- 
2.25.1

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

[Lothar Rubusch]

Move driver wide constants and fields into the header.
Let probe call a separate setup function. Provide
possibility for an SPI/I2C specific setup to be passed
as function pointer to core.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 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*));
 
 #endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..040c3f05a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,38 +17,9 @@
 
 #include "adxl345.h"
 
-#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_2G		0
-#define ADXL345_DATA_FORMAT_4G		1
-#define ADXL345_DATA_FORMAT_8G		2
-#define ADXL345_DATA_FORMAT_16G		3
-
-#define ADXL345_DEVID			0xE5
-
 struct adxl345_data {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
-	u8 data_range;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -62,6 +33,18 @@ struct adxl345_data {
 		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
 }
 
+const struct adxl345_chip_info adxl3x5_chip_info[] = {
+	[ADXL345] = {
+		.name = "adxl345",
+		.uscale = ADXL345_USCALE,
+	},
+	[ADXL375] = {
+		.name = "adxl375",
+		.uscale = ADXL375_USCALE,
+	},
+};
+EXPORT_SYMBOL_NS_GPL(adxl3x5_chip_info, IIO_ADXL345);
+
 static const struct iio_chan_spec adxl345_channels[] = {
 	ADXL345_CHANNEL(0, X),
 	ADXL345_CHANNEL(1, Y),
@@ -197,14 +180,21 @@ static void adxl345_powerdown(void *regmap)
 	regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
 }
 
-int adxl345_core_probe(struct device *dev, struct regmap *regmap)
+static int adxl345_setup(struct device *dev, struct adxl345_data *data,
+			 int (*setup)(struct device*, struct regmap*))
 {
-	struct adxl345_data *data;
-	struct iio_dev *indio_dev;
 	u32 regval;
 	int ret;
 
-	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
+	/* Perform bus specific settings if available */
+	if (setup) {
+		ret = setup(dev, data->regmap);
+		if (ret)
+			return ret;
+	}
+
+	/* Read out DEVID */
+	ret = regmap_read(data->regmap, ADXL345_REG_DEVID, &regval);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Error reading device ID\n");
 
@@ -212,37 +202,62 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 		return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x, expected %x\n",
 				     regval, ADXL345_DEVID);
 
+	/* Update data_format to full-resolution mode */
+	ret = regmap_update_bits(data->regmap, ADXL345_REG_DATA_FORMAT,
+				 ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to update data_format register\n");
+
+	/* Enable measurement mode */
+	ret = adxl345_powerup(data->regmap);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
+
+	ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/**
+ * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
+ *                        also covers the adlx375 accelerometer
+ * @dev:	Driver model representation of the device
+ * @regmap:	Regmap instance for the device
+ * @chip_info:  Structure containing device specific data
+ * @setup:	Setup routine to be executed right before the standard device
+ *		setup, can also be set to NULL if not required
+ *
+ * Return: 0 on success, negative errno on error
+ */
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       const struct adxl345_chip_info *chip_info,
+		       int (*setup)(struct device*, struct regmap*))
+{
+	struct adxl345_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
 	data->regmap = regmap;
-	/* Enable full-resolution mode */
-	data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
-	data->info = device_get_match_data(dev);
-	if (!data->info)
-		return -ENODEV;
-
-	ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
-			   data->data_range);
-	if (ret < 0)
-		return dev_err_probe(dev, ret, "Failed to set data range\n");
+	data->info = chip_info;
 
-	indio_dev->name = data->info->name;
+	indio_dev->name = chip_info->name;
 	indio_dev->info = &adxl345_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->channels = adxl345_channels;
 	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
 
-	/* Enable measurement mode */
-	ret = adxl345_powerup(data->regmap);
-	if (ret < 0)
-		return dev_err_probe(dev, ret, "Failed to enable measurement mode\n");
-
-	ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
-	if (ret < 0)
+	ret = adxl345_setup(dev, data, setup);
+	if (ret) {
+		dev_err(dev, "ADXL345 setup failed\n");
 		return ret;
+	}
 
 	return devm_iio_device_register(dev, indio_dev);
 }
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index a3084b0a8..3f882e2e0 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
 
@@ -21,41 +22,36 @@ static const struct regmap_config adxl345_i2c_regmap_config = {
 
 static int adxl345_i2c_probe(struct i2c_client *client)
 {
+	const struct adxl345_chip_info *chip_data;
 	struct regmap *regmap;
 
+	/* Retrieve device data, i.e. the name, to pass it to the core */
+	chip_data = i2c_get_match_data(client);
+
 	regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
 	if (IS_ERR(regmap))
-		return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
+		return dev_err_probe(&client->dev, PTR_ERR(regmap),
+				     "Error initializing regmap\n");
 
-	return adxl345_core_probe(&client->dev, regmap);
+	return adxl345_core_probe(&client->dev, regmap, chip_data, NULL);
 }
 
-static const struct adxl345_chip_info adxl345_i2c_info = {
-	.name = "adxl345",
-	.uscale = ADXL345_USCALE,
-};
-
-static const struct adxl345_chip_info adxl375_i2c_info = {
-	.name = "adxl375",
-	.uscale = ADXL375_USCALE,
-};
-
 static const struct i2c_device_id adxl345_i2c_id[] = {
-	{ "adxl345", (kernel_ulong_t)&adxl345_i2c_info },
-	{ "adxl375", (kernel_ulong_t)&adxl375_i2c_info },
+	{ "adxl345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
+	{ "adxl375", (kernel_ulong_t)&adxl3x5_chip_info[ADXL375] },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, adxl345_i2c_id);
 
 static const struct of_device_id adxl345_of_match[] = {
-	{ .compatible = "adi,adxl345", .data = &adxl345_i2c_info },
-	{ .compatible = "adi,adxl375", .data = &adxl375_i2c_info },
+	{ .compatible = "adi,adxl345", .data = &adxl3x5_chip_info[ADXL345] },
+	{ .compatible = "adi,adxl375", .data = &adxl3x5_chip_info[ADXL375] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adxl345_of_match);
 
 static const struct acpi_device_id adxl345_acpi_match[] = {
-	{ "ADS0345", (kernel_ulong_t)&adxl345_i2c_info },
+	{ "ADS0345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, adxl345_acpi_match);
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 93ca349f1..c26bac462 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -22,8 +22,14 @@ static const struct regmap_config adxl345_spi_regmap_config = {
 
 static int adxl345_spi_probe(struct spi_device *spi)
 {
+	const struct adxl345_chip_info *chip_data;
 	struct regmap *regmap;
 
+	/* Retrieve device name to pass it as driver specific data */
+	chip_data = device_get_match_data(&spi->dev);
+	if (!chip_data)
+		chip_data = spi_get_device_match_data(spi);
+
 	/* Bail out if max_speed_hz exceeds 5 MHz */
 	if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ)
 		return dev_err_probe(&spi->dev, -EINVAL, "SPI CLK, %d Hz exceeds 5 MHz\n",
@@ -33,35 +39,25 @@ static int adxl345_spi_probe(struct spi_device *spi)
 	if (IS_ERR(regmap))
 		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
-	return adxl345_core_probe(&spi->dev, regmap);
+	return adxl345_core_probe(&spi->dev, regmap, chip_data, NULL);
 }
 
-static const struct adxl345_chip_info adxl345_spi_info = {
-	.name = "adxl345",
-	.uscale = ADXL345_USCALE,
-};
-
-static const struct adxl345_chip_info adxl375_spi_info = {
-	.name = "adxl375",
-	.uscale = ADXL375_USCALE,
-};
-
 static const struct spi_device_id adxl345_spi_id[] = {
-	{ "adxl345", (kernel_ulong_t)&adxl345_spi_info },
-	{ "adxl375", (kernel_ulong_t)&adxl375_spi_info },
+	{ "adxl345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
+	{ "adxl375", (kernel_ulong_t)&adxl3x5_chip_info[ADXL375] },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adxl345_spi_id);
 
 static const struct of_device_id adxl345_of_match[] = {
-	{ .compatible = "adi,adxl345", .data = &adxl345_spi_info },
-	{ .compatible = "adi,adxl375", .data = &adxl375_spi_info },
+	{ .compatible = "adi,adxl345", .data = &adxl3x5_chip_info[ADXL345] },
+	{ .compatible = "adi,adxl375", .data = &adxl3x5_chip_info[ADXL375] },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adxl345_of_match);
 
 static const struct acpi_device_id adxl345_acpi_match[] = {
-	{ "ADS0345", (kernel_ulong_t)&adxl345_spi_info },
+	{ "ADS0345", (kernel_ulong_t)&adxl3x5_chip_info[ADXL345] },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, adxl345_acpi_match);
-- 
2.25.1

[PATCH v2 2/3] iio: accel: adxl345: Add spi-3wire feature

[Lothar Rubusch]

Add the spi-3wire feature to the iio driver. Pass a
function pointer to configure the device if 3-wire
was configured in the device-tree.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index c26bac462..aea368895 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -20,6 +20,16 @@ static const struct regmap_config adxl345_spi_regmap_config = {
 	.read_flag_mask = BIT(7) | BIT(6),
 };
 
+static int adxl345_spi_setup(struct device *dev, struct regmap *regmap)
+{
+	struct spi_device *spi = container_of(dev, struct spi_device, dev);
+
+	if (spi->mode & SPI_3WIRE)
+		return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
+			    ADXL345_DATA_FORMAT_SPI);
+	return 0;
+}
+
 static int adxl345_spi_probe(struct spi_device *spi)
 {
 	const struct adxl345_chip_info *chip_data;
@@ -39,7 +49,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
 	if (IS_ERR(regmap))
 		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
 
-	return adxl345_core_probe(&spi->dev, regmap, chip_data, NULL);
+	return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup);
 }
 
 static const struct spi_device_id adxl345_spi_id[] = {
-- 
2.25.1

[PATCH v2 3/3] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Lothar Rubusch]

Provide the optional spi-3wire in the example.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index 07cacc3f6..280ed479e 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -32,6 +32,8 @@ properties:
 
   spi-cpol: true
 
+  spi-3wire: true
+
   interrupts:
     maxItems: 1
 
-- 
2.25.1

Re: [PATCH v2 3/3] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Rob Herring]

On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
> Provide the optional spi-3wire in the example.

That doesn't match the diff as you don't touch the example. But really, 
this should say why you need spi-3wire.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> index 07cacc3f6..280ed479e 100644
> --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> @@ -32,6 +32,8 @@ properties:
>  
>    spi-cpol: true
>  
> +  spi-3wire: true
> +
>    interrupts:
>      maxItems: 1
>  
> -- 
> 2.25.1
> 

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

[Krzysztof Kozlowski]

On 22/03/2024 01:37, Lothar Rubusch wrote:
> Move driver wide constants and fields into the header.
> Let probe call a separate setup function. Provide
> possibility for an SPI/I2C specific setup to be passed
> as function pointer to core.

Subject: you received feedback already of not calling things "update".
Everything is update.

No, write descriptive text.

If you cannot, means you are doing way too many things in one patch.
Please read submitting-patches document.


> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  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

How is all this related to the patch? I don't understand. Several parts
of this patch are not explained / obvious.


> +
>  /*
>   * 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*));
>  
>  #endif /* _ADXL345_H_ */
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8bd30a23e..040c3f05a 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -17,38 +17,9 @@
>  
>  #include "adxl345.h"
>  
> -#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_2G		0
> -#define ADXL345_DATA_FORMAT_4G		1
> -#define ADXL345_DATA_FORMAT_8G		2
> -#define ADXL345_DATA_FORMAT_16G		3

Why?

...

>  
>  	return devm_iio_device_register(dev, indio_dev);
>  }
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index a3084b0a8..3f882e2e0 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>

One more... how is this related?

>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -21,41 +22,36 @@ static const struct regmap_config adxl345_i2c_regmap_config = {
>  
>  static int adxl345_i2c_probe(struct i2c_client *client)
>  {
> +	const struct adxl345_chip_info *chip_data;
>  	struct regmap *regmap;
>  
> +	/* Retrieve device data, i.e. the name, to pass it to the core */
> +	chip_data = i2c_get_match_data(client);
> +
>  	regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
>  	if (IS_ERR(regmap))
> -		return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> +				     "Error initializing regmap\n");

How is this change related to your commit?

Stop doing unrelated changes.

>  
> -	return adxl345_core_probe(&client->dev, regmap);
> +	return adxl345_core_probe(&client->dev, regmap, chip_data, NULL);
>  }
>  
> -static const struct adxl345_chip_info adxl345_i2c_info = {
> -	.name = "adxl345",
> -	.uscale = ADXL345_USCALE,
> -};

...

>  MODULE_DEVICE_TABLE(acpi, adxl345_acpi_match);
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 93ca349f1..c26bac462 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -22,8 +22,14 @@ static const struct regmap_config adxl345_spi_regmap_config = {
>  
>  static int adxl345_spi_probe(struct spi_device *spi)
>  {
> +	const struct adxl345_chip_info *chip_data;
>  	struct regmap *regmap;
>  
> +	/* Retrieve device name to pass it as driver specific data */
> +	chip_data = device_get_match_data(&spi->dev);
> +	if (!chip_data)
> +		chip_data = spi_get_device_match_data(spi);

That's not how you use it spi_get_device_match_data(). Open the function
and read it... it should be obvious that you now duplicate code.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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@gmail.com>
> ---
>  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.

Best regards,
Krzysztof

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

[Nuno Sá]

On Fri, 2024-03-22 at 00:37 +0000, Lothar Rubusch wrote:
> Move driver wide constants and fields into the header.
> Let probe call a separate setup function. Provide
> possibility for an SPI/I2C specific setup to be passed
> as function pointer to core.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  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*));
>  
>  #endif /* _ADXL345_H_ */
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8bd30a23e..040c3f05a 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -17,38 +17,9 @@
>  
>  #include "adxl345.h"
>  
> -#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_2G		0
> -#define ADXL345_DATA_FORMAT_4G		1
> -#define ADXL345_DATA_FORMAT_8G		2
> -#define ADXL345_DATA_FORMAT_16G		3
> -
> -#define ADXL345_DEVID			0xE5
> -
>  struct adxl345_data {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> -	u8 data_range;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -62,6 +33,18 @@ struct adxl345_data {
>  		BIT(IIO_CHAN_INFO_SAMP_FREQ),				\
>  }
>  
> +const struct adxl345_chip_info adxl3x5_chip_info[] = {
> +	[ADXL345] = {
> +		.name = "adxl345",
> +		.uscale = ADXL345_USCALE,
> +	},
> +	[ADXL375] = {
> +		.name = "adxl375",
> +		.uscale = ADXL375_USCALE,
> +	},
> +};
> +EXPORT_SYMBOL_NS_GPL(adxl3x5_chip_info, IIO_ADXL345);
> +
>  static const struct iio_chan_spec adxl345_channels[] = {
>  	ADXL345_CHANNEL(0, X),
>  	ADXL345_CHANNEL(1, Y),
> @@ -197,14 +180,21 @@ static void adxl345_powerdown(void *regmap)
>  	regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
>  }
>  
> -int adxl345_core_probe(struct device *dev, struct regmap *regmap)
> +static int adxl345_setup(struct device *dev, struct adxl345_data *data,
> +			 int (*setup)(struct device*, struct regmap*))
>  {
> -	struct adxl345_data *data;
> -	struct iio_dev *indio_dev;
>  	u32 regval;
>  	int ret;
>  
> -	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
> +	/* Perform bus specific settings if available */
> +	if (setup) {
> +		ret = setup(dev, data->regmap);
> +		if (ret)
> +			return ret;
> +	}

nit: likely a better name would be bus_setup(). Then you could drop the comment as it
becomes useless...

> +
> +	/* Read out DEVID */
> +	ret = regmap_read(data->regmap, ADXL345_REG_DEVID, &regval);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Error reading device ID\n");
>  
> @@ -212,37 +202,62 @@ int adxl345_core_probe(struct device *dev, struct regmap
> *regmap)
>  		return dev_err_probe(dev, -ENODEV, "Invalid device ID: %x,
> expected %x\n",
>  				     regval, ADXL345_DEVID);
>  
> +	/* Update data_format to full-resolution mode */
> +	ret = regmap_update_bits(data->regmap, ADXL345_REG_DATA_FORMAT,
> +				 ADXL345_DATA_FORMAT_MSK,
> ADXL345_DATA_FORMAT_FULL_RES);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to update data_format
> register\n");
> +
> +	/* Enable measurement mode */
> +	ret = adxl345_powerup(data->regmap);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to enable measurement
> mode\n");
> +
> +	ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +/**
> + * adxl345_core_probe() - probe and setup for the adxl345 accelerometer,
> + *                        also covers the adlx375 accelerometer
> + * @dev:	Driver model representation of the device
> + * @regmap:	Regmap instance for the device
> + * @chip_info:  Structure containing device specific data
> + * @setup:	Setup routine to be executed right before the standard device
> + *		setup, can also be set to NULL if not required
> + *
> + * Return: 0 on success, negative errno on error
> + */
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> +		       const struct adxl345_chip_info *chip_info,
> +		       int (*setup)(struct device*, struct regmap*))
> +{
> +	struct adxl345_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
>  	data = iio_priv(indio_dev);
>  	data->regmap = regmap;
> -	/* Enable full-resolution mode */
> -	data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> -	data->info = device_get_match_data(dev);
> -	if (!data->info)
> -		return -ENODEV;
> -
> -	ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> -			   data->data_range);
> -	if (ret < 0)
> -		return dev_err_probe(dev, ret, "Failed to set data range\n");
> +	data->info = chip_info;
>  
> -	indio_dev->name = data->info->name;
> +	indio_dev->name = chip_info->name;
>  	indio_dev->info = &adxl345_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->channels = adxl345_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>  
> -	/* Enable measurement mode */
> -	ret = adxl345_powerup(data->regmap);
> -	if (ret < 0)
> -		return dev_err_probe(dev, ret, "Failed to enable measurement
> mode\n");
> -
> -	ret = devm_add_action_or_reset(dev, adxl345_powerdown, data->regmap);
> -	if (ret < 0)
> +	ret = adxl345_setup(dev, data, setup);
> +	if (ret) {
> +		dev_err(dev, "ADXL345 setup failed\n");
>  		return ret;
> +	}
>  
>  	return devm_iio_device_register(dev, indio_dev);
>  }
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index a3084b0a8..3f882e2e0 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
>  
> @@ -21,41 +22,36 @@ static const struct regmap_config adxl345_i2c_regmap_config = {
>  
>  static int adxl345_i2c_probe(struct i2c_client *client)
>  {
> +	const struct adxl345_chip_info *chip_data;
>  	struct regmap *regmap;
>  
> +	/* Retrieve device data, i.e. the name, to pass it to the core */
> +	chip_data = i2c_get_match_data(client);
> +

While unlikely use a proper pattern. Meaning, check for NULL pointers and bail out in
that case... Also, you need to justify why are you moving these calls to the bus in
your commit message.


It seems to me that you're trying to refactor too much in a single patch. Maybe step
back and try to separate changes in different patches. Like this one (passing
chip_info) from the bus file could be in it's own patch. Will also (or should at
least :)) "force" you to have a more dedicated commit message explaining why you're
introducing the change.

- Nuno Sá

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@gmail.com>
> > ---
> >  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á 
> 

Re: [PATCH v2 3/3] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Lothar Rubusch]

On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
> > Provide the optional spi-3wire in the example.
>
> That doesn't match the diff as you don't touch the example. But really,
> this should say why you need spi-3wire.

I understand. The change does not add anything to the example. which
is definitely wrong.
Anyway I'm unsure about this change in particular. I know the spi-3wire
binding exists and can be implemented. Not all spi devices offer it. Not all
drivers implement it. My patch set tries to implement spi-3wire for the
particular accelerometer.
Do I need to add something here to dt-bindings documentation of the
adxl345? Or, as an optional spi feature, is it covered anyway by
documentation of optional spi bindings? So, should I refrase this particular
patch or may I drop it entirely? Could you please clarify.

>
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > index 07cacc3f6..280ed479e 100644
> > --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> > @@ -32,6 +32,8 @@ properties:
> >
> >    spi-cpol: true
> >
> > +  spi-3wire: true
> > +
> >    interrupts:
> >      maxItems: 1
> >
> > --
> > 2.25.1
> >

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

[Lothar Rubusch]

(...)
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  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?

Re: [PATCH v2 3/3] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Krzysztof Kozlowski]

On 23/03/2024 13:04, Lothar Rubusch wrote:
> On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote:
>>
>> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
>>> Provide the optional spi-3wire in the example.
>>
>> That doesn't match the diff as you don't touch the example. But really,
>> this should say why you need spi-3wire.
> 
> I understand. The change does not add anything to the example. which
> is definitely wrong.
> Anyway I'm unsure about this change in particular. I know the spi-3wire
> binding exists and can be implemented. Not all spi devices offer it. Not all
> drivers implement it. My patch set tries to implement spi-3wire for the
> particular accelerometer.
> Do I need to add something here to dt-bindings documentation of the
> adxl345? Or, as an optional spi feature, is it covered anyway by
> documentation of optional spi bindings? So, should I refrase this particular
> patch or may I drop it entirely? Could you please clarify.

Whether you need to change bindings or not, dtbs_check will tell you.
Just run dtbs_check on your DTS.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

AFAIR, spi-3wire requires being explicitly mentioned in the device bindings.


Best regards,
Krzysztof

Re: [PATCH v2 3/3] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Lothar Rubusch]

On Sat, Mar 23, 2024 at 3:27 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 23/03/2024 13:04, Lothar Rubusch wrote:
> > On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote:
> >>
> >> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
> >>> Provide the optional spi-3wire in the example.
> >>
> >> That doesn't match the diff as you don't touch the example. But really,
> >> this should say why you need spi-3wire.
> >
> > I understand. The change does not add anything to the example. which
> > is definitely wrong.
> > Anyway I'm unsure about this change in particular. I know the spi-3wire
> > binding exists and can be implemented. Not all spi devices offer it. Not all
> > drivers implement it. My patch set tries to implement spi-3wire for the
> > particular accelerometer.
> > Do I need to add something here to dt-bindings documentation of the
> > adxl345? Or, as an optional spi feature, is it covered anyway by
> > documentation of optional spi bindings? So, should I refrase this particular
> > patch or may I drop it entirely? Could you please clarify.
>
> Whether you need to change bindings or not, dtbs_check will tell you.
> Just run dtbs_check on your DTS.
>

I'm not changing upstream DTS. At most, the documentation should
mention something.

> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
>

No, I didn't. dtbs_check did not work right out of the box, but it
sounds great and I will figure out. Currently my setup is a bit
customized. I compile the modules out of tree, dockerized with several
DTBOs. I use an automized setup to verify spi, spi-3wire and i2c
probing still works on the hardware. It is tested at least somehow.

> AFAIR, spi-3wire requires being explicitly mentioned in the device bindings.
>
>
> Best regards,
> Krzysztof
>

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

[Jonathan Cameron]

On Sat, 23 Mar 2024 13:16:56 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> (...)
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  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?

Move them as a block (which you did).  It's confusing to have only a subset of
defines in one place.

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

Re: [PATCH v2 3/3] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Krzysztof Kozlowski]

On 23/03/2024 18:44, Lothar Rubusch wrote:
> On Sat, Mar 23, 2024 at 3:27 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 23/03/2024 13:04, Lothar Rubusch wrote:
>>> On Fri, Mar 22, 2024 at 3:17 AM Rob Herring <robh@kernel.org> wrote:
>>>>
>>>> On Fri, Mar 22, 2024 at 12:37:13AM +0000, Lothar Rubusch wrote:
>>>>> Provide the optional spi-3wire in the example.
>>>>
>>>> That doesn't match the diff as you don't touch the example. But really,
>>>> this should say why you need spi-3wire.
>>>
>>> I understand. The change does not add anything to the example. which
>>> is definitely wrong.
>>> Anyway I'm unsure about this change in particular. I know the spi-3wire
>>> binding exists and can be implemented. Not all spi devices offer it. Not all
>>> drivers implement it. My patch set tries to implement spi-3wire for the
>>> particular accelerometer.
>>> Do I need to add something here to dt-bindings documentation of the
>>> adxl345? Or, as an optional spi feature, is it covered anyway by
>>> documentation of optional spi bindings? So, should I refrase this particular
>>> patch or may I drop it entirely? Could you please clarify.
>>
>> Whether you need to change bindings or not, dtbs_check will tell you.
>> Just run dtbs_check on your DTS.
>>
> 
> I'm not changing upstream DTS. At most, the documentation should
> mention something.

Nothing should stop you testing from downstream DTS...

Best regards,
Krzysztof