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

[PATCH v5 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

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                   | 42 ++++++++++++-
 drivers/iio/accel/adxl345_core.c              | 62 ++++++++-----------
 drivers/iio/accel/adxl345_i2c.c               |  2 +-
 drivers/iio/accel/adxl345_spi.c               | 12 +++-
 5 files changed, 80 insertions(+), 40 deletions(-)

-- 
2.25.1

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

[Lothar Rubusch]

Replace write() data_format by regmap_update_bits(), because bus specific
pre-configuration may have happened before on the same register. For
further updates to the data_format register then bus pre-configuration
needs 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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..35df5e372 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -37,7 +37,15 @@
 #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_MSK		(ADXL345_DATA_FORMAT_RANGE | \
+					 ADXL345_DATA_FORMAT_JUSTIFY |  \
+					 ADXL345_DATA_FORMAT_FULL_RES | \
+					 ADXL345_DATA_FORMAT_SELF_TEST)
+
 #define ADXL345_DATA_FORMAT_2G		0
 #define ADXL345_DATA_FORMAT_4G		1
 #define ADXL345_DATA_FORMAT_8G		2
@@ -48,7 +56,6 @@
 struct adxl345_data {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
-	u8 data_range;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -218,15 +225,14 @@ 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)
+	/* Enable full-resolution mode */
+	ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+				 ADXL345_DATA_FORMAT_MSK, 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 v5 2/7] iio: accel: adxl345: Group bus configuration

[Lothar Rubusch]

In the probe function group bus configuration and the indio_dev
initialization to improve 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 35df5e372..5c24ef9ca 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -229,18 +229,18 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 	if (!data->info)
 		return -ENODEV;
 
-	/* Enable full-resolution mode */
-	ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
-				 ADXL345_DATA_FORMAT_MSK, 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;
 	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 full-resolution mode */
+	ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
+				 ADXL345_DATA_FORMAT_MSK, ADXL345_DATA_FORMAT_FULL_RES);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set data range\n");
+
 	/* Enable measurement mode */
 	ret = adxl345_powerup(data->regmap);
 	if (ret < 0)
-- 
2.25.1

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

[Lothar Rubusch]

Move defines to the header file. This keeps the defines block together in
one location, when extended by new shared defines which then have to be
in the header file.

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..564f7baf1 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,43 @@
 #ifndef _ADXL345_H_
 #define _ADXL345_H_
 
+#include <linux/iio/iio.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_MSK		(ADXL345_DATA_FORMAT_RANGE |	\
+					 ADXL345_DATA_FORMAT_JUSTIFY |	\
+					 ADXL345_DATA_FORMAT_FULL_RES | \
+					 ADXL345_DATA_FORMAT_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 5c24ef9ca..c7c1156e8 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,42 +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_MSK		(ADXL345_DATA_FORMAT_RANGE | \
-					 ADXL345_DATA_FORMAT_JUSTIFY |  \
-					 ADXL345_DATA_FORMAT_FULL_RES | \
-					 ADXL345_DATA_FORMAT_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 v5 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>
---
 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 v5 5/7] iio: accel: adxl345: Pass function pointer to core

[Lothar Rubusch]

Add a function pointer argument to the probe function in the core module
to provide a way to pre-configure the bus.

The passed setup function can be prepared in the bus specific spi or the
i2c module, or NULL otherwise. It shall then be executed in the bus
independent core module.

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 564f7baf1..4ea9341d4 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -65,6 +65,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 c7c1156e8..40aab7683 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -168,13 +168,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)
+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;
 	u32 regval;
 	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 v5 6/7] iio: accel: adxl345: Add comment to probe

[Lothar Rubusch]

Add a comment to the probe() function to describe the passed function
pointer setup 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 40aab7683..0046e8d19 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 v5 7/7] iio: accel: adxl345: Add spi-3wire option

[Lothar Rubusch]

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

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

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 4ea9341d4..e6bc3591c 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -30,6 +30,8 @@
 #define ADXL345_POWER_CTL_MEASURE	BIT(3)
 #define ADXL345_POWER_CTL_STANDBY	0x00
 
+#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6) /* 3-wire SPI mode */
+
 #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 */
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 v5 4/7] dt-bindings: iio: accel: adxl345: Add spi-3wire

[Krzysztof Kozlowski]

