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

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

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

-- 
2.25.1

[PATCH v4 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. Changes then need to be masked.

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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 8bd30a23e..be6758015 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -42,13 +42,13 @@
 #define ADXL345_DATA_FORMAT_4G		1
 #define ADXL345_DATA_FORMAT_8G		2
 #define ADXL345_DATA_FORMAT_16G		3
+#define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */
 
 #define ADXL345_DEVID			0xE5
 
 struct adxl345_data {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
-	u8 data_range;
 };
 
 #define ADXL345_CHANNEL(index, axis) {					\
@@ -219,14 +219,13 @@ 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)
+	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

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

[Jonathan Cameron]

On Mon, 25 Mar 2024 15:33:50 +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. Changes then need to be masked.
> 
> 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 | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 8bd30a23e..be6758015 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -42,13 +42,13 @@
>  #define ADXL345_DATA_FORMAT_4G		1
>  #define ADXL345_DATA_FORMAT_8G		2
>  #define ADXL345_DATA_FORMAT_16G		3
> +#define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */

I'm not keen on seeing masking of a bit we don't yet
handle done by value.  Can we instead build this up by what we 'want' to
write rather than don't. Will need a few more defines perhaps to cover
the masks of SELF_TEST, INT_INVERT, FULL_RES, Justify and Range.

>  
>  #define ADXL345_DEVID			0xE5
>  
>  struct adxl345_data {
>  	const struct adxl345_chip_info *info;
>  	struct regmap *regmap;
> -	u8 data_range;
>  };
>  
>  #define ADXL345_CHANNEL(index, axis) {					\
> @@ -219,14 +219,13 @@ 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)
> +	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 v4 1/7] iio: accel: adxl345: Make data_range obsolete

[Lothar Rubusch]

On Mon, Mar 25, 2024 at 9:32 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 25 Mar 2024 15:33:50 +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. Changes then need to be masked.
> >
> > 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 | 9 ++++-----
> >  1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 8bd30a23e..be6758015 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -42,13 +42,13 @@
> >  #define ADXL345_DATA_FORMAT_4G               1
> >  #define ADXL345_DATA_FORMAT_8G               2
> >  #define ADXL345_DATA_FORMAT_16G              3
> > +#define ADXL345_DATA_FORMAT_MSK              ~((u8) BIT(6)) /* ignore spi-3wire */
>
> I'm not keen on seeing masking of a bit we don't yet
> handle done by value.  Can we instead build this up by what we 'want' to
> write rather than don't. Will need a few more defines perhaps to cover
> the masks of SELF_TEST, INT_INVERT, FULL_RES, Justify and Range.
>

Good point. Anyway, there is also an input driver implementation for
the adxl345, mainly dealing with the interrupt feature as input
device. Thus, for the iio implementation I would suggest to reduce the
mask just to cover SELF_TEST and FULL_RES and leave INT_INVERT out. Is
this ok?

> >
> >  #define ADXL345_DEVID                        0xE5
> >
> >  struct adxl345_data {
> >       const struct adxl345_chip_info *info;
> >       struct regmap *regmap;
> > -     u8 data_range;
> >  };
> >
> >  #define ADXL345_CHANNEL(index, axis) {                                       \
> > @@ -219,14 +219,13 @@ 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)
> > +     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 v4 1/7] iio: accel: adxl345: Make data_range obsolete

[Jonathan Cameron]

