[PATCH v4 0/4] iio: accel: adxl367: add INT2 interrupt pin support

[PATCH v4 0/4] iio: accel: adxl367: add INT2 interrupt pin support

[Antoniu Miclaus]

The ADXL367 exposes two independent interrupt output pins, INT1 and INT2,
each with its own event mapping register. The driver has always hardcoded
INT1, so a board that routes only INT2 to the host never receives
interrupts. This series lets the driver pick the interrupt map register
based on the interrupt-names device tree property.

Changes in v4:
- Collected Reviewed-by tags from Marcelo Schmitt on patches 1 and 3.
- Reworked the interrupt-name lookup in the INT2 patch to propagate errors
  from fwnode_irq_get_byname(): only -EINVAL (no interrupt-names) and
  -ENODATA (requested name not listed) fall back to the INT1 default, so
  -EPROBE_DEFER and other real errors are no longer swallowed.

Antoniu Miclaus (4):
  dt-bindings: iio: update unreachable maintainer entries
  dt-bindings: iio: accel: adi,adxl367: add interrupt-names
  iio: accel: adxl367: use regmap_assign_bits()
  iio: accel: adxl367: add support for INT2 interrupt pin

 .../bindings/iio/accel/adi,adxl367.yaml       | 13 ++++-
 .../bindings/iio/adc/adi,ad4130.yaml          |  3 +-
 .../bindings/iio/addac/adi,ad74115.yaml       |  3 +-
 .../bindings/iio/addac/adi,ad74413r.yaml      |  3 +-
 MAINTAINERS                                   |  4 ++
 drivers/iio/accel/adxl367.c                   | 52 ++++++++++++++++---
 6 files changed, 65 insertions(+), 13 deletions(-)


base-commit: 350d1fb9204b13c5f95e511e98b8bcb47574d425
-- 
2.43.0

[PATCH v4 1/4] dt-bindings: iio: update unreachable maintainer entries

[Antoniu Miclaus]

Cosmin's Analog Devices address is no longer valid, leaving the ADXL367,
AD4130, AD74115 and AD74413R bindings without a reachable maintainer.
Take over maintainership of those bindings and add myself to the
corresponding MAINTAINERS entries alongside Marcelo.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Reviewed-by: Nuno Sá <nuno.sa@analog.com>
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
Changes in v4:
- Collected Reviewed-by from Marcelo Schmitt.

 Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml  | 2 +-
 Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml     | 3 ++-
 Documentation/devicetree/bindings/iio/addac/adi,ad74115.yaml  | 3 ++-
 Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml | 3 ++-
 MAINTAINERS                                                   | 4 ++++
 5 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml
index f10d98d34cb8..29689bc38b90 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml
@@ -7,7 +7,7 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Analog Devices ADXL367 3-Axis Digital Accelerometer
 
 maintainers:
-  - Cosmin Tanislav <cosmin.tanislav@analog.com>
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
 
 description: |
   The ADXL367 is an ultralow power, 3-axis MEMS accelerometer.
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
index cc38617bb829..a4ab6adef1c6 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml
@@ -8,7 +8,8 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Analog Devices AD4130 family ADCs
 
 maintainers:
-  - Cosmin Tanislav <cosmin.tanislav@analog.com>
+  - Marcelo Schmitt <marcelo.schmitt@analog.com>
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
 
 description: |
   Bindings for the Analog Devices AD4130 family ADCs.
diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74115.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74115.yaml
index 2a04906531fb..6aa86cb56032 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74115.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74115.yaml
@@ -7,7 +7,8 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Analog Devices AD74115H device
 
 maintainers:
-  - Cosmin Tanislav <cosmin.tanislav@analog.com>
+  - Marcelo Schmitt <marcelo.schmitt@analog.com>
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
 
 description: |
   The AD74115H is a single-channel software configurable input/output
diff --git a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
index 590ea7936ad7..5318a00e7669 100644
--- a/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
+++ b/Documentation/devicetree/bindings/iio/addac/adi,ad74413r.yaml
@@ -7,7 +7,8 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: Analog Devices AD74412R/AD74413R device
 
 maintainers:
-  - Cosmin Tanislav <cosmin.tanislav@analog.com>
+  - Marcelo Schmitt <marcelo.schmitt@analog.com>
+  - Antoniu Miclaus <antoniu.miclaus@analog.com>
 
 description: |
   The AD74412R and AD74413R are quad-channel software configurable input/output
diff --git a/MAINTAINERS b/MAINTAINERS
index 04fa5322d9f7..ade07e6637bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -641,6 +641,7 @@ F:	drivers/iio/accel/adxl355_spi.c
 
 ADXL367 THREE-AXIS DIGITAL ACCELEROMETER DRIVER
 M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