On 27/03/2024 23:03, Lothar Rubusch wrote:
> Add spi-3wire because the device allows to be configured for spi 3-wire
> communication.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

I don't understand why after my last message you still ignore my
feedback, but fine. I'll ignore this patchset.

Best regards,
Krzysztof

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

[Lothar Rubusch]

On Thu, Mar 28, 2024 at 10:22 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 27/03/2024 23:03, Lothar Rubusch wrote:
> > Add spi-3wire because the device allows to be configured for spi 3-wire
> > communication.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> I don't understand why after my last message you still ignore my
> feedback, but fine. I'll ignore this patchset.

This is sad. I refrased the comment to the patch and tried to stress
more on the device instead of the driver. Wasn't this the point of
your criticism to the patch?

So, instead of answering to your last email, I posted the modified
patch as answer. I think I considered your feedback to the best of my
understanding. I'm sorry, it is certainly not my intention to ignore
you. I appreciate your direct feedback.

I'll write an answer to the last email right away.

> Best regards,
> Krzysztof
>

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

[Lothar Rubusch]

On Thu, Mar 28, 2024 at 10:22 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 27/03/2024 23:03, Lothar Rubusch wrote:
> > Add spi-3wire because the device allows to be configured for spi 3-wire
> > communication.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>
> I don't understand why after my last message you still ignore my
> feedback, but fine. I'll ignore this patchset.
>

I'm sorry. I found it now. Please ignore my last v5. I'll repatch an
adjusted v6.

> Best regards,
> Krzysztof
>

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

[Jonathan Cameron]

On Wed, 27 Mar 2024 22:03:14 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Replace write() data_format by regmap_update_bits(), because bus specific
> pre-configuration may have happened before on the same register. For
> further updates to the data_format register then bus pre-configuration
> needs 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 | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8bd30a23e..35df5e372 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -37,7 +37,15 @@
>  #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_MSK		(ADXL345_DATA_FORMAT_RANGE | \
> +					 ADXL345_DATA_FORMAT_JUSTIFY |  \
> +					 ADXL345_DATA_FORMAT_FULL_RES | \
> +					 ADXL345_DATA_FORMAT_SELF_TEST)
This needs renaming.  It's not a mask of everything in the register, or
even just of everything related to format. 

Actually I'd just not have this definition.  Use the build up value
from all the submasks at the call site.  Then we are just making it clear
only a subset of fields are being cleared.

Jonathan

> +
>  #define ADXL345_DATA_FORMAT_2G		0
>  #define ADXL345_DATA_FORMAT_4G		1
>  #define ADXL345_DATA_FORMAT_8G		2
> @@ -48,7 +56,6 @@
>  struct adxl345_data {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> -	u8 data_range;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -218,15 +225,14 @@ 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)
> +	/* Enable full-resolution mode */
> +	ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> +				 ADXL345_DATA_FORMAT_MSK, 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;

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

[Jonathan Cameron]

On Wed, 27 Mar 2024 22:03:20 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add a setup function implementation to the spi module to enable spi-3wire
> as option when specified in the device-tree.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  drivers/iio/accel/adxl345.h     |  2 ++
>  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index 4ea9341d4..e6bc3591c 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -30,6 +30,8 @@
>  #define ADXL345_POWER_CTL_MEASURE	BIT(3)
>  #define ADXL345_POWER_CTL_STANDBY	0x00
>  
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6) /* 3-wire SPI mode */
> +
>  #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 */
> 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);
Your earlier patch carefully (I think) left one or two fields alone, then
this write just comes in and changes them. In particular INT_INVERT.

If it doesn't makes sense to write it there, either write that bit
every time here, or leave it alone every time.  Not decide on whether
to write the bit based on SPI_3WIRE or not.  As far as I know they
are unconnected features.

> +	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 v5 1/7] iio: accel: adxl345: Make data_range obsolete

[Lothar Rubusch]

