[PATCH v6 0/7] iio: accel: adxl345: Add spi-3wire feature

[PATCH v6 0/7] iio: accel: adxl345: Add spi-3wire feature

[Lothar Rubusch]

Pass a function setup() as pointer from SPI/I2C specific modules to the
core module. Implement setup() to pass the spi-3wire bus option, if
declared in the device-tree.

In the core module, then update data_format register configuration bits
instead of overwriting it. The changes allow to remove a data_range field.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
V1 -> V2: Split into spi-3wire and refactoring
V2 -> V3: Split further, focus on needed changesets
V3 -> V4: Drop "Remove single info instances";
          split "Group bus configuration" into separat
          comment patch; reorder patch set
V4 -> V5: Refrase comments; Align comments to 75; rebuild FORMAT_MASK by
          available flags; fix indention
V5 -> V6: Remove FORMAT_MASK by a local variable on call site;
          Refrase comments;
          Remove unneeded include


Lothar Rubusch (7):
  iio: accel: adxl345: Make data_range obsolete
  iio: accel: adxl345: Group bus configuration
  iio: accel: adxl345: Move defines to header
  dt-bindings: iio: accel: adxl345: Add spi-3wire
  iio: accel: adxl345: Pass function pointer to core
  iio: accel: adxl345: Add comment to probe
  iio: accel: adxl345: Add spi-3wire option

 .../bindings/iio/accel/adi,adxl345.yaml       |  2 +
 drivers/iio/accel/adxl345.h                   | 36 +++++++++-
 drivers/iio/accel/adxl345_core.c              | 72 +++++++++----------
 drivers/iio/accel/adxl345_i2c.c               |  2 +-
 drivers/iio/accel/adxl345_spi.c               | 12 +++-
 5 files changed, 84 insertions(+), 40 deletions(-)

-- 
2.25.1

[PATCH v6 1/7] iio: accel: adxl345: Make data_range obsolete

[Lothar Rubusch]

Replace write() data_format by regmap_update_bits() to keep bus specific
pre-configuration which might have happened before on the same register.
The bus specific bits in data_format register then need to be masked out,

Remove the data_range field from the struct adxl345_data, because it is
not used anymore.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345_core.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..f4dec5ff1 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,7 +37,11 @@
 #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_RANGE	GENMASK(1, 0)	/* Set the g range */
+#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
+#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
+
 #define ADXL345_DATA_FORMAT_2G		0
 #define ADXL345_DATA_FORMAT_4G		1
 #define ADXL345_DATA_FORMAT_8G		2
@@ -48,7 +52,6 @@
 struct adxl345_data {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
-	u8 data_range;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -202,6 +205,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 	struct adxl345_data *data;
 	struct iio_dev *indio_dev;
 	u32 regval;
+	unsigned int data_format_mask;
 	int ret;
 
 	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
@@ -218,15 +222,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 
 	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)
+	/*
+	 * Only allow updates of bus independent bits to the data_format
+	 * register. Keep the bus specific pre-configuration.
+	 */
+	data_format_mask = (ADXL345_DATA_FORMAT_RANGE |
+				  ADXL345_DATA_FORMAT_JUSTIFY |
+				  ADXL345_DATA_FORMAT_FULL_RES |
+				  ADXL345_DATA_FORMAT_SELF_TEST);
+
+	/* Enable full-resolution mode */
+	ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+			data_format_mask, ADXL345_DATA_FORMAT_FULL_RES);
+	if (ret)
 		return dev_err_probe(dev, ret, "Failed to set data range\n");
 
 	indio_dev->name = data->info->name;
-- 
2.25.1

[PATCH v6 2/7] iio: accel: adxl345: Group bus configuration

[Lothar Rubusch]

Group the indio_dev initialization and bus configuration for improved
readability.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index f4dec5ff1..78ac6ea54 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -226,6 +226,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 	if (!data->info)
 		return -ENODEV;
 
