[PATCH v4 0/6] io: accel: mma8452: Allow open drain interrupt pin configuration

[PATCH v4 0/6] io: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

Extend the mma8452 driver with support for configuration of the
interrupt line in open-drain mode, which is needed for hardware designs
where the interrupt line is shared with other chips.

Adding drive-open-drain property to mma8452 device-tree node for such
designs to enable switching pin configuration to open-drain mode.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
Changes in v4:
- Fixed interrupt handler to check runtime PM status before trying to
  access the chip.
- Split open-drain support and interrupt sharing into separate patches.
- Added new patch to reuse existing struct device * through mma8452_probe()
  function.
- Print warning message when irq type is not set by firmware.
- Link to v3: https://patch.msgid.link/20260805-mma8452-open-drain-v3-0-6149f406a409@geanix.com

Changes in v3:
- Reordered patches, swapping #2 and #3.
- Always add IRQF_SHARED flag.
- New patch to change it so IQRF_TRIGGER_LOW flag is only added when no
  trigger type is set by firmware.
- Link to v2: https://patch.msgid.link/20260715-mma8452-open-drain-v2-0-95be9f5f4795@geanix.com

Changes in v2:
- Commit message of patch 2 updated.
- Operator precedence bug fixed in flags argument to
  request_threaded_irq().
- Always check return value of mma8452_set_interrupt_pin_mode(), and just
  check for non-zero value.
- Added new patch with optimization of struct mma8452_data ordering.
- Link to v1: https://patch.msgid.link/20260715-mma8452-open-drain-v1-0-b1dd2a440c60@geanix.com

To: Jonathan Cameron <jic23@kernel.org>
To: Lars-Peter Clausen <lars@metafoo.de>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Martin Kepplinger <martink@posteo.de>
To: Sean Nyekjaer <sean@geanix.com>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Martin Kepplinger <martin.kepplinger@theobroma-systems.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Esben Haabendal (6):
      dt-bindings: iio: accel: mma8452: Add drive-open-drain
      iio: accel: mma8452: Optimize struct mm8452_data member orders
      iio: accel: mma8452: Only apply trigger type when not set by firmware
      iio: accel: mma8452: Support interrupt sharing
      iio: accel: mma8452: Allow open drain interrupt pin configuration
      iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

 .../devicetree/bindings/iio/accel/fsl,mma8452.yaml |  6 ++
 drivers/iio/accel/mma8452.c                        | 90 ++++++++++++++++------
 2 files changed, 73 insertions(+), 23 deletions(-)
---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
change-id: 20250401-mma8452-open-drain-81577c41375c

Best regards,
--  
Esben Haabendal <esben@geanix.com>

[PATCH v4 1/6] dt-bindings: iio: accel: mma8452: Add drive-open-drain

[Esben Haabendal]

Add new boolean to configure selected interrupt pin to open drain instead
of the default push-pull mode.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
index b0dd2b4e116a..20701aa725d0 100644
--- a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
@@ -39,6 +39,12 @@ properties:
     minItems: 1
     maxItems: 2
 
+  drive-open-drain:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description: the interrupt line will be configured as open drain, which is
+      useful if several sensors share the same interrupt line. (This binding is
+      taken from pinctrl.)
+
   vdd-supply: true
   vddio-supply: true
 

-- 
2.55.0

Re: [PATCH v4 1/6] dt-bindings: iio: accel: mma8452: Add drive-open-drain

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete Implementation: The `drive-open-drain` property is added to the Device Tree bindings, but the corresponding driver implementation to actually configure the hardware is entirely missing.
--

--- Patch [1]: [PATCH v4 1/6] dt-bindings: iio: accel: mma8452: Add drive-open-drain ---
commit 43db06f651ed5305e7ec90f57f0128f294c32c55
Author: Esben Haabendal <esben@geanix.com>

dt-bindings: iio: accel: mma8452: Add drive-open-drain

This commit adds a new drive-open-drain boolean property to the MMA8452
device tree binding. This allows configuring the interrupt pin to open drain
mode instead of the default push-pull mode.

> diff --git a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
> index b0dd2b4e116ab..20701aa725d07 100644
> --- a/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/fsl,mma8452.yaml
> @@ -39,6 +39,12 @@ properties:
>      minItems: 1
>      maxItems: 2
>  
> +  drive-open-drain:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: the interrupt line will be configured as open drain, which is
> +      useful if several sensors share the same interrupt line. (This binding is
> +      taken from pinctrl.)

