[PATCH 0/2] iio: adxl345: add spi-3wire and refac

[PATCH 0/2] iio: adxl345: add spi-3wire and refac

[Lothar Rubusch]

The patch adds the spi-3wire feature and some refactoring to
the adxl345/375 accelerometer driver (iio). A separate patch
updates the DT binding.

Lothar Rubusch (2):
  iio: adxl345: add spi-3wire
  iio: adxl345: update documentation for spi-3wire

 .../bindings/iio/accel/adi,adxl345.yaml       |   2 +
 drivers/iio/accel/adxl345.h                   |  44 ++++++-
 drivers/iio/accel/adxl345_core.c              | 116 ++++++++++--------
 drivers/iio/accel/adxl345_i2c.c               |  30 ++---
 drivers/iio/accel/adxl345_spi.c               |  50 +++++---
 5 files changed, 155 insertions(+), 87 deletions(-)

-- 
2.25.1

[PATCH 1/2] iio: adxl345: add spi-3wire

[Lothar Rubusch]

Adds the spi-3wire feature and adds general refactoring to the
iio driver.

The patch moves driver wide constants and fields into the
header. Thereby reduces redundant info struct definitions.
Allows to pass a function pointer from SPI/I2C specific probe,
and smaller refactorings. A regmap_update_bits() in the core
file replaces the regmap_write() to format_data.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      |  44 +++++++++++-
 drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++--------------
 drivers/iio/accel/adxl345_i2c.c  |  30 ++++----
 drivers/iio/accel/adxl345_spi.c  |  50 ++++++++-----
 4 files changed, 153 insertions(+), 87 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..3c0e14b41 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,61 @@ 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