+	indio_dev->name = data->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);
+
 	/*
 	 * Only allow updates of bus independent bits to the data_format
 	 * register. Keep the bus specific pre-configuration.
@@ -241,12 +247,6 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to set data range\n");
 
-	indio_dev->name = data->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)
-- 
2.25.1

[PATCH v6 3/7] iio: accel: adxl345: Move defines to header

[Lothar Rubusch]

Move defines from core to the header file. Keep the defines block together
in one location.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      | 32 ++++++++++++++++++++++++++++++++
 drivers/iio/accel/adxl345_core.c | 32 --------------------------------
 2 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..732820d34 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,38 @@
 #ifndef _ADXL345_H_
 #define _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_RANGE	GENMASK(1, 0)	/* Set the g range */
+#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
+#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
+
+#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
+
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
  * in all g ranges.
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 78ac6ea54..2d229fa80 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,38 +17,6 @@
 
 #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_RANGE	GENMASK(1, 0)	/* Set the g range */
-#define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
-#define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
-
-#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;
-- 
2.25.1

[PATCH v6 4/7] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Lothar Rubusch]

Add spi-3wire because the device allows to be configured for spi 3-wire
communication.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 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

[PATCH v6 5/7] iio: accel: adxl345: Pass function pointer to core

[Lothar Rubusch]

Provide a way for bus specific pre-configuration by adding a function
pointer argument to the driver core's probe() function, and keep
the driver core implementation bus independent.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 drivers/iio/accel/adxl345.h      |  3 ++-
 drivers/iio/accel/adxl345_core.c | 10 +++++++++-
 drivers/iio/accel/adxl345_i2c.c  |  2 +-
 drivers/iio/accel/adxl345_spi.c  |  2 +-
 4 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 732820d34..e859c01d4 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -60,6 +60,7 @@ struct adxl345_chip_info {
 	int uscale;
 };
 
-int adxl345_core_probe(struct device *dev, struct regmap *regmap);
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       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 2d229fa80..83d7e7d4e 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,7 +168,8 @@ 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)
+int adxl345_core_probe(struct device *dev, struct regmap *regmap,
+		       int (*setup)(struct device*, struct regmap*))
 {
 	struct adxl345_data *data;
 	struct iio_dev *indio_dev;
@@ -176,6 +177,13 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 	unsigned int data_format_mask;
 	int ret;
 
+	/* Perform optional initial bus specific configuration */
+	if (setup) {
+		ret = setup(dev, regmap);
+		if (ret)
+			return ret;
+	}
+
 	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Error reading device ID\n");
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index a3084b0a8..4065b8f7c 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
 	if (IS_ERR(regmap))
 		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, NULL);
 }
 
 static const struct adxl345_chip_info adxl345_i2c_info = {
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 93ca349f1..1c0513bd3 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -33,7 +33,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);
+	return adxl345_core_probe(&spi->dev, regmap, NULL);
 }
 
 static const struct adxl345_chip_info adxl345_spi_info = {
-- 
2.25.1

[PATCH v6 6/7] iio: accel: adxl345: Add comment to probe

[Lothar Rubusch]

Add a comment to the probe() function to describe the passed function
pointer argument in particular.

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

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 83d7e7d4e..511c27eb3 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,6 +168,16 @@ static void adxl345_powerdown(void *regmap)
 	regmap_write(regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_STANDBY);
 }
 