[Severity: High]
Is the corresponding driver implementation to actually configure the hardware
missing?

If a system integrator relies on this new property and physically wires
multiple sensor interrupts together, the missing driver implementation means
the sensors will silently remain in push-pull mode.

Looking at mma8452_probe() in drivers/iio/accel/mma8452.c, there appears
to be no logic to read drive-open-drain via device_property_read_bool()
and configure the sensor's CTRL_REG3 register (0x2C) accordingly.

If two push-pull outputs on a shared line drive different logic levels
simultaneously, can this create a short circuit that physically damages the
board?

> +
>    vdd-supply: true
>    vddio-supply: true

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com?part=1

[PATCH v4 2/6] iio: accel: mma8452: Optimize struct mm8452_data member orders

[Esben Haabendal]

Reorder struct mma8452_data members to avoid holes.

Reviewed-by: Joshua Crofts <joshua.crofts1@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7d683686dd9d..f645a5c6fd1c 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -106,10 +106,7 @@ struct mma8452_data {
 	struct i2c_client *client;
 	struct mutex lock;
 	struct iio_mount_matrix orientation;
-	u8 ctrl_reg1;
-	u8 data_cfg;
 	const struct mma_chip_info *chip_info;
-	int sleep_val;
 	struct regulator *vdd_reg;
 	struct regulator *vddio_reg;
 
@@ -118,6 +115,10 @@ struct mma8452_data {
 		__be16 channels[3];
 		aligned_s64 ts;
 	} buffer;
+
+	int sleep_val;
+	u8 ctrl_reg1;
+	u8 data_cfg;
 };
 
  /**

-- 
2.55.0

Re: [PATCH v4 2/6] iio: accel: mma8452: Optimize struct mm8452_data member orders

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] AB-BA deadlock between the driver's data->lock and the PM runtime core synchronization.
- [High] Short I2C reads leak uninitialized kernel stack memory to userspace.
--

--- Patch [2]: [PATCH v4 2/6] iio: accel: mma8452: Optimize struct mm8452_data member orders ---
commit 6be65aa63f260fa43ab647c9d5c94e2ecf3e69dd
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Optimize struct mm8452_data member orders

Reorder struct mma8452_data members to avoid holes.

> iio: accel: mma8452: Optimize struct mm8452_data member orders

This isn't a bug, but there's a typo (mm8452) in the subject line.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..f645a5c6fd1c2 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -106,10 +106,7 @@ struct mma8452_data {
>  	struct i2c_client *client;
>  	struct mutex lock;

[Severity: High]
This is a pre-existing issue, but is there an AB-BA deadlock between the
driver's lock and the PM runtime core synchronization?

If a sysfs read calls mma8452_read_raw(), which acquires the lock:

mma8452_read_raw()
    mutex_lock(&data->lock);
    ret = mma8452_read(data, buffer);

And then mma8452_read() attempts to acquire a PM runtime reference by calling
pm_runtime_resume_and_get(), it blocks waiting for a suspend operation to
complete if the device is auto-suspending.

Concurrently, if the PM autosuspend timer expires, the PM core executes the
driver's suspend callback:

mma8452_runtime_suspend()
    mutex_lock(&data->lock);
    ret = mma8452_standby(data);
    mutex_unlock(&data->lock);

Could this result in a permanent deadlock if the sysfs read thread holds the
lock while waiting for the PM core, which is in turn waiting for the lock in
mma8452_runtime_suspend()?

>  	struct iio_mount_matrix orientation;
> -	u8 ctrl_reg1;
> -	u8 data_cfg;
>  	const struct mma_chip_info *chip_info;
> -	int sleep_val;
>  	struct regulator *vdd_reg;
>  	struct regulator *vddio_reg;
>  
> @@ -118,6 +115,10 @@ struct mma8452_data {
>  		__be16 channels[3];
>  		aligned_s64 ts;
>  	} buffer;

[Severity: High]
This isn't a bug introduced by this patch, but do short I2C reads leak
uninitialized kernel stack memory to userspace here?

In mma8452_read_raw(), an uninitialized buffer is allocated on the stack and
passed to mma8452_read():

mma8452_read_raw()
    __be16 buffer[3];
    ...
    ret = mma8452_read(data, buffer);

In mma8452_read(), an I2C read is performed:

mma8452_read()
    ret = i2c_smbus_read_i2c_block_data(data->client, MMA8452_OUT_X,
                                        3 * sizeof(__be16), (u8 *)buf);
    if (ret < 0)
        return ret;

    ret = mma8452_set_runtime_pm_state(data->client, false);

    return ret;

If the I2C device returns fewer bytes than expected (a short read),
i2c_smbus_read_i2c_block_data() returns a positive integer less than 6.

The code only checks for ret < 0, and then overwrites the short read length by
returning the result of mma8452_set_runtime_pm_state(), which is 0 on success.

Does this hide the short read from the caller, causing mma8452_read_raw() to
extract a value from uninitialized stack memory and return it to userspace via
the sysfs interface?

> +
> +	int sleep_val;
> +	u8 ctrl_reg1;
> +	u8 data_cfg;
>  };
>  
>   /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com?part=2

[PATCH v4 3/6] iio: accel: mma8452: Only apply trigger type when not set by firmware

[Esben Haabendal]

Instead of unconditionally overriding the trigger type, it is better to
only apply a default when no trigger type is set by firmware. This should
be reasonably backward compatible, and should only potentially cause
problems if systems exist where firmware specifies an incorrect trigger
type. With a bit of luck, there are no such systems.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index f645a5c6fd1c..1fb43c5b0b72 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1683,9 +1683,16 @@ static int mma8452_probe(struct i2c_client *client)
 		goto trigger_cleanup;
 
 	if (client->irq) {
+		unsigned long irq_flags;
+
+		irq_flags = irq_get_trigger_type(client->irq);
+		if (irq_flags == IRQ_TYPE_NONE) {
+			dev_info(dev, "invalid irq type, setting default active low\n");
+			irq_flags = IRQF_TRIGGER_LOW;
+		}
+		irq_flags |= IRQF_ONESHOT;
 		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
-					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
-					   client->name, indio_dev);
+					   irq_flags, client->name, indio_dev);
 		if (ret)
 			goto buffer_cleanup;
 	}

-- 
2.55.0

Re: [PATCH v4 3/6] iio: accel: mma8452: Only apply trigger type when not set by firmware

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unvalidated and unconfigured IRQ trigger types from firmware cause interrupt polarity mismatch or hardware stalls.
--

--- Patch [3]: [PATCH v4 3/6] iio: accel: mma8452: Only apply trigger type when not set by firmware ---
commit 6463a4aeb8274414532a847585e92a3f1c4195b9
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Only apply trigger type when not set by firmware

This patch modifies the driver to use the IRQ trigger type specified by
the firmware instead of unconditionally forcing an active-low trigger. It
falls back to active-low if the firmware does not provide a valid trigger
type.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index f645a5c6fd1c2..1fb43c5b0b72f 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1683,9 +1683,16 @@ static int mma8452_probe(struct i2c_client *client)
>  		goto trigger_cleanup;
>  
>  	if (client->irq) {
> +		unsigned long irq_flags;
> +
> +		irq_flags = irq_get_trigger_type(client->irq);
> +		if (irq_flags == IRQ_TYPE_NONE) {
> +			dev_info(dev, "invalid irq type, setting default active low\n");
> +			irq_flags = IRQF_TRIGGER_LOW;
> +		}
> +		irq_flags |= IRQF_ONESHOT;
>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> -					   client->name, indio_dev);
> +					   irq_flags, client->name, indio_dev);

[Severity: High]
Will this cause an interrupt polarity mismatch if the firmware specifies
an active-high trigger?

Looking at mma8452_probe(), the driver calls mma8452_reset() earlier,
which forces CTRL_REG3 (and its IPOL bit) to the hardware default of
active low.

If the firmware specifies IRQ_TYPE_LEVEL_HIGH, the OS interrupt subsystem
is configured for active-high, but the sensor continues to assert active-low
because CTRL_REG3 is never reprogrammed to match the firmware trigger type.

Additionally, can passing edge triggers (like IRQ_TYPE_EDGE_FALLING) to a
strictly level-triggered I2C sensor cause a missed-edge race condition?
If a second event occurs before the interrupt line is deasserted, this could
permanently stall the sensor.

>  		if (ret)
>  			goto buffer_cleanup;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com?part=3

[PATCH v4 4/6] iio: accel: mma8452: Support interrupt sharing

[Esben Haabendal]

Adding handling of rutnime PM suspension in the interrupt handler allows
sharing interrupt with other devices.

Keep in mind that the device by default is using push-pull for the irq pin,
which might require additional hardware design to allow interrupt sharing.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 1fb43c5b0b72..106cdd904dbf 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1056,14 +1056,19 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 	struct iio_dev *indio_dev = p;
 	struct mma8452_data *data = iio_priv(indio_dev);
 	irqreturn_t ret = IRQ_NONE;
+	int pm_status;
 	int src;
 
+	pm_status = pm_runtime_get_if_active(&data->client->dev);
+	if (pm_status == 0)
+		return IRQ_NONE; /* device is powered down */
+
 	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
 	if (src < 0)