On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 27 Mar 2024 22:03:14 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Replace write() data_format by regmap_update_bits(), because bus specific
> > pre-configuration may have happened before on the same register. For
> > further updates to the data_format register then bus pre-configuration
> > needs 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 | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 8bd30a23e..35df5e372 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -37,7 +37,15 @@
> >  #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_MSK              (ADXL345_DATA_FORMAT_RANGE | \
> > +                                      ADXL345_DATA_FORMAT_JUSTIFY |  \
> > +                                      ADXL345_DATA_FORMAT_FULL_RES | \
> > +                                      ADXL345_DATA_FORMAT_SELF_TEST)
> This needs renaming.  It's not a mask of everything in the register, or
> even just of everything related to format.
>
> Actually I'd just not have this definition.  Use the build up value
> from all the submasks at the call site.  Then we are just making it clear
> only a subset of fields are being cleared.
>
I understand this solution was not very useful. I'm not sure, I
understood you correctly. Please have a look into v6 if this matches
your comment.
Now, I remove the mask, instead I use a local variable in core's
probe() for the update mask. I added a comment. Nevertheless, I keep
the used flags for FORMAT_DATA. Does this go into the direction of
using the build up value from the submasks at the call site?

> Jonathan
>
> > +
> >  #define ADXL345_DATA_FORMAT_2G               0
> >  #define ADXL345_DATA_FORMAT_4G               1
> >  #define ADXL345_DATA_FORMAT_8G               2
> > @@ -48,7 +56,6 @@
> >  struct adxl345_data {
> >       const struct adxl345_chip_info *info;
> >       struct regmap *regmap;
> > -     u8 data_range;
> >  };
> >
> >  #define ADXL345_CHANNEL(index, axis) {                                       \
> > @@ -218,15 +225,14 @@ 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)
> > +     /* Enable full-resolution mode */
> > +     ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> > +                              ADXL345_DATA_FORMAT_MSK, 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;
>

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

[Lothar Rubusch]

On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 27 Mar 2024 22:03:20 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > Add a setup function implementation to the spi module to enable spi-3wire
> > as option when specified in the device-tree.
> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
> >  drivers/iio/accel/adxl345.h     |  2 ++
> >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 4ea9341d4..e6bc3591c 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -30,6 +30,8 @@
> >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> >  #define ADXL345_POWER_CTL_STANDBY    0x00
> >
> > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > +
> >  #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 */
> > 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);
> Your earlier patch carefully (I think) left one or two fields alone, then
> this write just comes in and changes them. In particular INT_INVERT.
>
Why do you refer here to INT_INVERT? In this code above I try to set
SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
it happens in adxl345_spi.c. Passing this function to the bus
independent adxl345_core.c file allows to configure the bus first.
Therefore, I'm using the update function in core masking out the SPI
filag.

My reason why I try to keep INT_INVERT out is different. There is
another driver for the same part in the kernel:
./drivers/input/misc/adxl34x.c - This is a input driver, using the
interrupts of the adxl345 for the input device implementation. I
assumed, that in the iio implementation there won't be interrupt
handling for the adx345, since it is not an input device. Does this
make sense?

> If it doesn't makes sense to write it there, either write that bit
> every time here, or leave it alone every time.  Not decide on whether
> to write the bit based on SPI_3WIRE or not.  As far as I know they
> are unconnected features.
>
I think I did not understand. Could you please specify a bit more?
When spi-3wire is configured in the DT it has to be parsed and handled
in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
SPI_3WIRE there. I don't want to place SPI specific code in the core
file.

> > +     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 v5 1/7] iio: accel: adxl345: Make data_range obsolete

[Jonathan Cameron]

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

> On Thu, Mar 28, 2024 at 2:37 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 27 Mar 2024 22:03:14 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Replace write() data_format by regmap_update_bits(), because bus specific
> > > pre-configuration may have happened before on the same register. For
> > > further updates to the data_format register then bus pre-configuration
> > > needs 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 | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 8bd30a23e..35df5e372 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -37,7 +37,15 @@
> > >  #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_MSK              (ADXL345_DATA_FORMAT_RANGE | \
> > > +                                      ADXL345_DATA_FORMAT_JUSTIFY |  \
> > > +                                      ADXL345_DATA_FORMAT_FULL_RES | \
> > > +                                      ADXL345_DATA_FORMAT_SELF_TEST)  
> > This needs renaming.  It's not a mask of everything in the register, or
> > even just of everything related to format.
> >
> > Actually I'd just not have this definition.  Use the build up value
> > from all the submasks at the call site.  Then we are just making it clear
> > only a subset of fields are being cleared.
> >  
> I understand this solution was not very useful. I'm not sure, I
> understood you correctly. Please have a look into v6 if this matches
> your comment.
> Now, I remove the mask, instead I use a local variable in core's
> probe() for the update mask. I added a comment. Nevertheless, I keep
> the used flags for FORMAT_DATA. Does this go into the direction of
> using the build up value from the submasks at the call site?
> 
The new code looks good to me.  A local variable doesn't carry the
same implication of global applicability as the define did.
Thanks,