+/**
+ * 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
+ *
+ * Return: 0 on success, negative errno on error
+ */
 int adxl345_core_probe(struct device *dev, struct regmap *regmap,
 		       int (*setup)(struct device*, struct regmap*))
 {
-- 
2.25.1

[PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option

[Lothar Rubusch]

Add a setup function implementation to the spi module to enable spi-3wire
when specified in the device-tree.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index e859c01d4..3d5c8719d 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -31,6 +31,7 @@
 #define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
 #define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
 #define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)	/* 3-wire SPI mode */
 #define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
 
 #define ADXL345_DATA_FORMAT_2G		0
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 1c0513bd3..f145d5c1d 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_3WIRE);
+	return 0;
+}
+
 static int adxl345_spi_probe(struct spi_device *spi)
 {
 	struct regmap *regmap;
@@ -33,7 +43,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, NULL);
+	return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
 }
 
 static const struct adxl345_chip_info adxl345_spi_info = {
-- 
2.25.1

Re: [PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option

[Jonathan Cameron]

On Fri, 29 Mar 2024 00:40:30 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a setup function implementation to the spi module to enable spi-3wire
> when specified in the device-tree.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h     |  1 +
>  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index e859c01d4..3d5c8719d 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -31,6 +31,7 @@
>  #define ADXL345_DATA_FORMAT_RANGE	GENMASK(1, 0)	/* Set the g range */
>  #define ADXL345_DATA_FORMAT_JUSTIFY	BIT(2)	/* Left-justified (MSB) mode */
>  #define ADXL345_DATA_FORMAT_FULL_RES	BIT(3)	/* Up to 13-bits resolution */
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)	/* 3-wire SPI mode */
>  #define ADXL345_DATA_FORMAT_SELF_TEST	BIT(7)	/* Enable a self test */
>  
>  #define ADXL345_DATA_FORMAT_2G		0
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 1c0513bd3..f145d5c1d 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_3WIRE);
My only remaining comment on this patch set is to add equivalent of
	else
		return regmap_write(regmap, ADXL345_REG_DATA_FORMAT, 0);

If the hardware had some sort of software reset, that was used,
this wouldn't be needed as the status of those other bits would be known.
If we leave them alone in the non 3wire path we may in the future have
subtle issues because some other code left this in an odd state and
we clear those other bits only for 3wire mode.

Jonathan

> +	return 0;
> +}
> +
>  static int adxl345_spi_probe(struct spi_device *spi)
>  {
>  	struct regmap *regmap;
> @@ -33,7 +43,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, NULL);
> +	return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
>  }
>  
>  static const struct adxl345_chip_info adxl345_spi_info = {

Re: [PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option

[Lothar Rubusch]

On Sat, Mar 30, 2024 at 4:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 29 Mar 2024 00:40:30 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a setup function implementation to the spi module to enable spi-3wire
> > when specified in the device-tree.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h     |  1 +
> >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index e859c01d4..3d5c8719d 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -31,6 +31,7 @@
> >  #define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0)   /* Set the g range */
> >  #define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2)  /* Left-justified (MSB) mode */
> >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3)  /* Up to 13-bits resolution */
> > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6)  /* 3-wire SPI mode */
> >  #define ADXL345_DATA_FORMAT_SELF_TEST        BIT(7)  /* Enable a self test */
> >
> >  #define ADXL345_DATA_FORMAT_2G               0
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > index 1c0513bd3..f145d5c1d 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_3WIRE);
> My only remaining comment on this patch set is to add equivalent of
>         else
>                 return regmap_write(regmap, ADXL345_REG_DATA_FORMAT, 0);
>
> If the hardware had some sort of software reset, that was used,
> this wouldn't be needed as the status of those other bits would be known.
> If we leave them alone in the non 3wire path we may in the future have
> subtle issues because some other code left this in an odd state and
> we clear those other bits only for 3wire mode.
>

I see your point. Thinking over it, I came to the following: Given the
spi-3wire case, if I did a regmap_write(spi-3wire), else I did
regmap_write(0), in the i2c case I still passed NULL as setup()
function. So there would still be just a regmap_update() only in the
core module. Furthermore I see three cases: spi_setup() passed w/
3wire, spi_setu() passed w/o 3wire or NULL passed. This means there is
the same issue and more complexity. Hence, I will not do this. I think
I found something else.