-		return IRQ_NONE;
+		goto out;
 
 	if (!(src & (data->chip_info->enabled_events | MMA8452_INT_DRDY)))
-		return IRQ_NONE;
+		goto out;
 
 	if (src & MMA8452_INT_DRDY) {
 		iio_trigger_poll_nested(indio_dev->trig);
@@ -1089,6 +1094,10 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
 		ret = IRQ_HANDLED;
 	}
 
+out:
+	if (pm_status > 0)
+		pm_runtime_put_autosuspend(&data->client->dev);
+
 	return ret;
 }
 
@@ -1690,7 +1699,7 @@ static int mma8452_probe(struct i2c_client *client)
 			dev_info(dev, "invalid irq type, setting default active low\n");
 			irq_flags = IRQF_TRIGGER_LOW;
 		}
-		irq_flags |= IRQF_ONESHOT;
+		irq_flags |= IRQF_ONESHOT | IRQF_SHARED;
 		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
 					   irq_flags, client->name, indio_dev);
 		if (ret)

-- 
2.55.0

Re: [PATCH v4 4/6] iio: accel: mma8452: Support interrupt sharing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The interrupt handler incorrectly checks `pm_status == 0` instead of `<= 0`, leading to unpowered hardware access when Runtime PM is disabled.
- [Medium] The runtime suspend callback lacks a call to synchronize_irq() before powering down the hardware, violating PM subsystem guidelines for devices using shared IRQs.
--