J
> > Jonathan
> >  
> > > +
> > >  #define ADXL345_DATA_FORMAT_2G               0
> > >  #define ADXL345_DATA_FORMAT_4G               1
> > >  #define ADXL345_DATA_FORMAT_8G               2
> > > @@ -48,7 +56,6 @@
> > >  struct adxl345_data {
> > >       const struct adxl345_chip_info *info;
> > >       struct regmap *regmap;
> > > -     u8 data_range;
> > >  };
> > >
> > >  #define ADXL345_CHANNEL(index, axis) {                                       \
> > > @@ -218,15 +225,14 @@ 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)
> > > +     /* Enable full-resolution mode */
> > > +     ret = regmap_update_bits(regmap, ADXL345_REG_DATA_FORMAT,
> > > +                              ADXL345_DATA_FORMAT_MSK, 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;  
> >  

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

[Jonathan Cameron]

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

> On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Wed, 27 Mar 2024 22:03:20 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > Add a setup function implementation to the spi module to enable spi-3wire
> > > as option when specified in the device-tree.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl345.h     |  2 ++
> > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > index 4ea9341d4..e6bc3591c 100644
> > > --- a/drivers/iio/accel/adxl345.h
> > > +++ b/drivers/iio/accel/adxl345.h
> > > @@ -30,6 +30,8 @@
> > >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > >
> > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > > +
> > >  #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 */
> > > 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);  
> > Your earlier patch carefully (I think) left one or two fields alone, then
> > this write just comes in and changes them. In particular INT_INVERT.
> >  
> Why do you refer here to INT_INVERT? In this code above I try to set
> SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> it happens in adxl345_spi.c. Passing this function to the bus
> independent adxl345_core.c file allows to configure the bus first.
> Therefore, I'm using the update function in core masking out the SPI
> filag.

Ah. Ok.  It was only intended to mask out the SPI3-wire bit, not the
other bits that you also masked out.  I thought intent was to leave
them untouched for some reason.  Given they don't matter in the driver
at the moment (no interrupt support) then no problem.

> 
> My reason why I try to keep INT_INVERT out is different. There is
> another driver for the same part in the kernel:
> ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> interrupts of the adxl345 for the input device implementation. I
> assumed, that in the iio implementation there won't be interrupt
> handling for the adx345, since it is not an input device. Does this
> make sense?

No. You can't use these two drivers at the same time.  They will almost
certainly trample over each other in actually reading channels etc.

Their is some legacy of old drivers in input from a long time back.
Given this driver clearly doesn't have full functionality yet in IIO there
and the different userspace ABI, we've just left the input driver alone.

> 
> > If it doesn't makes sense to write it there, either write that bit
> > every time here, or leave it alone every time.  Not decide on whether
> > to write the bit based on SPI_3WIRE or not.  As far as I know they
> > are unconnected features.
> >  
> I think I did not understand. Could you please specify a bit more?
> When spi-3wire is configured in the DT it has to be parsed and handled
> in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> SPI_3WIRE there. I don't want to place SPI specific code in the core
> file.

My confusion was that you were deliberately not touching the other unused
flags.  In reality you are touching the but only if you enable 3wire.
I would write them register to 0 in the !3wire case so all other values
are the same in both paths.

> 
> > > +     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 v5 7/7] iio: accel: adxl345: Add spi-3wire option

[Lothar Rubusch]