On Tue, 26 Mar 2024 21:59:34 +0100
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> On Mon, Mar 25, 2024 at 9:32 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 25 Mar 2024 15:33:50 +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. Changes then need to be masked.
> > >
> > > 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 | 9 ++++-----
> > >  1 file changed, 4 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > > index 8bd30a23e..be6758015 100644
> > > --- a/drivers/iio/accel/adxl345_core.c
> > > +++ b/drivers/iio/accel/adxl345_core.c
> > > @@ -42,13 +42,13 @@
> > >  #define ADXL345_DATA_FORMAT_4G               1
> > >  #define ADXL345_DATA_FORMAT_8G               2
> > >  #define ADXL345_DATA_FORMAT_16G              3
> > > +#define ADXL345_DATA_FORMAT_MSK              ~((u8) BIT(6)) /* ignore spi-3wire */  
> >
> > I'm not keen on seeing masking of a bit we don't yet
> > handle done by value.  Can we instead build this up by what we 'want' to
> > write rather than don't. Will need a few more defines perhaps to cover
> > the masks of SELF_TEST, INT_INVERT, FULL_RES, Justify and Range.
> >  
> 
> Good point. Anyway, there is also an input driver implementation for
> the adxl345, mainly dealing with the interrupt feature as input
> device. Thus, for the iio implementation I would suggest to reduce the
> mask just to cover SELF_TEST and FULL_RES and leave INT_INVERT out. Is
> this ok?
yes, that sounds fine
> 
> > >
> > >  #define ADXL345_DEVID                        0xE5
> > >
> > >  struct adxl345_data {
> > >       const struct adxl345_chip_info *info;
> > >       struct regmap *regmap;
> > > -     u8 data_range;
> > >  };
> > >
> > >  #define ADXL345_CHANNEL(index, axis) {                                       \
> > > @@ -219,14 +219,13 @@ 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)
> > > +     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;  
> >  

[PATCH v4 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 | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index be6758015..469015e9c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -218,22 +218,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
 
 	data = iio_priv(indio_dev);
 	data->regmap = regmap;
-	/* Enable full-resolution mode */
+
 	data->info = device_get_match_data(dev);
 	if (!data->info)
 		return -ENODEV;
 
-	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

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

[Jonathan Cameron]

On Mon, 25 Mar 2024 15:33:51 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> 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 | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index be6758015..469015e9c 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -218,22 +218,23 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap)
>  
>  	data = iio_priv(indio_dev);
>  	data->regmap = regmap;
> -	/* Enable full-resolution mode */
The naming of the written value gives this comment away + it's obviously misplaced
after previous patch. I'd just drop the comment in patch 1.

> +
>  	data->info = device_get_match_data(dev);
>  	if (!data->info)
>  		return -ENODEV;
>  
> -	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)

