Re: [PATCH v1 06/12] iio: accel: adxl313: prepare interrupt handling

Re: [PATCH v1 06/12] iio: accel: adxl313: prepare interrupt handling

[Lothar Rubusch]

Hi Andy, I forgot to put my mail addresses as well. I copied your answer
now from the mailing list archive. Hence, sorry for the bad formatting
of this mail.

One question / remark down below.

> On Sun, May 18, 2025 at 11:13:15AM +0000, Lothar Rubusch wrote:
> > Evaluate the devicetree property for an optional interrupt line, and
> > configure the interrupt mapping accordingly. When no interrupt line
> > is defined in the devicetree, keep the FIFO in bypass mode as before.
>
> ...
>
> > +        ret = regmap_write(data->regmap, ADXL313_REG_INT_MAP, regval);
>
> Don't you want to use regmap_assign_bits() or something like this to have
> the above ternary be included?
>

Thank you so much. I guess this is a function I was looking for quite
a while and I
know several places where to use it.

Anyway, I saw, my hardware test setup still runs on an older kernel
w/o regmap_assign_bits().
So, I kindly liked to ask if you have any objections against leaving
regmap_write() for now? Actually I'd prefer first to see the
activity/inactivity stuff in, in case this will need some more
modifications and I need to verify them on hardware. I think, leaving
regmap_write() here would make that easier for this patch set. Please,
let me know?

I'm about to send a v2, for the follow up discussion.
Best,
L

> > +        if (ret)
> > +            return ret;
>

Re: [PATCH v1 06/12] iio: accel: adxl313: prepare interrupt handling

[Andy Shevchenko]

On Tue, May 20, 2025 at 09:32:18PM +0200, Lothar Rubusch wrote:
> Hi Andy, I forgot to put my mail addresses as well. I copied your answer
> now from the mailing list archive. Hence, sorry for the bad formatting
> of this mail.
> 
> One question / remark down below.
> 
> > On Sun, May 18, 2025 at 11:13:15AM +0000, Lothar Rubusch wrote:
> > > Evaluate the devicetree property for an optional interrupt line, and
> > > configure the interrupt mapping accordingly. When no interrupt line
> > > is defined in the devicetree, keep the FIFO in bypass mode as before.

...

> > > +        ret = regmap_write(data->regmap, ADXL313_REG_INT_MAP, regval);
> >
> > Don't you want to use regmap_assign_bits() or something like this to have
> > the above ternary be included?
> 
> Thank you so much. I guess this is a function I was looking for quite
> a while and I know several places where to use it.
> 
> Anyway, I saw, my hardware test setup still runs on an older kernel
> w/o regmap_assign_bits().

You are going to upstream the driver, right? So, we don't care about old
kernels as there was no such code at all, and since it's not a fix for
backporting I see no impediments to use the modern APIs.

> So, I kindly liked to ask if you have any objections against leaving
> regmap_write() for now? Actually I'd prefer first to see the
> activity/inactivity stuff in, in case this will need some more
> modifications and I need to verify them on hardware. I think, leaving
> regmap_write() here would make that easier for this patch set. Please,
> let me know?

Ask maintainers. I will not object if they agree on your justification.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 06/12] iio: accel: adxl313: prepare interrupt handling

[Jonathan Cameron]