--- Patch [4]: [PATCH v4 4/6] iio: accel: mma8452: Support interrupt sharing ---
commit ae80b7f558736b5e80920be8e31037e5cc8645ba
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Support interrupt sharing

This commit adds handling of runtime PM suspension in the interrupt handler.
It also adds the IRQF_SHARED flag to the interrupt request to allow sharing
the interrupt with other devices.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 1fb43c5b0b72f..106cdd904dbfd 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1056,14 +1056,19 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>  	struct iio_dev *indio_dev = p;
>  	struct mma8452_data *data = iio_priv(indio_dev);
>  	irqreturn_t ret = IRQ_NONE;
> +	int pm_status;
>  	int src;
>  
> +	pm_status = pm_runtime_get_if_active(&data->client->dev);
> +	if (pm_status == 0)
> +		return IRQ_NONE; /* device is powered down */
> +

[Severity: High]
Does this correctly handle the case where runtime PM is disabled?

When pm_runtime_disable() is called during device removal,
pm_runtime_get_if_active() can return -EINVAL.

If a shared interrupt fires from another device during this window, the
handler will see a negative return value. Since the check is explicitly
for == 0, the handler will proceed and attempt to read from the I2C bus while
the device regulators might be disabled.

Should this check be <= 0 instead?

>  	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>  	if (src < 0)
> -		return IRQ_NONE;
> +		goto out;

[ ... ]

> @@ -1690,7 +1699,7 @@ static int mma8452_probe(struct i2c_client *client)
>  			dev_info(dev, "invalid irq type, setting default active low\n");
>  			irq_flags = IRQF_TRIGGER_LOW;
>  		}
> -		irq_flags |= IRQF_ONESHOT;
> +		irq_flags |= IRQF_ONESHOT | IRQF_SHARED;

[Severity: Medium]
With the addition of IRQF_SHARED, is synchronize_irq() needed in the runtime
suspend path?

The PM subsystem guidelines require that drivers using shared interrupts must
call synchronize_irq() in their runtime suspend callback before powering down
hardware. This guarantees no interrupt handler is executing mid-flight when
the regulators are disabled.

Would it be safer to add this to mma8452_runtime_suspend() before the device
is placed in standby?