On Sat, Mar 30, 2024 at 4:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Fri, 29 Mar 2024 01:33:01 +0100
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Wed, 27 Mar 2024 22:03:20 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > Add a setup function implementation to the spi module to enable spi-3wire
> > > > as option when specified in the device-tree.
> > > >
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > ---
> > > >  drivers/iio/accel/adxl345.h     |  2 ++
> > > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > > index 4ea9341d4..e6bc3591c 100644
> > > > --- a/drivers/iio/accel/adxl345.h
> > > > +++ b/drivers/iio/accel/adxl345.h
> > > > @@ -30,6 +30,8 @@
> > > >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > > >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > > >
> > > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > > > +
> > > >  #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 */
> > > > 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);
> > > Your earlier patch carefully (I think) left one or two fields alone, then
> > > this write just comes in and changes them. In particular INT_INVERT.
> > >
> > Why do you refer here to INT_INVERT? In this code above I try to set
> > SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> > it happens in adxl345_spi.c. Passing this function to the bus
> > independent adxl345_core.c file allows to configure the bus first.
> > Therefore, I'm using the update function in core masking out the SPI
> > filag.
>
> Ah. Ok.  It was only intended to mask out the SPI3-wire bit, not the
> other bits that you also masked out.  I thought intent was to leave
> them untouched for some reason.  Given they don't matter in the driver
> at the moment (no interrupt support) then no problem.
>
> >
> > My reason why I try to keep INT_INVERT out is different. There is
> > another driver for the same part in the kernel:
> > ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> > interrupts of the adxl345 for the input device implementation. I
> > assumed, that in the iio implementation there won't be interrupt
> > handling for the adx345, since it is not an input device. Does this
> > make sense?
>
> No. You can't use these two drivers at the same time.  They will almost
> certainly trample over each other in actually reading channels etc.
>
> Their is some legacy of old drivers in input from a long time back.
> Given this driver clearly doesn't have full functionality yet in IIO there
> and the different userspace ABI, we've just left the input driver alone.
>
Going by the git history gave this impression, too. But it was still a
bit confusing to me.

The IIO driver so far does not handle any of the interrupt features.
The older driver also seems to support more of the chip's features.
Would it make sense to continue implement/port what's missing -
feature by feature - for the IIO driver in order to make the input
driver obsolete (one day)?

> >
> > > If it doesn't makes sense to write it there, either write that bit
> > > every time here, or leave it alone every time.  Not decide on whether
> > > to write the bit based on SPI_3WIRE or not.  As far as I know they
> > > are unconnected features.
> > >
> > I think I did not understand. Could you please specify a bit more?
> > When spi-3wire is configured in the DT it has to be parsed and handled
> > in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> > SPI_3WIRE there. I don't want to place SPI specific code in the core
> > file.
>
> My confusion was that you were deliberately not touching the other unused
> flags.  In reality you are touching the but only if you enable 3wire.
> I would write them register to 0 in the !3wire case so all other values
> are the same in both paths.
>
> >
> > > > +     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 v5 7/7] iio: accel: adxl345: Add spi-3wire option

[Jonathan Cameron]

On Mon, 1 Apr 2024 18:06:24 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Sat, Mar 30, 2024 at 4:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Fri, 29 Mar 2024 01:33:01 +0100
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Wed, 27 Mar 2024 22:03:20 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >  
> > > > > Add a setup function implementation to the spi module to enable spi-3wire
> > > > > as option when specified in the device-tree.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > ---
> > > > >  drivers/iio/accel/adxl345.h     |  2 ++
> > > > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > > > index 4ea9341d4..e6bc3591c 100644
> > > > > --- a/drivers/iio/accel/adxl345.h
> > > > > +++ b/drivers/iio/accel/adxl345.h
> > > > > @@ -30,6 +30,8 @@
> > > > >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > > > >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > > > >
> > > > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > > > > +
> > > > >  #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 */
> > > > > 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);  
> > > > Your earlier patch carefully (I think) left one or two fields alone, then
> > > > this write just comes in and changes them. In particular INT_INVERT.
> > > >  
> > > Why do you refer here to INT_INVERT? In this code above I try to set
> > > SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> > > it happens in adxl345_spi.c. Passing this function to the bus
> > > independent adxl345_core.c file allows to configure the bus first.
> > > Therefore, I'm using the update function in core masking out the SPI
> > > filag.  
> >
> > Ah. Ok.  It was only intended to mask out the SPI3-wire bit, not the
> > other bits that you also masked out.  I thought intent was to leave
> > them untouched for some reason.  Given they don't matter in the driver
> > at the moment (no interrupt support) then no problem.
> >  
> > >
> > > My reason why I try to keep INT_INVERT out is different. There is
> > > another driver for the same part in the kernel:
> > > ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> > > interrupts of the adxl345 for the input device implementation. I
> > > assumed, that in the iio implementation there won't be interrupt
> > > handling for the adx345, since it is not an input device. Does this
> > > make sense?  
> >
> > No. You can't use these two drivers at the same time.  They will almost
> > certainly trample over each other in actually reading channels etc.
> >
> > Their is some legacy of old drivers in input from a long time back.
> > Given this driver clearly doesn't have full functionality yet in IIO there
> > and the different userspace ABI, we've just left the input driver alone.
> >  
> Going by the git history gave this impression, too. But it was still a
> bit confusing to me.
> 
> The IIO driver so far does not handle any of the interrupt features.
> The older driver also seems to support more of the chip's features.
> Would it make sense to continue implement/port what's missing -
> feature by feature - for the IIO driver in order to make the input
> driver obsolete (one day)?