On Tue, 20 May 2025 22:49:06 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Tue, May 20, 2025 at 09:32:18PM +0200, Lothar Rubusch wrote:
> > Hi Andy, I forgot to put my mail addresses as well. I copied your answer
> > now from the mailing list archive. Hence, sorry for the bad formatting
> > of this mail.
> > 
> > One question / remark down below.
> >   
> > > On Sun, May 18, 2025 at 11:13:15AM +0000, Lothar Rubusch wrote:  
> > > > Evaluate the devicetree property for an optional interrupt line, and
> > > > configure the interrupt mapping accordingly. When no interrupt line
> > > > is defined in the devicetree, keep the FIFO in bypass mode as before.  
> 
> ...
> 
> > > > +        ret = regmap_write(data->regmap, ADXL313_REG_INT_MAP, regval);  
> > >
> > > Don't you want to use regmap_assign_bits() or something like this to have
> > > the above ternary be included?  
> > 
> > Thank you so much. I guess this is a function I was looking for quite
> > a while and I know several places where to use it.
> > 
> > Anyway, I saw, my hardware test setup still runs on an older kernel
> > w/o regmap_assign_bits().  
> 
> You are going to upstream the driver, right? So, we don't care about old
> kernels as there was no such code at all, and since it's not a fix for
> backporting I see no impediments to use the modern APIs.
> 
> > So, I kindly liked to ask if you have any objections against leaving
> > regmap_write() for now? Actually I'd prefer first to see the
> > activity/inactivity stuff in, in case this will need some more
> > modifications and I need to verify them on hardware. I think, leaving
> > regmap_write() here would make that easier for this patch set. Please,
> > let me know?  
> 
> Ask maintainers. I will not object if they agree on your justification.
> 
Hmm. Given the good progress you are making on this driver anyway I'll
go with 'maybe' particularly if you add a final patch on top that
updates the code to use nicer bits of regmap that have been introduced
more recently.

Jonathan

[PATCH v1 00/12] iio: accel: adxl313: add power-save on activity/inactivity

[Lothar Rubusch]

The patch set covers the following topics:
- add debug register and regmap cache
- prepare iio channel scan_type and scan_index
- prepare interrupt handling
- implement fifo with watermark
- add activity/inactivity together with auto-sleep with link bit
- documentation

Similar situation and approach as for the ADXL345. AC/DC coupling might be
a pending option as it is quite the same as for ADXL345.

Since activity and inactivity here are implemented covering all axis, I
assumed x&y&z. Thus the driver uses a fake channel for activity/inactiviy.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
Lothar Rubusch (12):
  iio: accel: adxl313: add debug register
  iio: accel: adxl313: introduce channel scan_index
  iio: accel: adxl313: configure scan type for buffer
  iio: accel: adxl313: make use of regmap cache
  iio: accel: adxl313: add function to enable measurement
  iio: accel: adxl313: prepare interrupt handling
  iio: accel: adxl313: add basic interrupt handling
  iio: accel: adxl313: add FIFO watermark
  iio: accel: adxl313: add activity sensing
  iio: accel: adxl313: add inactivity sensing
  iio: accel: adxl313: implement power-save on inactivity
  docs: iio: add ADXL313 accelerometer

 Documentation/iio/adxl313.rst    | 196 +++++++++
 Documentation/iio/index.rst      |   1 +
 drivers/iio/accel/adxl313.h      |  35 +-
 drivers/iio/accel/adxl313_core.c | 658 ++++++++++++++++++++++++++++++-
 drivers/iio/accel/adxl313_i2c.c  |   6 +
 drivers/iio/accel/adxl313_spi.c  |   6 +
 6 files changed, 893 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/iio/adxl313.rst

-- 
2.39.5

[PATCH v1 06/12] iio: accel: adxl313: prepare interrupt handling

[Lothar Rubusch]

Evaluate the devicetree property for an optional interrupt line, and
configure the interrupt mapping accordingly. When no interrupt line
is defined in the devicetree, keep the FIFO in bypass mode as before.

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

diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
index ba5b5d53a0ea..c5673f1934fb 100644
--- a/drivers/iio/accel/adxl313.h
+++ b/drivers/iio/accel/adxl313.h
@@ -21,7 +21,9 @@
 #define ADXL313_REG_ACT_INACT_CTL	0x27
 #define ADXL313_REG_BW_RATE		0x2C
 #define ADXL313_REG_POWER_CTL		0x2D
+#define ADXL313_REG_INT_ENABLE		0x2E
 #define ADXL313_REG_INT_MAP		0x2F