>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
>  					   irq_flags, client->name, indio_dev);
>  		if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com?part=4

[PATCH v4 5/6] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

When designing systems sharing the interrupt for mma8452 chips, it is
helpful to be able to configure the irq pin in open-drain mode (default is
push-pull).

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 106cdd904dbf..7992f9e38818 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -81,6 +81,8 @@
 #define  MMA8452_CTRL_REG2_RST			BIT(6)
 #define  MMA8452_CTRL_REG2_MODS_SHIFT		3
 #define  MMA8452_CTRL_REG2_MODS_MASK		0x1b
+#define MMA8452_CTRL_REG3			0x2c
+#define  MMA8452_CTRL_REG3_PP_OD		BIT(0)
 #define MMA8452_CTRL_REG4			0x2d
 #define MMA8452_CTRL_REG5			0x2e
 #define MMA8452_OFF_X				0x2f
@@ -119,6 +121,7 @@ struct mma8452_data {
 	int sleep_val;
 	u8 ctrl_reg1;
 	u8 data_cfg;
+	bool open_drain;
 };
 
  /**
@@ -647,6 +650,22 @@ static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
 	return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
 }
 
+static int mma8452_set_interrupt_pin_mode(struct mma8452_data *data)
+{
+	int reg;
+
+	reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG3);
+	if (reg < 0)
+		return reg;
+
+	if (data->open_drain)
+		reg |= MMA8452_CTRL_REG3_PP_OD;
+	else
+		reg &= ~MMA8452_CTRL_REG3_PP_OD;
+
+	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG3, reg);
+}
+
 /* returns >0 if in freefall mode, 0 if not or <0 if an error occurred */
 static int mma8452_freefall_mode_enabled(struct mma8452_data *data)
 {
@@ -1676,6 +1695,11 @@ static int mma8452_probe(struct i2c_client *client)
 			goto disable_regulators;
 	}
 
+	data->open_drain = device_property_read_bool(dev, "drive-open-drain");
+	ret = mma8452_set_interrupt_pin_mode(data);
+	if (ret)
+		goto trigger_cleanup;
+
 	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
 			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
 
@@ -1817,6 +1841,10 @@ static int mma8452_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	ret = mma8452_set_interrupt_pin_mode(data);
+	if (ret)
+		goto runtime_resume_failed;
+
 	ret = mma8452_active(data);
 	if (ret < 0)
 		goto runtime_resume_failed;

-- 
2.55.0

Re: [PATCH v4 5/6] iio: accel: mma8452: Allow open drain interrupt pin configuration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] The PM runtime resume handler fails to restore most device configuration registers after a power cycle, reverting to POR defaults.
- [High] The driver ignores the requested interrupt polarity from irq_get_trigger_type() and fails to configure the hardware's WAKE_INT_POL bit.
- [High] Sysfs read/write handlers modify device registers and poll STATUS without acquiring a PM runtime reference.
--

--- Patch [5]: [PATCH v4 5/6] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
commit 7603b79afdeeb3c4d36936afa7e16bc8d738683c
Author: Esben Haabendal <esben@geanix.com>

    iio: accel: mma8452: Allow open drain interrupt pin configuration
    
    When designing systems sharing the interrupt for mma8452 chips, it is
    helpful to be able to configure the irq pin in open-drain mode (default is
    push-pull).
    
    Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
    Signed-off-by: Esben Haabendal <esben@geanix.com>

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 106cdd904dbfd..7992f9e388180 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
[ ... ]
> @@ -647,6 +650,22 @@ static int mma8452_set_power_mode(struct mma8452_data *data, u8 mode)
>  	return mma8452_change_config(data, MMA8452_CTRL_REG2, reg);
>  }
>  
> +static int mma8452_set_interrupt_pin_mode(struct mma8452_data *data)
> +{
> +	int reg;
> +
> +	reg = i2c_smbus_read_byte_data(data->client, MMA8452_CTRL_REG3);
> +	if (reg < 0)
> +		return reg;
> +
> +	if (data->open_drain)
> +		reg |= MMA8452_CTRL_REG3_PP_OD;
> +	else
> +		reg &= ~MMA8452_CTRL_REG3_PP_OD;

[Severity: High]
This is a pre-existing issue, but could this function also configure the
interrupt polarity bit to match the requested irq polarity?

If a board's device tree specifies an active-high interrupt, the driver
requests the irq with that flag but leaves the chip generating active-low
signals.

Since this function modifies the open-drain setting in MMA8452_CTRL_REG3,
might it also ensure the polarity bit is set correctly so we don't end up
with missing interrupts or continuous interrupt storms?

> +
> +	return i2c_smbus_write_byte_data(data->client, MMA8452_CTRL_REG3, reg);
> +}
> +
[ ... ]
> @@ -1817,6 +1841,10 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = mma8452_set_interrupt_pin_mode(data);
> +	if (ret)
> +		goto runtime_resume_failed;
> +
>  	ret = mma8452_active(data);