Yes, though it will be challenging because of the ABI differences.
We do have a few cross subsystem bridge drivers, but the few goes we've
had at an accel bridge driver haven't made it into the kernel.  In particular
we don't have an in kernel interface for threshold events and similar in IIO.
It would be easy enough to add one, but no one has ever cared enough to
do the work. :(

Jonathan


> 
> > >  
> > > > If it doesn't makes sense to write it there, either write that bit
> > > > every time here, or leave it alone every time.  Not decide on whether
> > > > to write the bit based on SPI_3WIRE or not.  As far as I know they
> > > > are unconnected features.
> > > >  
> > > I think I did not understand. Could you please specify a bit more?
> > > When spi-3wire is configured in the DT it has to be parsed and handled
> > > in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> > > SPI_3WIRE there. I don't want to place SPI specific code in the core
> > > file.  
> >
> > My confusion was that you were deliberately not touching the other unused
> > flags.  In reality you are touching the but only if you enable 3wire.
> > I would write them register to 0 in the !3wire case so all other values
> > are the same in both paths.
> >  
> > >  
> > > > > +     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 v5 7/7] iio: accel: adxl345: Add spi-3wire option

[Lothar Rubusch]

On Mon, Apr 1, 2024 at 6:53 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Mon, 1 Apr 2024 18:06:24 +0200
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
>
> > On Sat, Mar 30, 2024 at 4:24 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Fri, 29 Mar 2024 01:33:01 +0100
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >
> > > > On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > > > >
> > > > > On Wed, 27 Mar 2024 22:03:20 +0000
> > > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > > >
> > > > > > Add a setup function implementation to the spi module to enable spi-3wire
> > > > > > as option when specified in the device-tree.
> > > > > >
> > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > > ---
> > > > > >  drivers/iio/accel/adxl345.h     |  2 ++
> > > > > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > > > > index 4ea9341d4..e6bc3591c 100644
> > > > > > --- a/drivers/iio/accel/adxl345.h
> > > > > > +++ b/drivers/iio/accel/adxl345.h
> > > > > > @@ -30,6 +30,8 @@
> > > > > >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > > > > >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > > > > >
> > > > > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > > > > > +
> > > > > >  #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 */
> > > > > > 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);
> > > > > Your earlier patch carefully (I think) left one or two fields alone, then
> > > > > this write just comes in and changes them. In particular INT_INVERT.
> > > > >
> > > > Why do you refer here to INT_INVERT? In this code above I try to set
> > > > SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> > > > it happens in adxl345_spi.c. Passing this function to the bus
> > > > independent adxl345_core.c file allows to configure the bus first.
> > > > Therefore, I'm using the update function in core masking out the SPI
> > > > filag.
> > >
> > > Ah. Ok.  It was only intended to mask out the SPI3-wire bit, not the
> > > other bits that you also masked out.  I thought intent was to leave
> > > them untouched for some reason.  Given they don't matter in the driver
> > > at the moment (no interrupt support) then no problem.
> > >
> > > >
> > > > My reason why I try to keep INT_INVERT out is different. There is
> > > > another driver for the same part in the kernel:
> > > > ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> > > > interrupts of the adxl345 for the input device implementation. I
> > > > assumed, that in the iio implementation there won't be interrupt
> > > > handling for the adx345, since it is not an input device. Does this
> > > > make sense?
> > >
> > > No. You can't use these two drivers at the same time.  They will almost
> > > certainly trample over each other in actually reading channels etc.
> > >
> > > Their is some legacy of old drivers in input from a long time back.
> > > Given this driver clearly doesn't have full functionality yet in IIO there
> > > and the different userspace ABI, we've just left the input driver alone.
> > >
> > Going by the git history gave this impression, too. But it was still a
> > bit confusing to me.
> >
> > The IIO driver so far does not handle any of the interrupt features.
> > The older driver also seems to support more of the chip's features.
> > Would it make sense to continue implement/port what's missing -
> > feature by feature - for the IIO driver in order to make the input
> > driver obsolete (one day)?
>
> Yes, though it will be challenging because of the ABI differences.
> We do have a few cross subsystem bridge drivers, but the few goes we've
> had at an accel bridge driver haven't made it into the kernel.  In particular
> we don't have an in kernel interface for threshold events and similar in IIO.
> It would be easy enough to add one, but no one has ever cared enough to
> do the work. :(
>
Perhaps there are easier things (precision, power saving measures,
etc) of that particular accelerometer to port first. Open issues which
even I could give a try here. Sounds really exciting to me, though,
but before I definitely need a bit more experience with community and
APIs.

What do you mean with cross subsystem bridge drivers, or this accel
bridge driver? I mean, can you provide me with a link to that driver
to have a look into what direction that is going to?

Anyway, I really appreciate already your patience with my patches, the
direct and helpful answers! I appreciate the support. Thinking and
re-thinking over every particular line of code is really an
experience. I don't want to go too much into off-topic discussions
here, if this is not the forum for that, please let me know :)