+M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
@@ -1493,6 +1494,7 @@ F:	drivers/iio/adc/ad4080.c
 
 ANALOG DEVICES INC AD4130 DRIVER
 M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
+M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
@@ -1598,6 +1600,7 @@ F:	drivers/iio/dac/ad7293.c
 
 ANALOG DEVICES INC AD74115 DRIVER
 M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
+M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
@@ -1606,6 +1609,7 @@ F:	drivers/iio/addac/ad74115.c
 
 ANALOG DEVICES INC AD74413R DRIVER
 M:	Marcelo Schmitt <marcelo.schmitt@analog.com>
+M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
 L:	linux-iio@vger.kernel.org
 S:	Supported
 W:	https://ez.analog.com/linux-software-drivers
-- 
2.43.0

[PATCH v4 2/4] dt-bindings: iio: accel: adi,adxl367: add interrupt-names

[Antoniu Miclaus]

The ADXL367 has two independent interrupt output pins, INT1 and INT2,
either of which may be wired to the host. Allow the interrupt line that
is connected to be described by adding an optional interrupt-names
property accepting "INT1" and/or "INT2", and permit a second interrupt
entry. interrupt-names is kept optional so existing device trees that
only specify a bare interrupts property remain valid.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
---
Changes in v4:
- No changes.

 .../devicetree/bindings/iio/accel/adi,adxl367.yaml    | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml
index 29689bc38b90..115170eac058 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl367.yaml
@@ -33,7 +33,14 @@ properties:
     maxItems: 1
 
   interrupts:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
+
+  interrupt-names:
+    minItems: 1
+    items:
+      - enum: [INT1, INT2]
+      - const: INT2
 
   vdd-supply: true
   vddio-supply: true
@@ -61,6 +68,7 @@ examples:
         reg = <0x53>;
         interrupt-parent = <&gpio>;
         interrupts = <25 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "INT1";
       };
     };
   - |
@@ -76,5 +84,6 @@ examples:
         spi-max-frequency = <1000000>;
         interrupt-parent = <&gpio>;
         interrupts = <25 IRQ_TYPE_EDGE_RISING>;
+        interrupt-names = "INT1";
       };
     };
-- 
2.43.0

[PATCH v4 3/4] iio: accel: adxl367: use regmap_assign_bits()

[Antoniu Miclaus]

Several helpers set or clear a fixed set of bits based on a boolean using
the regmap_update_bits(reg, mask, en ? mask : 0) idiom. Replace these with
regmap_assign_bits(), which expresses the same intent more concisely.