[Severity: High]
This is a pre-existing issue, but does mma8452_runtime_resume() fully restore
the device state after a power cycle?

When the device goes to runtime suspend after a period of inactivity,
mma8452_runtime_suspend() disables the vdd and vddio regulators, causing the
hardware to lose all its register state.

When it wakes up, only MMA8452_CTRL_REG1 and MMA8452_CTRL_REG3 are restored
here. Does this leave crucial configurations like MMA8452_DATA_CFG,
MMA8452_CTRL_REG4, and MMA8452_CTRL_REG5 uninitialized and revert to power-on
defaults?

Could this cause the device to operate with incorrect full-scale settings and
disabled interrupts?

>  	if (ret < 0)
>  		goto runtime_resume_failed;


[Severity: High]
This is a pre-existing issue, but do the sysfs read/write handlers modify
device registers and poll the status without acquiring a pm runtime reference?

For example, looking at mma8452_read() in drivers/iio/accel/mma8452.c:

mma8452_read() {
    ...
    int ret = mma8452_drdy(data);

    if (ret < 0)
        return ret;

    ret = mma8452_set_runtime_pm_state(data->client, true);
    ...
}

The driver polls the status register before waking up the device.
Since the device enters autosuspend and turns off its regulators, will any
subsequent sysfs read or write attempt to access an unpowered chip?

Could this result in bus errors and completely break the sysfs interface?

Also, __mma8452_write_raw() calls into mma8452_change_config() which reads
from the device without holding a pm runtime reference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com?part=5

[PATCH v4 6/6] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

[Esben Haabendal]

In commit 32a5c04d4575 ("iio: accel: mma8452: Use dev_err_probe()") the
struct device * pointer was assigned to local variable dev, so we can just
as well reuse that throughout the function for sligthly more readable code.

Signed-off-by: Esben Haabendal <esben@geanix.com>
---
 drivers/iio/accel/mma8452.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7992f9e38818..2fbf9f9ed9d7 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1581,7 +1581,7 @@ static int mma8452_probe(struct i2c_client *client)
 	struct iio_dev *indio_dev;
 	int ret;
 
-	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1591,21 +1591,21 @@ static int mma8452_probe(struct i2c_client *client)
 
 	data->chip_info = i2c_get_match_data(client);
 	if (!data->chip_info)
-		return dev_err_probe(&client->dev, -ENODEV,
+		return dev_err_probe(dev, -ENODEV,
 				     "unknown device model\n");
 
-	ret = iio_read_mount_matrix(&client->dev, &data->orientation);
+	ret = iio_read_mount_matrix(dev, &data->orientation);
 	if (ret)
 		return ret;
 
-	data->vdd_reg = devm_regulator_get(&client->dev, "vdd");
+	data->vdd_reg = devm_regulator_get(dev, "vdd");
 	if (IS_ERR(data->vdd_reg))
-		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_reg),
+		return dev_err_probe(dev, PTR_ERR(data->vdd_reg),
 				     "failed to get VDD regulator!\n");
 
-	data->vddio_reg = devm_regulator_get(&client->dev, "vddio");
+	data->vddio_reg = devm_regulator_get(dev, "vddio");
 	if (IS_ERR(data->vddio_reg))
-		return dev_err_probe(&client->dev, PTR_ERR(data->vddio_reg),
+		return dev_err_probe(dev, PTR_ERR(data->vddio_reg),
 				     "failed to get VDDIO regulator!\n");
 
 	ret = regulator_enable(data->vdd_reg);
@@ -1637,7 +1637,7 @@ static int mma8452_probe(struct i2c_client *client)
 		goto disable_regulators;
 	}
 