> Jonathan
>
>
> >
> > > >
> > > > > If it doesn't makes sense to write it there, either write that bit
> > > > > every time here, or leave it alone every time.  Not decide on whether
> > > > > to write the bit based on SPI_3WIRE or not.  As far as I know they
> > > > > are unconnected features.
> > > > >
> > > > I think I did not understand. Could you please specify a bit more?
> > > > When spi-3wire is configured in the DT it has to be parsed and handled
> > > > in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> > > > SPI_3WIRE there. I don't want to place SPI specific code in the core
> > > > file.
> > >
> > > My confusion was that you were deliberately not touching the other unused
> > > flags.  In reality you are touching the but only if you enable 3wire.
> > > I would write them register to 0 in the !3wire case so all other values
> > > are the same in both paths.
> > >
> > > >
> > > > > > +     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 v5 7/7] iio: accel: adxl345: Add spi-3wire option

[Jonathan Cameron]

On Tue, 2 Apr 2024 12:11:43 +0200
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Mon, Apr 1, 2024 at 6:53 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Mon, 1 Apr 2024 18:06:24 +0200
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >  
> > > On Sat, Mar 30, 2024 at 4:24 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Fri, 29 Mar 2024 01:33:01 +0100
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >  
> > > > > On Thu, Mar 28, 2024 at 2:39 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > > > >
> > > > > > On Wed, 27 Mar 2024 22:03:20 +0000
> > > > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > > > >  
> > > > > > > Add a setup function implementation to the spi module to enable spi-3wire
> > > > > > > as option when specified in the device-tree.
> > > > > > >
> > > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > > > > ---
> > > > > > >  drivers/iio/accel/adxl345.h     |  2 ++
> > > > > > >  drivers/iio/accel/adxl345_spi.c | 12 +++++++++++-
> > > > > > >  2 files changed, 13 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > > > > > > index 4ea9341d4..e6bc3591c 100644
> > > > > > > --- a/drivers/iio/accel/adxl345.h
> > > > > > > +++ b/drivers/iio/accel/adxl345.h
> > > > > > > @@ -30,6 +30,8 @@
> > > > > > >  #define ADXL345_POWER_CTL_MEASURE    BIT(3)
> > > > > > >  #define ADXL345_POWER_CTL_STANDBY    0x00
> > > > > > >
> > > > > > > +#define ADXL345_DATA_FORMAT_SPI_3WIRE        BIT(6) /* 3-wire SPI mode */
> > > > > > > +
> > > > > > >  #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 */
> > > > > > > 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);  
> > > > > > Your earlier patch carefully (I think) left one or two fields alone, then
> > > > > > this write just comes in and changes them. In particular INT_INVERT.
> > > > > >  
> > > > > Why do you refer here to INT_INVERT? In this code above I try to set
> > > > > SPI to 3 lines. Since this is a SPI configuration, i.e. bus specific,
> > > > > it happens in adxl345_spi.c. Passing this function to the bus
> > > > > independent adxl345_core.c file allows to configure the bus first.
> > > > > Therefore, I'm using the update function in core masking out the SPI
> > > > > filag.  
> > > >
> > > > Ah. Ok.  It was only intended to mask out the SPI3-wire bit, not the
> > > > other bits that you also masked out.  I thought intent was to leave
> > > > them untouched for some reason.  Given they don't matter in the driver
> > > > at the moment (no interrupt support) then no problem.
> > > >  
> > > > >
> > > > > My reason why I try to keep INT_INVERT out is different. There is
> > > > > another driver for the same part in the kernel:
> > > > > ./drivers/input/misc/adxl34x.c - This is a input driver, using the
> > > > > interrupts of the adxl345 for the input device implementation. I
> > > > > assumed, that in the iio implementation there won't be interrupt
> > > > > handling for the adx345, since it is not an input device. Does this
> > > > > make sense?  
> > > >
> > > > No. You can't use these two drivers at the same time.  They will almost
> > > > certainly trample over each other in actually reading channels etc.
> > > >
> > > > Their is some legacy of old drivers in input from a long time back.
> > > > Given this driver clearly doesn't have full functionality yet in IIO there
> > > > and the different userspace ABI, we've just left the input driver alone.
> > > >  
> > > Going by the git history gave this impression, too. But it was still a
> > > bit confusing to me.
> > >
> > > The IIO driver so far does not handle any of the interrupt features.
> > > The older driver also seems to support more of the chip's features.
> > > Would it make sense to continue implement/port what's missing -
> > > feature by feature - for the IIO driver in order to make the input
> > > driver obsolete (one day)?  
> >
> > Yes, though it will be challenging because of the ABI differences.
> > We do have a few cross subsystem bridge drivers, but the few goes we've
> > had at an accel bridge driver haven't made it into the kernel.  In particular
> > we don't have an in kernel interface for threshold events and similar in IIO.
> > It would be easy enough to add one, but no one has ever cared enough to
> > do the work. :(
> >  
> Perhaps there are easier things (precision, power saving measures,
> etc) of that particular accelerometer to port first. Open issues which
> even I could give a try here. Sounds really exciting to me, though,
> but before I definitely need a bit more experience with community and
> APIs.

That would be great.

> 
> What do you mean with cross subsystem bridge drivers, or this accel
> bridge driver? I mean, can you provide me with a link to that driver
> to have a look into what direction that is going to?
The last go at this was probably this one.

https://lore.kernel.org/linux-iio/d52cf9ee5944c90c69f6e74a46d844cef51e487e.1555362312.git.hns@goldelico.com/

> 
> Anyway, I really appreciate already your patience with my patches, the
> direct and helpful answers! I appreciate the support. Thinking and
> re-thinking over every particular line of code is really an
> experience. I don't want to go too much into off-topic discussions
> here, if this is not the forum for that, please let me know :)