No functional change intended.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Reviewed-by: Marcelo Schmitt <marcelo.schmitt@analog.com>
---
Changes in v4:
- Collected Reviewed-by from Marcelo Schmitt.

 drivers/iio/accel/adxl367.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index 4ff1c7a0988b..54c4d1226790 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -366,8 +366,7 @@ static int adxl367_set_act_interrupt_en(struct adxl367_state *st,
 {
 	unsigned int mask = adxl367_act_int_mask_tbl[act];
 
-	return regmap_update_bits(st->regmap, ADXL367_REG_INT1_MAP,
-				  mask, en ? mask : 0);
+	return regmap_assign_bits(st->regmap, ADXL367_REG_INT1_MAP, mask, en);
 }
 
 static int adxl367_get_act_interrupt_en(struct adxl367_state *st,
@@ -401,9 +400,8 @@ static int adxl367_set_act_en(struct adxl367_state *st,
 static int adxl367_set_fifo_watermark_interrupt_en(struct adxl367_state *st,
 						   bool en)
 {
-	return regmap_update_bits(st->regmap, ADXL367_REG_INT1_MAP,
-				  ADXL367_INT_FIFO_WATERMARK_MASK,
-				  en ? ADXL367_INT_FIFO_WATERMARK_MASK : 0);
+	return regmap_assign_bits(st->regmap, ADXL367_REG_INT1_MAP,
+				  ADXL367_INT_FIFO_WATERMARK_MASK, en);
 }
 
 static int adxl367_get_fifo_mode(struct adxl367_state *st,
@@ -631,8 +629,7 @@ static int adxl367_set_odr(struct iio_dev *indio_dev, enum adxl367_odr odr)
 static int adxl367_set_temp_adc_en(struct adxl367_state *st, unsigned int reg,
 				   bool en)
 {
-	return regmap_update_bits(st->regmap, reg, ADXL367_ADC_EN_MASK,
-				  en ? ADXL367_ADC_EN_MASK : 0);
+	return regmap_assign_bits(st->regmap, reg, ADXL367_ADC_EN_MASK, en);
 }
 
 static int adxl367_set_temp_adc_reg_en(struct adxl367_state *st,
-- 
2.43.0

[PATCH v4 4/4] iio: accel: adxl367: add support for INT2 interrupt pin

[Antoniu Miclaus]

The ADXL367 provides two independent interrupt output pins, INT1 and
INT2, each with its own event mapping register (INTMAP1_LOWER at 0x2A
and INTMAP2_LOWER at 0x2B) sharing an identical bit layout. Until now
the driver hardcoded INT1 for all interrupt mappings, so a board that
routes only INT2 to the host could never receive activity, inactivity
or FIFO watermark interrupts.

Determine the connected pin from the interrupt-names device tree
property using fwnode_irq_get_byname(), and route the interrupt
mappings to the matching register. Use the interrupt number returned by
the lookup for devm_request_threaded_irq() so the requested line always
matches the routed INTMAP register, regardless of the order in which the
interrupts are listed. When no interrupt-names are present, default to
INT1 and the bus-provided interrupt to preserve the existing behaviour.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
Changes in v4:
- Propagate errors from fwnode_irq_get_byname() instead of silently
  falling back: only -EINVAL (no interrupt-names) and -ENODATA (requested
  name not listed) select the INT1 default, so -EPROBE_DEFER and other
  errors are returned to the caller.

 drivers/iio/accel/adxl367.c | 45 ++++++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
index 54c4d1226790..9a2312da6238 100644
--- a/drivers/iio/accel/adxl367.c
+++ b/drivers/iio/accel/adxl367.c
@@ -13,6 +13,7 @@
 #include <linux/iio/sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/unaligned.h>
@@ -81,6 +82,7 @@
 #define ADXL367_SAMPLES_L_MASK		GENMASK(7, 0)
 
 #define ADXL367_REG_INT1_MAP		0x2A
+#define ADXL367_REG_INT2_MAP		0x2B
 #define ADXL367_INT_INACT_MASK		BIT(5)
 #define ADXL367_INT_ACT_MASK		BIT(4)
 #define ADXL367_INT_FIFO_WATERMARK_MASK	BIT(2)
@@ -168,6 +170,8 @@ struct adxl367_state {
 	enum adxl367_odr	odr;
 	enum adxl367_range	range;
 
+	u8		int_map_reg;
+
 	unsigned int	act_threshold;
 	unsigned int	act_time_ms;
 	unsigned int	inact_threshold;
@@ -366,7 +370,7 @@ static int adxl367_set_act_interrupt_en(struct adxl367_state *st,
 {
 	unsigned int mask = adxl367_act_int_mask_tbl[act];
 
-	return regmap_assign_bits(st->regmap, ADXL367_REG_INT1_MAP, mask, en);
+	return regmap_assign_bits(st->regmap, st->int_map_reg, mask, en);
 }
 
 static int adxl367_get_act_interrupt_en(struct adxl367_state *st,
@@ -377,7 +381,7 @@ static int adxl367_get_act_interrupt_en(struct adxl367_state *st,
 	unsigned int val;
 	int ret;
 
-	ret = regmap_read(st->regmap, ADXL367_REG_INT1_MAP, &val);
+	ret = regmap_read(st->regmap, st->int_map_reg, &val);
 	if (ret)
 		return ret;
 
@@ -400,7 +404,7 @@ static int adxl367_set_act_en(struct adxl367_state *st,
 static int adxl367_set_fifo_watermark_interrupt_en(struct adxl367_state *st,
 						   bool en)
 {
-	return regmap_assign_bits(st->regmap, ADXL367_REG_INT1_MAP,
+	return regmap_assign_bits(st->regmap, st->int_map_reg,
 				  ADXL367_INT_FIFO_WATERMARK_MASK, en);
 }
 
@@ -1423,6 +1427,36 @@ static int adxl367_setup(struct adxl367_state *st)
 	return adxl367_set_measure_en(st, true);
 }
 
+static int adxl367_set_int_map_reg(struct adxl367_state *st, int irq)
+{
+	int ret;
+
+	/*
+	 * Route the mappings to whichever of INT1/INT2 is named in the
+	 * interrupt-names property. -EINVAL (property absent) and -ENODATA
+	 * (name not listed) are not errors; anything else is.
+	 */
+	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT1");
+	if (ret > 0) {
+		st->int_map_reg = ADXL367_REG_INT1_MAP;
+		return ret;
+	}
+	if (ret != -ENODATA && ret != -EINVAL)
+		return ret;
+
+	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT2");
+	if (ret > 0) {
+		st->int_map_reg = ADXL367_REG_INT2_MAP;
+		return ret;
+	}
+	if (ret != -ENODATA && ret != -EINVAL)
+		return ret;
+
+	/* No INT1/INT2 named: default to INT1 and the bus interrupt. */
+	st->int_map_reg = ADXL367_REG_INT1_MAP;
+	return irq;
+}
+
 int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
 		  void *context, struct regmap *regmap, int irq)
 {
@@ -1479,6 +1513,11 @@ int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
 	if (ret)
 		return ret;
 
+	ret = adxl367_set_int_map_reg(st, irq);
+	if (ret < 0)
+		return dev_err_probe(st->dev, ret, "Failed to get interrupt\n");
+	irq = ret;
+
 	ret = devm_request_threaded_irq(st->dev, irq, NULL,
 					adxl367_irq_handler, IRQF_ONESHOT,
 					indio_dev->name, indio_dev);
-- 
2.43.0

Re: [PATCH v4 4/4] iio: accel: adxl367: add support for INT2 interrupt pin

[Jonathan Cameron]

> The ADXL367 provides two independent interrupt output pins, INT1 and
> INT2, each with its own event mapping register (INTMAP1_LOWER at 0x2A
> and INTMAP2_LOWER at 0x2B) sharing an identical bit layout. Until now
> the driver hardcoded INT1 for all interrupt mappings, so a board that
> routes only INT2 to the host could never receive activity, inactivity
> or FIFO watermark interrupts.
> 
> Determine the connected pin from the interrupt-names device tree
> property using fwnode_irq_get_byname(), and route the interrupt
> mappings to the matching register. Use the interrupt number returned by
> the lookup for devm_request_threaded_irq() so the requested line always
> matches the routed INTMAP register, regardless of the order in which the
> interrupts are listed. When no interrupt-names are present, default to
> INT1 and the bus-provided interrupt to preserve the existing behaviour.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
>

One follow on thing to look at via sashiko.

Otherwise, this looks good to me, but given the active discussion on previous
version I'm not going to pick this up until plenty of time has passed
or tags have been given.

> diff --git a/drivers/iio/accel/adxl367.c b/drivers/iio/accel/adxl367.c
> index b4e8dc6d6a5c..9b47e66c49ea 100644
> --- a/drivers/iio/accel/adxl367.c
> +++ b/drivers/iio/accel/adxl367.c

> @@ -1430,6 +1434,36 @@ static int adxl367_setup(struct adxl367_state *st)
>  	return adxl367_set_measure_en(st, true);
>  }
>  
> +static int adxl367_set_int_map_reg(struct adxl367_state *st, int irq)
> +{
> +	int ret;
> +
> +	/*
> +	 * Route the mappings to whichever of INT1/INT2 is named in the
> +	 * interrupt-names property. -EINVAL (property absent) and -ENODATA
> +	 * (name not listed) are not errors; anything else is.
> +	 */
> +	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT1");
> +	if (ret > 0) {
> +		st->int_map_reg = ADXL367_REG_INT1_MAP;
> +		return ret;
> +	}
> +	if (ret != -ENODATA && ret != -EINVAL)
> +		return ret;
> +
> +	ret = fwnode_irq_get_byname(dev_fwnode(st->dev), "INT2");
> +	if (ret > 0) {
> +		st->int_map_reg = ADXL367_REG_INT2_MAP;
> +		return ret;
> +	}
> +	if (ret != -ENODATA && ret != -EINVAL)
> +		return ret;
> +
> +	/* No INT1/INT2 named: default to INT1 and the bus interrupt. */
> +	st->int_map_reg = ADXL367_REG_INT1_MAP;
> +	return irq;

I'd like to explore the helper idea for this, but it can be a separate
series when someone has the time.

> +}
> +
>  int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
>  		  void *context, struct regmap *regmap, int irq)
>  {
> @@ -1486,6 +1520,11 @@ int adxl367_probe(struct device *dev, const struct adxl367_ops *ops,
>  	if (ret)
>  		return ret;
>  

[Severity: Medium]
Since fwnode_irq_get_byname() inside adxl367_set_int_map_reg() can return
-EPROBE_DEFER if the interrupt controller is not yet ready, will placing this
lookup after adxl367_setup() cause repeated boot delays and leave the device
powered up on deferral?

Looking at adxl367_probe(), it resets the hardware, sleeps for 15ms, and calls
adxl367_setup() which powers up the device into measurement mode.

If adxl367_set_int_map_reg() then returns -EPROBE_DEFER, the probe aborts
without an error cleanup path to power down the device.

Could this software resource acquisition be performed before the hardware
interaction to avoid unnecessary boot stalls and power leaks during probe
deferral?

-

This obviously isn't new, but it might make sense to turn the device
off at least.

Can be a follow up patch.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>