[PATCH v4 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      | 31 +++++++++++++++++++++++++++++++
 drivers/iio/accel/adxl345_core.c | 29 -----------------------------
 2 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index 284bd387c..ee169fed4 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -8,6 +8,37 @@
 #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_FULL_RES	BIT(3) /* Up to 13-bits resolution */
+#define ADXL345_DATA_FORMAT_2G		0
+#define ADXL345_DATA_FORMAT_4G		1
+#define ADXL345_DATA_FORMAT_8G		2
+#define ADXL345_DATA_FORMAT_16G		3
+#define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */
+
+#define ADXL345_DEVID			0xE5
+
 /*
  * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
  * in all g ranges.
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 469015e9c..eba9c048a 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -17,35 +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_FULL_RES	BIT(3) /* Up to 13-bits resolution */
-#define ADXL345_DATA_FORMAT_2G		0
-#define ADXL345_DATA_FORMAT_4G		1
-#define ADXL345_DATA_FORMAT_8G		2
-#define ADXL345_DATA_FORMAT_16G		3
-#define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */
-
-#define ADXL345_DEVID			0xE5
-
 struct adxl345_data {
 	const struct adxl345_chip_info *info;
 	struct regmap *regmap;
-- 
2.25.1

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

[Lothar Rubusch]

Add spi-3wire because the driver optionally supports spi-3wire.

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

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

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

[Krzysztof Kozlowski]

On 25/03/2024 16:33, Lothar Rubusch wrote:
> Add spi-3wire because the driver optionally supports spi-3wire.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.

> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---

This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.

Best regards,
Krzysztof

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

[Lothar Rubusch]

On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/03/2024 16:33, Lothar Rubusch wrote:
> > Add spi-3wire because the driver optionally supports spi-3wire.
>
> This is a friendly reminder during the review process.
>
> It seems my or other reviewer's previous comments were not fully
> addressed. Maybe the feedback got lost between the quotes, maybe you
> just forgot to apply it. Please go back to the previous discussion and
> either implement all requested changes or keep discussing them.
>
> Thank you.
>

You refer yourself to the above mentioned wording. Would replacing
"driver" by "device" in the dt-bindings patch comment be sufficient?
Did I miss something else?

> >
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > ---
>
> This is a friendly reminder during the review process.
>
> It looks like you received a tag and forgot to add it.
>
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, under or above your Signed-off-by tag. Tag is "received", when
> provided in a message replied to you on the mailing list. Tools like b4
> can help here. However, there's no need to repost patches *only* to add

Just for confirmation: when I receive a feedback, requesting a change.
And, I accept the change request. This means, I received a tag
"Reviewed-by" which I have to mention in the upcoming patch version
where this change is implemented and in that particular patch?

> the tags. The upstream maintainer will do that for tags received on the
> version they apply.
>

I'm pretty sure we will still see further iterations. So, I apply the
tags in the next version, already scheduled. Ok?

> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>

Going over the books I feel it does not make sense to still mention
feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
does it? Your link mentiones "However if the patch has changed
substantially in followin version, these tags might not be applicable
anymore"
https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L579

> If a tag was not added on purpose, please state why and what changed.
>
> Best regards,
> Krzysztof
>

I give it a try with b4. Let's see.

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

[Krzysztof Kozlowski]

On 25/03/2024 22:05, Lothar Rubusch wrote:
> On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 25/03/2024 16:33, Lothar Rubusch wrote:
>>> Add spi-3wire because the driver optionally supports spi-3wire.
>>
>> This is a friendly reminder during the review process.
>>
>> It seems my or other reviewer's previous comments were not fully
>> addressed. Maybe the feedback got lost between the quotes, maybe you
>> just forgot to apply it. Please go back to the previous discussion and
>> either implement all requested changes or keep discussing them.
>>
>> Thank you.
>>
> 
> You refer yourself to the above mentioned wording. Would replacing
> "driver" by "device" in the dt-bindings patch comment be sufficient?
> Did I miss something else?

Yes, the wording, but isn't the device require 3-wire mode? Don't just
replace one word with another, but write the proper rationale for your
hardware.

> 
>>>
>>> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
>>> ---
>>
>> This is a friendly reminder during the review process.
>>
>> It looks like you received a tag and forgot to add it.
>>
>> If you do not know the process, here is a short explanation:
>> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
>> versions, under or above your Signed-off-by tag. Tag is "received", when
>> provided in a message replied to you on the mailing list. Tools like b4
>> can help here. However, there's no need to repost patches *only* to add
> 
> Just for confirmation: when I receive a feedback, requesting a change.
> And, I accept the change request. This means, I received a tag
> "Reviewed-by" which I have to mention in the upcoming patch version
> where this change is implemented and in that particular patch?

Please go through the docs. Yes, you received a tag which should be
included with the change.

Reviewer's feedback should not be ignored.


> 
>> the tags. The upstream maintainer will do that for tags received on the
>> version they apply.
>>
> 
> I'm pretty sure we will still see further iterations. So, I apply the
> tags in the next version, already scheduled. Ok?
> 
>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>
> 
> Going over the books I feel it does not make sense to still mention
> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> does it? Your link mentiones "However if the patch has changed

I don't understand. When did you receive the tag? v3, right? So what do
you mean by v1 and v2?


Best regards,
Krzysztof

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

[Lothar Rubusch]

On Mon, Mar 25, 2024 at 10:40 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/03/2024 22:05, Lothar Rubusch wrote:
> > On Mon, Mar 25, 2024 at 7:32 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 25/03/2024 16:33, Lothar Rubusch wrote:
> >>> Add spi-3wire because the driver optionally supports spi-3wire.
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It seems my or other reviewer's previous comments were not fully
> >> addressed. Maybe the feedback got lost between the quotes, maybe you
> >> just forgot to apply it. Please go back to the previous discussion and
> >> either implement all requested changes or keep discussing them.
> >>
> >> Thank you.
> >>
> >
> > You refer yourself to the above mentioned wording. Would replacing
> > "driver" by "device" in the dt-bindings patch comment be sufficient?
> > Did I miss something else?
>
> Yes, the wording, but isn't the device require 3-wire mode? Don't just
> replace one word with another, but write the proper rationale for your
> hardware.
>
It does not require 3-wire SPI. By default the device communicates
regular SPI. It can be configured, though, to communicate 3-wire. The
given patch offers this as option in the DT.

> >
> >>>
> >>> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> >>> ---
> >>
> >> This is a friendly reminder during the review process.
> >>
> >> It looks like you received a tag and forgot to add it.
> >>
> >> If you do not know the process, here is a short explanation:
> >> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> >> versions, under or above your Signed-off-by tag. Tag is "received", when
> >> provided in a message replied to you on the mailing list. Tools like b4
> >> can help here. However, there's no need to repost patches *only* to add
> >
> > Just for confirmation: when I receive a feedback, requesting a change.
> > And, I accept the change request. This means, I received a tag
> > "Reviewed-by" which I have to mention in the upcoming patch version
> > where this change is implemented and in that particular patch?
>
> Please go through the docs. Yes, you received a tag which should be
> included with the change.
>
> Reviewer's feedback should not be ignored.
>
>
> >
> >> the tags. The upstream maintainer will do that for tags received on the
> >> version they apply.
> >>
> >
> > I'm pretty sure we will still see further iterations. So, I apply the
> > tags in the next version, already scheduled. Ok?
> >
> >> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> >>
> >
> > Going over the books I feel it does not make sense to still mention
> > feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> > does it? Your link mentiones "However if the patch has changed
>
> I don't understand. When did you receive the tag? v3, right? So what do
> you mean by v1 and v2?
>

V1: The first version of the 3wire patch. I have split the single
patch upon some feedback (yours?!) - V2... So, my current
interpretation is, that every feedback I need to mention as
Reviewed-by tag, no?

>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 25/03/2024 23:09, Lothar Rubusch wrote:
>>
>>
>>>
>>>> the tags. The upstream maintainer will do that for tags received on the
>>>> version they apply.
>>>>
>>>
>>> I'm pretty sure we will still see further iterations. So, I apply the
>>> tags in the next version, already scheduled. Ok?
>>>
>>>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
>>>>
>>>
>>> Going over the books I feel it does not make sense to still mention
>>> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
>>> does it? Your link mentiones "However if the patch has changed
>>
>> I don't understand. When did you receive the tag? v3, right? So what do
>> you mean by v1 and v2?
>>
> 
> V1: The first version of the 3wire patch. I have split the single
> patch upon some feedback (yours?!) - V2... So, my current
> interpretation is, that every feedback I need to mention as
> Reviewed-by tag, no?

What? Feedback is not review. It's clearly explained in submitting
patches. Please read it.

Best regards,
Krzysztof

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

[Lothar Rubusch]

On Tue, Mar 26, 2024 at 7:30 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 25/03/2024 23:09, Lothar Rubusch wrote:
> >>
> >>
> >>>
> >>>> the tags. The upstream maintainer will do that for tags received on the
> >>>> version they apply.
> >>>>
> >>>
> >>> I'm pretty sure we will still see further iterations. So, I apply the
> >>> tags in the next version, already scheduled. Ok?
> >>>
> >>>> https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577
> >>>>
> >>>
> >>> Going over the books I feel it does not make sense to still mention
> >>> feedback ("Reveiewed-by") for the v1 or v2 of the patch here in a v5,
> >>> does it? Your link mentiones "However if the patch has changed
> >>
> >> I don't understand. When did you receive the tag? v3, right? So what do
> >> you mean by v1 and v2?
> >>
> >
> > V1: The first version of the 3wire patch. I have split the single
> > patch upon some feedback (yours?!) - V2... So, my current
> > interpretation is, that every feedback I need to mention as
> > Reviewed-by tag, no?
>
> What? Feedback is not review. It's clearly explained in submitting
> patches. Please read it.
>

Exactly. My missunderstanding here is this:  Why did you send me a
reminder that I forgot to add "Reviewed-by" tag in your last mail?
Could you please clarify your last mail? You wrote:
"(...)
This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

If you do not know the process, here is a short explanation:
Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, (...)"

AFAIK noone literally had told me: "please add a Reviewed-by me tag",
or did I miss something? I'm a bit lost here, sorry.

> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 26/03/2024 21:17, Lothar Rubusch wrote:
>>>
>>> V1: The first version of the 3wire patch. I have split the single
>>> patch upon some feedback (yours?!) - V2... So, my current
>>> interpretation is, that every feedback I need to mention as
>>> Reviewed-by tag, no?
>>
>> What? Feedback is not review. It's clearly explained in submitting
>> patches. Please read it.
>>
> 
> Exactly. My missunderstanding here is this:  Why did you send me a
> reminder that I forgot to add "Reviewed-by" tag in your last mail?
> Could you please clarify your last mail? You wrote:
> "(...)
> This is a friendly reminder during the review process.
> 
> It looks like you received a tag and forgot to add it.
> 
> If you do not know the process, here is a short explanation:
> Please add Acked-by/Reviewed-by/Tested-by tags when posting new
> versions, (...)"
> 
> AFAIK noone literally had told me: "please add a Reviewed-by me tag",
> or did I miss something? I'm a bit lost here, sorry.
> 

What was this then:

https://lore.kernel.org/all/9700cc88-bddb-480d-9417-04b2ff539a2f@linaro.org/

Best regards,
Krzysztof

[PATCH v4 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 ee169fed4..620a2e0f0 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -59,6 +59,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 eba9c048a..476d729bc 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

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

[Jonathan Cameron]

On Mon, 25 Mar 2024 15:33:54 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

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

Wrap descriptions at 75 chars, not around 60 as you've done here.

> 
> 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 ee169fed4..620a2e0f0 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -59,6 +59,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 eba9c048a..476d729bc 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*))

Wrap as per the header definition above. (align just after opening bracket).

>  {
>  	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 = {

[PATCH v4 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 476d729bc..cc3da4ece 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 v4 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     |  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 620a2e0f0..55a72ca38 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -36,6 +36,7 @@
 #define ADXL345_DATA_FORMAT_8G		2
 #define ADXL345_DATA_FORMAT_16G		3
 #define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */
+#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
 
 #define ADXL345_DEVID			0xE5
 
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 v4 7/7] iio: accel: adxl345: Add spi-3wire option

[Jonathan Cameron]

On Mon, 25 Mar 2024 15:33:56 +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     |  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 620a2e0f0..55a72ca38 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -36,6 +36,7 @@
>  #define ADXL345_DATA_FORMAT_8G		2
>  #define ADXL345_DATA_FORMAT_16G		3
>  #define ADXL345_DATA_FORMAT_MSK		~((u8) BIT(6)) /* ignore spi-3wire */
> +#define ADXL345_DATA_FORMAT_SPI_3WIRE	BIT(6)
If you argue in favour of keeping the MSK as negation of this bit, then
reorder these two and define FORMAT_MSK in terms of SPI_3WIRE.

Note the name of FORMAT_MASK is also an issue now you actually write the 3wire bit.
So I'd get rid of that define in favour of positive defines of all the fields that
make it up used inline as it's only accessed in one place anyway.
That avoids the need for a mask of 'not' something.

>  
>  #define ADXL345_DEVID			0xE5
>  
> 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 = {