+ * @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..e456b61c6 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -20,48 +20,62 @@ 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);
+	int ret;
+
+	if (spi->mode & SPI_3WIRE) {
+		ret = regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
+				   ADXL345_DATA_FORMAT_SPI);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 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 = (const struct adxl345_chip_info *) spi_get_device_id(spi)->driver_data;
+
 	/* 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",
 				     spi->max_speed_hz);
 
 	regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config);
-	if (IS_ERR(regmap))
-		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
+	if (IS_ERR(regmap)) {
+		dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing spi regmap: %ld\n",
+			      PTR_ERR(regmap));
+		return PTR_ERR(regmap);
+	}
 
-	return adxl345_core_probe(&spi->dev, regmap);
+	return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup);
 }
 
-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

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Krzysztof Kozlowski]

On 19/03/2024 22:27, Lothar Rubusch wrote:
> Adds the spi-3wire feature and adds general refactoring to the

Add

> iio driver.
> 
> The patch moves driver wide constants and fields into the

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> header. Thereby reduces redundant info struct definitions.
> Allows to pass a function pointer from SPI/I2C specific probe,
> and smaller refactorings. A regmap_update_bits() in the core
> file replaces the regmap_write() to format_data.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---



>  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 = (const struct adxl345_chip_info *) spi_get_device_id(spi)->driver_data;

Are you sure you need the cast?

And why aren't you using spi_get_device_match_data()?

> +
>  	/* 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",
>  				     spi->max_speed_hz);
>  
>  	regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config);
> -	if (IS_ERR(regmap))
> -		return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> +	if (IS_ERR(regmap)) {
> +		dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing spi regmap: %ld\n",
> +			      PTR_ERR(regmap));
> +		return PTR_ERR(regmap);

Why are you changing correct code into incorrect?

> +	}
>  
> -	return adxl345_core_probe(&spi->dev, regmap);
> +	return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup);
>  }


Best regards,
Krzysztof

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Lothar Rubusch]

On Wed, Mar 20, 2024 at 10:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/03/2024 22:27, Lothar Rubusch wrote:
> > Adds the spi-3wire feature and adds general refactoring to the
>
> Add
>
> > iio driver.
> >
> > The patch moves driver wide constants and fields into the
>
> Please do not use "This commit/patch/change", but imperative mood. See
> longer explanation here:
> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
>
> > header. Thereby reduces redundant info struct definitions.
> > Allows to pass a function pointer from SPI/I2C specific probe,
> > and smaller refactorings. A regmap_update_bits() in the core
> > file replaces the regmap_write() to format_data.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---

Agree

> >  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 = (const struct adxl345_chip_info *) spi_get_device_id(spi)->driver_data;
>
> Are you sure you need the cast?
>
> And why aren't you using spi_get_device_match_data()?

Agree

> > +
> >       /* 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",
> >                                    spi->max_speed_hz);
> >
> >       regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config);
> > -     if (IS_ERR(regmap))
> > -             return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> > +     if (IS_ERR(regmap)) {
> > +             dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing spi regmap: %ld\n",
> > +                           PTR_ERR(regmap));
> > +             return PTR_ERR(regmap);
>
> Why are you changing correct code into incorrect?

I'll adjust that. It looks odd, I agree, but is this incorrect code? I found
similar code in the neighbor adxl313 accel driver. I may change/update
that then, too. Thank   you for the hints.

adxl313_i2c.c
72        if (IS_ERR(regmap)) {
73                dev_err(&client->dev, "Error initializing i2c regmap: %ld\n",
74                        PTR_ERR(regmap));
75                return PTR_ERR(regmap);
76        }

> > +     }
> >
> > -     return adxl345_core_probe(&spi->dev, regmap);
> > +     return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup);
> >  }
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Krzysztof Kozlowski]

On 22/03/2024 01:32, Lothar Rubusch wrote:
> 
>>> +
>>>       /* 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",
>>>                                    spi->max_speed_hz);
>>>
>>>       regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config);
>>> -     if (IS_ERR(regmap))
>>> -             return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
>>> +     if (IS_ERR(regmap)) {
>>> +             dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing spi regmap: %ld\n",
>>> +                           PTR_ERR(regmap));
>>> +             return PTR_ERR(regmap);
>>
>> Why are you changing correct code into incorrect?
> 
> I'll adjust that. It looks odd, I agree, but is this incorrect code? I found
> similar code in the neighbor adxl313 accel driver. I may change/update
> that then, too. Thank   you for the hints.

Please explain why you are doing this. How is this related to adding SPI
3-wire mode?


Best regards,
Krzysztof

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Nuno Sá]

On Fri, 2024-03-22 at 06:37 +0100, Krzysztof Kozlowski wrote:
> On 22/03/2024 01:32, Lothar Rubusch wrote:
> > 
> > > > +
> > > >       /* 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",
> > > >                                    spi->max_speed_hz);
> > > > 
> > > >       regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config);
> > > > -     if (IS_ERR(regmap))
> > > > -             return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error
> > > > initializing regmap\n");
> > > > +     if (IS_ERR(regmap)) {
> > > > +             dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing
> > > > spi regmap: %ld\n",
> > > > +                           PTR_ERR(regmap));
> > > > +             return PTR_ERR(regmap);
> > > 
> > > Why are you changing correct code into incorrect?
> > 
> > I'll adjust that. It looks odd, I agree, but is this incorrect code? I found
> > similar code in the neighbor adxl313 accel driver. I may change/update
> > that then, too. Thank   you for the hints.
> 
> Please explain why you are doing this. How is this related to adding SPI
> 3-wire mode?
> 
> 

Yeah... Already asked to separate the refactor from the feature in two different
patches.

- Nuno Sá

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Jonathan Cameron]

On Fri, 22 Mar 2024 01:32:11 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Wed, Mar 20, 2024 at 10:37 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 19/03/2024 22:27, Lothar Rubusch wrote:  
> > > Adds the spi-3wire feature and adds general refactoring to the  
> >
> > Add
> >  
> > > iio driver.
> > >
> > > The patch moves driver wide constants and fields into the  
> >
> > Please do not use "This commit/patch/change", but imperative mood. See
> > longer explanation here:
> > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95
> >  
> > > header. Thereby reduces redundant info struct definitions.
> > > Allows to pass a function pointer from SPI/I2C specific probe,
> > > and smaller refactorings. A regmap_update_bits() in the core
> > > file replaces the regmap_write() to format_data.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---  
> 
> Agree
> 
> > >  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 = (const struct adxl345_chip_info *) spi_get_device_id(spi)->driver_data;  
> >
> > Are you sure you need the cast?
> >
> > And why aren't you using spi_get_device_match_data()?  
> 
> Agree
> 
> > > +
> > >       /* 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",
> > >                                    spi->max_speed_hz);
> > >
> > >       regmap = devm_regmap_init_spi(spi, &adxl345_spi_regmap_config);
> > > -     if (IS_ERR(regmap))
> > > -             return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> > > +     if (IS_ERR(regmap)) {
> > > +             dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing spi regmap: %ld\n",
> > > +                           PTR_ERR(regmap));
> > > +             return PTR_ERR(regmap);  
> >
> > Why are you changing correct code into incorrect?  
> 
> I'll adjust that. It looks odd, I agree, but is this incorrect code? I found
> similar code in the neighbor adxl313 accel driver. I may change/update
> that then, too. Thank   you for the hints.
> 
> adxl313_i2c.c
> 72        if (IS_ERR(regmap)) {
> 73                dev_err(&client->dev, "Error initializing i2c regmap: %ld\n",
> 74                        PTR_ERR(regmap));
> 75                return PTR_ERR(regmap);
> 76        }

dev_err_probe() already pretty prints the error so no point in repeating it.
It also returns the error value so you can do it on one line. dev_err()
does neither of these things.

Likely that axl313_i2c.c could be converted to use dev_err_probe()
for all calls in the probe() function and anything only called as part of that.

Patches welcome.

Jonathan


> 
> > > +     }
> > >
> > > -     return adxl345_core_probe(&spi->dev, regmap);
> > > +     return adxl345_core_probe(&spi->dev, regmap, chip_data, &adxl345_spi_setup);
> > >  }  
> >
> >
> > Best regards,
> > Krzysztof
> >  

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Nuno Sá]

On Tue, 2024-03-19 at 21:27 +0000, Lothar Rubusch wrote:
> Adds the spi-3wire feature and adds general refactoring to the
> iio driver.
> 
> The patch moves driver wide constants and fields into the
> header. Thereby reduces redundant info struct definitions.
> Allows to pass a function pointer from SPI/I2C specific probe,
> and smaller refactorings. A regmap_update_bits() in the core
> file replaces the regmap_write() to format_data.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---

On top of what Krzysztof already said I would also like for you to split the 
spi-3wire (which is adding a new feature) from the refactor in two different
patches. One more comment inline...

>  drivers/iio/accel/adxl345.h      |  44 +++++++++++-
>  drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++--------------
>  drivers/iio/accel/adxl345_i2c.c  |  30 ++++----
>  drivers/iio/accel/adxl345_spi.c  |  50 ++++++++-----
>  4 files changed, 153 insertions(+), 87 deletions(-)
> 

...

> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 93ca349f1..e456b61c6 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -20,48 +20,62 @@ 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);
> +	int ret;
> +
> +	if (spi->mode & SPI_3WIRE) {
> +		ret = regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> +				   ADXL345_DATA_FORMAT_SPI);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;

I think this would be neater:

if (!(spi->mode & SPI_3WIRE))
	return 0;

return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
		    ADXL345_DATA_FORMAT_SPI);

- Nuno Sá

Re: [PATCH 1/2] iio: adxl345: add spi-3wire

[Lothar Rubusch]

On Wed, Mar 20, 2024 at 11:30 AM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Tue, 2024-03-19 at 21:27 +0000, Lothar Rubusch wrote:
> > Adds the spi-3wire feature and adds general refactoring to the
> > iio driver.
> >
> > The patch moves driver wide constants and fields into the
> > header. Thereby reduces redundant info struct definitions.
> > Allows to pass a function pointer from SPI/I2C specific probe,
> > and smaller refactorings. A regmap_update_bits() in the core
> > file replaces the regmap_write() to format_data.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
>
> On top of what Krzysztof already said I would also like for you to split the
> spi-3wire (which is adding a new feature) from the refactor in two different
> patches. One more comment inline...

Agree

> >  drivers/iio/accel/adxl345.h      |  44 +++++++++++-
> >  drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++--------------
> >  drivers/iio/accel/adxl345_i2c.c  |  30 ++++----
> >  drivers/iio/accel/adxl345_spi.c  |  50 ++++++++-----
> >  4 files changed, 153 insertions(+), 87 deletions(-)
> >
>
> ...
>
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > index 93ca349f1..e456b61c6 100644
> > --- a/drivers/iio/accel/adxl345_spi.c
> > +++ b/drivers/iio/accel/adxl345_spi.c
> > @@ -20,48 +20,62 @@ 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);
> > +     int ret;
> > +
> > +     if (spi->mode & SPI_3WIRE) {
> > +             ret = regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
> > +                                ADXL345_DATA_FORMAT_SPI);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
>
> I think this would be neater:
>
> if (!(spi->mode & SPI_3WIRE))
>         return 0;
>
> return regmap_write(regmap, ADXL345_REG_DATA_FORMAT,
>                     ADXL345_DATA_FORMAT_SPI);

Definitely. Thank you.

Best,
L

[PATCH 2/2] iio: adxl345: update documentation for spi-3wire

[Lothar Rubusch]

Provide the optional spi-3wire option for the DT binding.

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 2/2] iio: adxl345: update documentation for spi-3wire

[Krzysztof Kozlowski]

On 19/03/2024 22:27, Lothar Rubusch wrote:
> Provide the optional spi-3wire option for the DT binding.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

A nit, subject: drop second/last, redundant "documentation". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

Also, everything is an update. Be descriptive.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

Re: [PATCH 2/2] iio: adxl345: update documentation for spi-3wire

[Lothar Rubusch]

On Wed, Mar 20, 2024 at 10:34 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 19/03/2024 22:27, Lothar Rubusch wrote:
> > Provide the optional spi-3wire option for the DT binding.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> A nit, subject: drop second/last, redundant "documentation". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Also, everything is an update. Be descriptive.

Ok

> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching.

Yes. Next time I'll chose better prefixes. For now I keep the subject
to the mail to not break the mailing thread and update the patches in a
follow up. I hope this is ok?

> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC. It might happen, that command when run on an older
> kernel, gives you outdated entries. Therefore please be sure you base
> your patches on recent Linux kernel.
>
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of
> people, so fix your workflow. Tools might also fail if you work on some
> ancient tree (don't, instead use mainline), work on fork of kernel
> (don't, instead use mainline) or you ignore some maintainers (really
> don't). Just use b4 and everything should be fine, although remember
> about `b4 prep --auto-to-cc` if you added new patches to the patchset.
>
> You missed at least devicetree list (maybe more), so this won't be
> tested by automated tooling. Performing review on untested code might be
> a waste of time, thus I will skip this patch entirely till you follow
> the process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.
>
> Best regards,
> Krzysztof

I understand. I took linux-next. I fetched it this week from mainline
kernel.org. I took "get_maintainers.pl" as a mere
recommendation rather than mandatory and skipped most of the emails.
Sry for that. Thank you  for all the information.

Re: [PATCH 2/2] iio: adxl345: update documentation for spi-3wire

[Krzysztof Kozlowski]

On 22/03/2024 01:27, Lothar Rubusch wrote:
> 
>> Please use subject prefixes matching the subsystem. You can get them for
>> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
>> your patch is touching.
> 
> Yes. Next time I'll chose better prefixes. For now I keep the subject
> to the mail to not break the mailing thread and update the patches in a
> follow up. I hope this is ok?

No. Email threading depends on reply-to headers, not subject. It's
irrelevant.


Best regards,
Krzysztof