-	dev_info(&client->dev, "registering %s accelerometer; ID 0x%x\n",
+	dev_info(dev, "registering %s accelerometer; ID 0x%x\n",
 		 data->chip_info->name, data->chip_info->chip_id);
 
 	i2c_set_clientdata(client, indio_dev);
@@ -1670,10 +1670,10 @@ static int mma8452_probe(struct i2c_client *client)
 	if (client->irq) {
 		int irq2;
 
-		irq2 = fwnode_irq_get_byname(dev_fwnode(&client->dev), "INT2");
+		irq2 = fwnode_irq_get_byname(dev_fwnode(dev), "INT2");
 
 		if (irq2 == client->irq) {
-			dev_dbg(&client->dev, "using interrupt line INT2\n");
+			dev_dbg(dev, "using interrupt line INT2\n");
 		} else {
 			ret = i2c_smbus_write_byte_data(client,
 							MMA8452_CTRL_REG5,
@@ -1681,7 +1681,7 @@ static int mma8452_probe(struct i2c_client *client)
 			if (ret < 0)
 				goto disable_regulators;
 
-			dev_dbg(&client->dev, "using interrupt line INT1\n");
+			dev_dbg(dev, "using interrupt line INT1\n");
 		}
 
 		ret = i2c_smbus_write_byte_data(client,
@@ -1730,14 +1730,13 @@ static int mma8452_probe(struct i2c_client *client)
 			goto buffer_cleanup;
 	}
 
-	ret = pm_runtime_set_active(&client->dev);
+	ret = pm_runtime_set_active(dev);
 	if (ret < 0)
 		goto free_irq;
 
-	pm_runtime_enable(&client->dev);
-	pm_runtime_set_autosuspend_delay(&client->dev,
-					 MMA8452_AUTO_SUSPEND_DELAY_MS);
-	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, MMA8452_AUTO_SUSPEND_DELAY_MS);
+	pm_runtime_use_autosuspend(dev);
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)

-- 
2.55.0

Re: [PATCH v4 6/6] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing synchronize_irq() in runtime suspend callback for a shared interrupt.
- [High] The IRQ handler incorrectly proceeds with I2C reads when the device's PM runtime is disabled.
- [High] Unprotected I2C hardware accesses in IIO callbacks without waking the device via PM runtime.
--

--- Patch [6]: [PATCH v4 6/6] iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe() ---
commit b1acc18af426345046769c66c8ed674e254ba4d0
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Reuse existing dev pointer in mma8452_probe()

This commit updates mma8452_probe() to reuse the existing local dev pointer
instead of repeatedly dereferencing client->dev. This simplifies the code
and improves readability.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7992f9e388180..2fbf9f9ed9d75 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed an issue in
mma8452_runtime_suspend():

    regulator_disable(data->vddio_reg);
    regulator_disable(data->vdd_reg);

The device requests an interrupt with IRQF_SHARED, but regulators are disabled
without waiting for shared IRQ handlers to complete.

Could an interrupt fire for another device on the same line, causing the
mma8452_interrupt() handler to execute while this device is physically
unpowered? Should we add synchronize_irq() before disabling the regulators?


[Severity: High]
This isn't a bug introduced by this patch, but looking at mma8452_interrupt():

    pm_status = pm_runtime_get_if_active(&data->client->dev);
    if (pm_status == 0)
        return IRQ_NONE; /* device is powered down */

The strict check for 0 ignores the -EINVAL error returned when PM runtime is
disabled. If pm_runtime_disable() is called before free_irq() during driver
removal, pm_runtime_get_if_active() returns -EINVAL.

Does this cause the handler to incorrectly treat -EINVAL as active and proceed
to issue I2C transactions to an unpowered device?


[Severity: High]
This isn't a bug introduced by this patch, but I noticed unprotected I2C
hardware accesses in some IIO callbacks without waking the device via PM
runtime.

In mma8452_read_raw():

    case IIO_CHAN_INFO_CALIBBIAS:
        ret = i2c_smbus_read_byte_data(data->client,
                                       MMA8452_OFF_X +
                                       chan->scan_index);

And in mma8452_read_event_config():

    case IIO_EV_DIR_RISING:
        ret = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);

These branches perform direct I2C reads without first invoking
pm_runtime_resume_and_get() to ensure the device is powered.

If userspace reads corresponding sysfs attributes while the device is in
auto-suspend, will this result in I/O errors and bus timeouts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812-mma8452-open-drain-v4-0-bfca15d02b59@geanix.com?part=6