+#define ADXL313_REG_INT_SOURCE		0x30
 #define ADXL313_REG_DATA_FORMAT		0x31
 #define ADXL313_REG_DATA_AXIS(index)	(0x32 + ((index) * 2))
 #define ADXL313_REG_FIFO_CTL		0x38
@@ -47,6 +49,11 @@
 #define ADXL313_SPI_3WIRE		BIT(6)
 #define ADXL313_I2C_DISABLE		BIT(6)
 
+#define ADXL313_REG_FIFO_CTL_MODE_MSK		GENMASK(7, 6)
+
+#define ADXL313_FIFO_BYPASS			0
+#define ADXL313_FIFO_STREAM			2
+
 extern const struct regmap_access_table adxl312_readable_regs_table;
 extern const struct regmap_access_table adxl313_readable_regs_table;
 extern const struct regmap_access_table adxl314_readable_regs_table;
@@ -67,6 +74,7 @@ struct adxl313_data {
 	struct regmap	*regmap;
 	const struct adxl313_chip_info *chip_info;
 	struct mutex	lock; /* lock to protect transf_buf */
+	int irq;
 	__le16		transf_buf __aligned(IIO_DMA_MINALIGN);
 };
 
diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
index 244fb2ec0b79..05e99708c2c1 100644
--- a/drivers/iio/accel/adxl313_core.c
+++ b/drivers/iio/accel/adxl313_core.c
@@ -8,11 +8,17 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 
 #include "adxl313.h"
 
+#define ADXL313_INT_NONE			0
+#define ADXL313_INT1				1
+#define ADXL313_INT2				2
+
 static const struct regmap_range adxl312_readable_reg_range[] = {
 	regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
 	regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
@@ -438,6 +444,8 @@ int adxl313_core_probe(struct device *dev,
 {
 	struct adxl313_data *data;
 	struct iio_dev *indio_dev;
+	unsigned int regval;
+	u8 int_line;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -463,6 +471,30 @@ int adxl313_core_probe(struct device *dev,
 		return ret;
 	}
 
+	int_line = ADXL313_INT1;
+	data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT1");
+	if (data->irq < 0) {
+		int_line = ADXL313_INT2;
+		data->irq = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
+		if (data->irq < 0)
+			int_line = ADXL313_INT_NONE;
+	}
+
+	if (int_line) {
+		/* FIFO_STREAM mode */
+		regval = int_line == ADXL313_INT2 ?  0xff : 0;
+		ret = regmap_write(data->regmap, ADXL313_REG_INT_MAP, regval);
+		if (ret)
+			return ret;
+	} else {
+		/* FIFO_BYPASSED mode */
+		ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
+				   FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK,
+					      ADXL313_FIFO_BYPASS));
+		if (ret)
+			return ret;
+	}
+
 	return devm_iio_device_register(dev, indio_dev);
 }
 EXPORT_SYMBOL_NS_GPL(adxl313_core_probe, IIO_ADXL313);
-- 
2.39.5

Re: [PATCH v1 06/12] iio: accel: adxl313: prepare interrupt handling

[Andy Shevchenko]

On Sun, May 18, 2025 at 11:13:15AM +0000, Lothar Rubusch wrote:
> Evaluate the devicetree property for an optional interrupt line, and
> configure the interrupt mapping accordingly. When no interrupt line
> is defined in the devicetree, keep the FIFO in bypass mode as before.

...

> +#define ADXL313_INT_NONE			0

Hmm... I would rather make it U8_MAX, but it's up to you.

> +#define ADXL313_INT1				1
> +#define ADXL313_INT2				2

...

> +		/* FIFO_STREAM mode */
> +		regval = int_line == ADXL313_INT2 ?  0xff : 0;

One space too many.

> +		ret = regmap_write(data->regmap, ADXL313_REG_INT_MAP, regval);

Don't you want to use regmap_assign_bits() or something like this to have
the above ternary be included?

> +		if (ret)
> +			return ret;

-- 
With Best Regards,
Andy Shevchenko