What do you think about the following approach: If there is a
spi-3wire set in the device-tree, I pass the setup() function, else I
pass NULL. Then in the core module, if the setup() function is valid,
I do a regmap_update(), else the first option will be set with
regmap_write(). This makes up only two cases: setup() passed, or not -
and in either case the first call will be a regmap_write(). Thus all
bits are initialized to a defined state. I will update the patchset
later today, that you can see.

Happy Easter!
Lothar

> Jonathan
>
> > +     return 0;
> > +}
> > +
> >  static int adxl345_spi_probe(struct spi_device *spi)
> >  {
> >       struct regmap *regmap;
> > @@ -33,7 +43,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, NULL);
> > +     return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> >  }
> >
> >  static const struct adxl345_chip_info adxl345_spi_info = {
>

Re: [PATCH v6 7/7] iio: accel: adxl345: Add spi-3wire option

[Jonathan Cameron]

On Mon, 1 Apr 2024 17:44:22 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Mar 30, 2024 at 4:30 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 29 Mar 2024 00:40:30 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add a setup function implementation to the spi module to enable spi-3wire
> > > when specified in the device-tree.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345.h     |  1 +
> > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > >  2 files changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index e859c01d4..3d5c8719d 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -31,6 +31,7 @@
> > >  #define ADXL345_DATA_FORMAT_RANGE    GENMASK(1, 0)   /* Set the g range */
> > >  #define ADXL345_DATA_FORMAT_JUSTIFY  BIT(2)  /* Left-justified (MSB) mode */
> > >  #define ADXL345_DATA_FORMAT_FULL_RES BIT(3)  /* Up to 13-bits resolution */
> > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6)  /* 3-wire SPI mode */
> > >  #define ADXL345_DATA_FORMAT_SELF_TEST        BIT(7)  /* Enable a self test */
> > >
> > >  #define ADXL345_DATA_FORMAT_2G               0
> > > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > > index 1c0513bd3..f145d5c1d 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_3WIRE);  
> > My only remaining comment on this patch set is to add equivalent of
> >         else
> >                 return regmap_write(regmap, ADXL345_REG_DATA_FORMAT, 0);
> >
> > If the hardware had some sort of software reset, that was used,
> > this wouldn't be needed as the status of those other bits would be known.
> > If we leave them alone in the non 3wire path we may in the future have
> > subtle issues because some other code left this in an odd state and
> > we clear those other bits only for 3wire mode.
> >  
> 
> I see your point. Thinking over it, I came to the following: Given the
> spi-3wire case, if I did a regmap_write(spi-3wire), else I did
> regmap_write(0), in the i2c case I still passed NULL as setup()
> function. So there would still be just a regmap_update() only in the
> core module. Furthermore I see three cases: spi_setup() passed w/
> 3wire, spi_setu() passed w/o 3wire or NULL passed. This means there is
> the same issue and more complexity. Hence, I will not do this. I think
> I found something else.

Good point. I'd forgotten the call was in an optional callback.

> 
> What do you think about the following approach: If there is a
> spi-3wire set in the device-tree, I pass the setup() function, else I
> pass NULL. Then in the core module, if the setup() function is valid,
> I do a regmap_update(), else the first option will be set with

regmap_update()?

> regmap_write(). This makes up only two cases: setup() passed, or not -
> and in either case the first call will be a regmap_write(). Thus all
> bits are initialized to a defined state. I will update the patchset
> later today, that you can see.
That sounds like a good solution.

Jonathan

> 
> Happy Easter!
> Lothar
> 
> > Jonathan
> >  
> > > +     return 0;
> > > +}
> > > +
> > >  static int adxl345_spi_probe(struct spi_device *spi)
> > >  {
> > >       struct regmap *regmap;
> > > @@ -33,7 +43,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, NULL);
> > > +     return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > >  }
> > >
> > >  static const struct adxl345_chip_info adxl345_spi_info = {  
> >  
>