People used to sometimes be on IRC but I'm not sure anyone is any more :(
I rarely joined because of network restrictions at work.

It's fine to ask questions on this mailing list though.
Nature of kernel development is that people need to get good at skipping
threads they aren't interested in!

Jonathan




> 
> > Jonathan
> >
> >  
> > >  
> > > > >  
> > > > > > If it doesn't makes sense to write it there, either write that bit
> > > > > > every time here, or leave it alone every time.  Not decide on whether
> > > > > > to write the bit based on SPI_3WIRE or not.  As far as I know they
> > > > > > are unconnected features.
> > > > > >  
> > > > > I think I did not understand. Could you please specify a bit more?
> > > > > When spi-3wire is configured in the DT it has to be parsed and handled
> > > > > in the bus specific part, i.e. the adxl345_spi.c Therefore I configure
> > > > > SPI_3WIRE there. I don't want to place SPI specific code in the core
> > > > > file.  
> > > >
> > > > My confusion was that you were deliberately not touching the other unused
> > > > flags.  In reality you are touching the but only if you enable 3wire.
> > > > I would write them register to 0 in the !3wire case so all other values
> > > > are the same in both paths.
> > > >  
> > > > >  
> > > > > > > +     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 = {  
> > > > > >  
> > > >  
> > >  
> >