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

[PATCH 0/2] 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>
---
Esben Haabendal (2):
      dt-bindings: iio: accel: mma8452: Add drive-open-drain
      iio: accel: mma8452: Allow open drain interrupt pin configuration

 .../devicetree/bindings/iio/accel/fsl,mma8452.yaml |  6 +++++
 drivers/iio/accel/mma8452.c                        | 29 +++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)
---
base-commit: a13c140cc289c0b7b3770bce5b3ad42ab35074aa
change-id: 20250401-mma8452-open-drain-81577c41375c

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

[PATCH 1/2] 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.

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

[PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

When sharing interrupt line with other chips, the interrupt pin most
likely needs to be configured in open-drain mode instead of push-pull.
If this is needed, you must add drive-open-drain property to the
device-tree.

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

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 7d683686dd9d..a20c02ce0b9c 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
@@ -108,6 +110,7 @@ struct mma8452_data {
 	struct iio_mount_matrix orientation;
 	u8 ctrl_reg1;
 	u8 data_cfg;
+	bool open_drain;
 	const struct mma_chip_info *chip_info;
 	int sleep_val;
 	struct regulator *vdd_reg;
@@ -646,6 +649,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)
 {
@@ -1666,6 +1685,9 @@ static int mma8452_probe(struct i2c_client *client)
 			goto disable_regulators;
 	}
 
+	data->open_drain = device_property_read_bool(&client->dev, "drive-open-drain");
+	mma8452_set_interrupt_pin_mode(data);
+
 	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
 			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
 
@@ -1683,7 +1705,8 @@ static int mma8452_probe(struct i2c_client *client)
 
 	if (client->irq) {
 		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
-					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
+					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
+					   data->open_drain ? IRQF_SHARED : 0,
 					   client->name, indio_dev);
 		if (ret)
 			goto buffer_cleanup;
@@ -1800,6 +1823,10 @@ static int mma8452_runtime_resume(struct device *dev)
 		return ret;
 	}
 
+	ret = mma8452_set_interrupt_pin_mode(data);
+	if (ret < 0)
+		goto runtime_resume_failed;
+
 	ret = mma8452_active(data);
 	if (ret < 0)
 		goto runtime_resume_failed;

-- 
2.55.0

Re: [PATCH 1/2] 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] The commit introduces the `drive-open-drain` device tree property to configure the sensor's interrupt pin as open drain, but fails to provide the necessary C driver implementation in `mma8452.c` to parse the property and write to the chip's control register.
--

--- Patch [1]: [PATCH 1/2] dt-bindings: iio: accel: mma8452: Add drive-open-drain ---
commit 3050ccf4ccce6dad107c37d81a589ca6d501304b
Author: Esben Haabendal <esben@geanix.com>
Subject: dt-bindings: iio: accel: mma8452: Add drive-open-drain

This commit adds the drive-open-drain property to the mma8452 device tree
binding. This allows configuring the sensor's interrupt pin as open drain
rather than 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
> --- 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 there a missing corresponding driver update to support this new property?

Looking at mma8452_probe() in drivers/iio/accel/mma8452.c, the driver does not
appear to query this property or program the hardware control register to actually
enable the open-drain mode:

drivers/iio/accel/mma8452.c:mma8452_probe() {
    ...
    /* Missing device_property_read_bool() for drive-open-drain */
    ...
}

If device tree authors use this property on a shared interrupt line expecting
it to be honored, but the driver silently leaves the sensor in the default
push-pull mode, could this cause a short circuit when one device drives the
shared line high while another drives it low?

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

Re: [PATCH 2/2] 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:

New issues:
- [High] Operator precedence bug in irqflags expression strips mandatory IRQ flags, causing probe to fail unconditionally.
- [Medium] Ignored return value of mma8452_set_interrupt_pin_mode() in probe can lead to silent hardware misconfiguration.

Pre-existing issues:
- [High] Critical hardware configuration registers are not restored during runtime resume, breaking device operation.
--

--- Patch [2]: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration ---
commit 1663cfc0dcf3fd1e1271945ad142ff5a049c2fc1
Author: Esben Haabendal <esben@geanix.com>

iio: accel: mma8452: Allow open drain interrupt pin configuration
    
When sharing interrupt line with other chips, the interrupt pin most
likely needs to be configured in open-drain mode instead of push-pull.
If this is needed, you must add drive-open-drain property to the
device-tree.

> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d6..a20c02ce0b9cd 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c

[ ... ]

> @@ -1666,6 +1685,9 @@ static int mma8452_probe(struct i2c_client *client)
>  			goto disable_regulators;
>  	}
>  
> +	data->open_drain = device_property_read_bool(&client->dev, "drive-open-drain");
> +	mma8452_set_interrupt_pin_mode(data);

[Severity: Medium]
Is it safe to ignore the return value of mma8452_set_interrupt_pin_mode()
here? 

If the underlying I2C transaction fails and returns an error like -EREMOTEIO,
the hardware interrupt pin will silently remain in push-pull mode instead of
being configured for open-drain. On a shared bus, this could lead to 
electrical contention.

> +
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>  			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>  
> @@ -1683,7 +1705,8 @@ static int mma8452_probe(struct i2c_client *client)
>  
>  	if (client->irq) {
>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
> +					   data->open_drain ? IRQF_SHARED : 0,

[Severity: High]
Does this expression evaluate the way it is intended? 

In C, the bitwise OR operator (|) has higher precedence than the ternary 
operator (?:). This means the flags expression evaluates as:

(IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain) ? IRQF_SHARED : 0

Since the condition part of the expression is always non-zero, this evaluates
strictly to IRQF_SHARED (0x80), entirely stripping away IRQF_TRIGGER_LOW and
IRQF_ONESHOT. 

Because IRQF_ONESHOT is mandatory for threaded IRQs without a primary handler,
request_threaded_irq() will reject the call with -EINVAL, causing the probe
function to unconditionally fail for any device using an interrupt.

>  					   client->name, indio_dev);
>  		if (ret)
>  			goto buffer_cleanup;

[ ... ]

> @@ -1800,6 +1823,10 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = mma8452_set_interrupt_pin_mode(data);
> +	if (ret < 0)
> +		goto runtime_resume_failed;
> +
>  	ret = mma8452_active(data);

[Severity: High]
This is a pre-existing issue, but since this patch modifies the runtime
resume handler, is the device state fully restored here?

The driver uses runtime PM, and mma8452_runtime_suspend() disables vdd_reg,
the main power supply. When power is removed, all volatile registers revert
to their power-on defaults. 

During mma8452_runtime_resume(), power is restored, but it appears the driver
only reconfigures CTRL_REG1 and CTRL_REG3.

Are vital registers configured in probe(), such as MMA8452_DATA_CFG (full
scale), MMA8452_CTRL_REG4 (interrupt enables), MMA8452_CTRL_REG5 (interrupt
routing), and MMA8452_TRANSIENT_THS missed during this resume? If so, the
device might stop producing interrupts and return invalid data after the first
autosuspend delay.

>  	if (ret < 0)
>  		goto runtime_resume_failed;

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

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Andy Shevchenko]

On Wed, Jul 15, 2026 at 10:07:39AM +0200, Esben Haabendal wrote:
> When sharing interrupt line with other chips, the interrupt pin most
> likely needs to be configured in open-drain mode instead of push-pull.
> If this is needed, you must add drive-open-drain property to the
> device-tree.

...

>  	if (client->irq) {
>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
> +					   data->open_drain ? IRQF_SHARED : 0,
>  					   client->name, indio_dev);

Why do we care? The (hidden) problem this will have in the future is that
the IRQ core will splat a warning in case that other shared IRQs might be
configured with different flags. Putting that flag conditionally makes it
a mine field for the users. Instead just unconditionally add that flag and
we will get reports as soon as there will be a user that shares the same
interrupt pin with some other devices which drivers do not use the same
settings.

Also setting to _LOW in the flags unconditionally is a (historic) bug.
The problem is that we might not fix it without breaking the existing
users which omit that flag in DT.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Joshua Crofts]

On Wed, 15 Jul 2026 10:07:39 +0200
Esben Haabendal <esben@geanix.com> wrote:

> When sharing interrupt line with other chips, the interrupt pin most
> likely needs to be configured in open-drain mode instead of push-pull.
> If this is needed, you must add drive-open-drain property to the
> device-tree.

Why are you mentioning the device tree in the commit message? Just keep
the first sentence + a short description of what you added/changed/removed.

> Signed-off-by: Esben Haabendal <esben@geanix.com>
> ---
>  drivers/iio/accel/mma8452.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 7d683686dd9d..a20c02ce0b9c 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)

I know that the defines are completely incorrectly aligned, but please
ensure that at least all the defines in this block are aligned.

Also, consider sending a patch which aligns all the other defines.

>  #define MMA8452_CTRL_REG4			0x2d
>  #define MMA8452_CTRL_REG5			0x2e
>  #define MMA8452_OFF_X				0x2f
> @@ -108,6 +110,7 @@ struct mma8452_data {
>  	struct iio_mount_matrix orientation;
>  	u8 ctrl_reg1;
>  	u8 data_cfg;
> +	bool open_drain;

Hmm, i checked pahole and it says there is a 1 byte hole, maybe try some more
reordering to pack it?

>  	const struct mma_chip_info *chip_info;
>  	int sleep_val;
>  	struct regulator *vdd_reg;
> @@ -646,6 +649,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)
>  {
> @@ -1666,6 +1685,9 @@ static int mma8452_probe(struct i2c_client *client)
>  			goto disable_regulators;
>  	}
>  
> +	data->open_drain = device_property_read_bool(&client->dev, "drive-open-drain");
> +	mma8452_set_interrupt_pin_mode(data);

You're not checking the return value here.

> +
>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>  			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>  
> @@ -1683,7 +1705,8 @@ static int mma8452_probe(struct i2c_client *client)
>  
>  	if (client->irq) {
>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
> +					   data->open_drain ? IRQF_SHARED : 0,

Sashiko raises a pretty fun issue: the statement

IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain ? IRQF_SHARED : 0

is actually evaluated as

(IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain) ? IRQF_SHARED : 0

Bitwise OR precedes the ternary operator.

You should wrap the data->open_drain ternary in parenthesis.

>  					   client->name, indio_dev);
>  		if (ret)
>  			goto buffer_cleanup;
> @@ -1800,6 +1823,10 @@ static int mma8452_runtime_resume(struct device *dev)
>  		return ret;
>  	}
>  
> +	ret = mma8452_set_interrupt_pin_mode(data);
> +	if (ret < 0)
> +		goto runtime_resume_failed;

You can just have if (ret), as only 0 is successful.

> +
>  	ret = mma8452_active(data);
>  	if (ret < 0)
>  		goto runtime_resume_failed;
> 



-- 
Kind regards

CJD

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Andy Shevchenko]

On Wed, Jul 15, 2026 at 10:45:42AM +0200, Joshua Crofts wrote:
> On Wed, 15 Jul 2026 10:07:39 +0200
> Esben Haabendal <esben@geanix.com> wrote:

...

> > @@ -108,6 +110,7 @@ struct mma8452_data {
> >  	struct iio_mount_matrix orientation;
> >  	u8 ctrl_reg1;
> >  	u8 data_cfg;
> > +	bool open_drain;
> 
> Hmm, i checked pahole and it says there is a 1 byte hole, maybe try some more
> reordering to pack it?

Taking into account that the usual frames are 4 bytes and that there were
already 2 1-byte members, the place is good enough.

> >  	const struct mma_chip_info *chip_info;
> >  	int sleep_val;
> >  	struct regulator *vdd_reg;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

"Joshua Crofts" <joshua.crofts1@gmail.com> writes:

> On Wed, 15 Jul 2026 10:07:39 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
>> When sharing interrupt line with other chips, the interrupt pin most
>> likely needs to be configured in open-drain mode instead of push-pull.
>> If this is needed, you must add drive-open-drain property to the
>> device-tree.
>
> Why are you mentioning the device tree in the commit message? Just keep
> the first sentence + a short description of what you
> added/changed/removed.

Sure. Will do that for v2.

>> Signed-off-by: Esben Haabendal <esben@geanix.com>
>> ---
>>  drivers/iio/accel/mma8452.c | 29 ++++++++++++++++++++++++++++-
>>  1 file changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 7d683686dd9d..a20c02ce0b9c 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)
>
> I know that the defines are completely incorrectly aligned, but please
> ensure that at least all the defines in this block are aligned.
>
> Also, consider sending a patch which aligns all the other defines.

How are they incorrectly aligned?
The all look perfectly fine here (visual tabs space set to 8).
Should I convert all the tabs used for alignment to spaces?

AFAICS, I have added the defines with same alignment as the other
defines in that block. I believe the misalignment is only a visual
artifact caused by the diff format.

>>  #define MMA8452_CTRL_REG4			0x2d
>>  #define MMA8452_CTRL_REG5			0x2e
>>  #define MMA8452_OFF_X				0x2f
>> @@ -108,6 +110,7 @@ struct mma8452_data {
>>  	struct iio_mount_matrix orientation;
>>  	u8 ctrl_reg1;
>>  	u8 data_cfg;
>> +	bool open_drain;
>
> Hmm, i checked pahole and it says there is a 1 byte hole, maybe try some more
> reordering to pack it?

On aarch64 it was this:

struct mma8452_data {
        struct i2c_client *        client;               /*     0     8 */
        struct mutex               lock __attribute__((__aligned__(8))); /*     8    24 */
        struct iio_mount_matrix    orientation;          /*    32    72 */
        /* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
        u8                         ctrl_reg1;            /*   104     1 */
        u8                         data_cfg;             /*   105     1 */
        bool                       open_drain;           /*   106     1 */

        /* XXX 5 bytes hole, try to pack */

        const struct mma_chip_info  * chip_info;         /*   112     8 */
        int                        sleep_val;            /*   120     4 */

        /* XXX 4 bytes hole, try to pack */

        /* --- cacheline 2 boundary (128 bytes) --- */
        struct regulator *         vdd_reg;              /*   128     8 */
        struct regulator *         vddio_reg;            /*   136     8 */
        struct {
                __be16             channels[3];          /*   144     6 */

                /* XXX 2 bytes hole, try to pack */

                __s64              ts __attribute__((__aligned__(8))); /*   152     8 */
        } __attribute__((__aligned__(8))) buffer __attribute__((__aligned__(8)));        /*   144    16 */

        /* XXX last struct has 1 hole */

        /* size: 160, cachelines: 3, members: 11 */
        /* sum members: 151, holes: 2, sum holes: 9 */
        /* member types with holes: 1, total: 1 */
        /* forced alignments: 2 */
        /* last cacheline: 32 bytes */
} __attribute__((__aligned__(8)));

After reordering member fields, I get this:

struct mma8452_data {
        struct i2c_client *        client;               /*     0     8 */
        struct mutex               lock __attribute__((__aligned__(8))); /*     8    24 */
        struct iio_mount_matrix    orientation;          /*    32    72 */
        /* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
        const struct mma_chip_info  * chip_info;         /*   104     8 */
        struct regulator *         vdd_reg;              /*   112     8 */
        struct regulator *         vddio_reg;            /*   120     8 */
        /* --- cacheline 2 boundary (128 bytes) --- */
        struct {
                __be16             channels[3];          /*   128     6 */

                /* XXX 2 bytes hole, try to pack */

                __s64              ts __attribute__((__aligned__(8))); /*   136     8 */
        } __attribute__((__aligned__(8))) buffer __attribute__((__aligned__(8)));        /*   128    16 */

        /* XXX last struct has 1 hole */

        int                        sleep_val;            /*   144     4 */
        u8                         ctrl_reg1;            /*   148     1 */
        u8                         data_cfg;             /*   149     1 */
        bool                       open_drain;           /*   150     1 */

        /* size: 152, cachelines: 3, members: 11 */
        /* padding: 1 */
        /* member types with holes: 1, total: 1 */
        /* forced alignments: 2 */
        /* last cacheline: 24 bytes */
} __attribute__((__aligned__(8)));

>>  	const struct mma_chip_info *chip_info;
>>  	int sleep_val;
>>  	struct regulator *vdd_reg;
>> @@ -646,6 +649,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)
>>  {
>> @@ -1666,6 +1685,9 @@ static int mma8452_probe(struct i2c_client *client)
>>  			goto disable_regulators;
>>  	}
>>
>> +	data->open_drain = device_property_read_bool(&client->dev, "drive-open-drain");
>> +	mma8452_set_interrupt_pin_mode(data);
>
> You're not checking the return value here.

Sorry, I will propagate the error code up.

>>  	data->ctrl_reg1 = MMA8452_CTRL_ACTIVE |
>>  			  (MMA8452_CTRL_DR_DEFAULT << MMA8452_CTRL_DR_SHIFT);
>>
>> @@ -1683,7 +1705,8 @@ static int mma8452_probe(struct i2c_client *client)
>>
>>  	if (client->irq) {
>>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
>> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
>> +					   data->open_drain ? IRQF_SHARED : 0,
>
> Sashiko raises a pretty fun issue: the statement
>
> IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain ? IRQF_SHARED : 0
>
> is actually evaluated as
>
> (IRQF_TRIGGER_LOW | IRQF_ONESHOT | data->open_drain) ? IRQF_SHARED : 0
>
> Bitwise OR precedes the ternary operator.
>
> You should wrap the data->open_drain ternary in parenthesis.

Yep. That was a nice catch indeed. Fixed.

>>  					   client->name, indio_dev);
>>  		if (ret)
>>  			goto buffer_cleanup;
>> @@ -1800,6 +1823,10 @@ static int mma8452_runtime_resume(struct device *dev)
>>  		return ret;
>>  	}
>>
>> +	ret = mma8452_set_interrupt_pin_mode(data);
>> +	if (ret < 0)
>> +		goto runtime_resume_failed;
>
> You can just have if (ret), as only 0 is successful.

Will do.

>> +
>>  	ret = mma8452_active(data);
>>  	if (ret < 0)
>>  		goto runtime_resume_failed;

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

"Andy Shevchenko" <andriy.shevchenko@intel.com> writes:

> On Wed, Jul 15, 2026 at 10:07:39AM +0200, Esben Haabendal wrote:
>> When sharing interrupt line with other chips, the interrupt pin most
>> likely needs to be configured in open-drain mode instead of push-pull.
>> If this is needed, you must add drive-open-drain property to the
>> device-tree.
>
> ...
>
>>  	if (client->irq) {
>>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
>> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
>> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
>> +					   data->open_drain ? IRQF_SHARED : 0,
>>  					   client->name, indio_dev);
>
> Why do we care?

Care about what exactly?

We need to add IRQF_SHARED flag in order to allow shared interrupt, and
we should not add it when using (the default) push-pull mode.

> The (hidden) problem this will have in the future is that the IRQ core
> will splat a warning in case that other shared IRQs might be
> configured with different flags. Putting that flag conditionally makes
> it a mine field for the users. Instead just unconditionally add that
> flag and we will get reports as soon as there will be a user that
> shares the same interrupt pin with some other devices which drivers do
> not use the same settings.

If we add the IRQF_SHARED flag unconditionally, it will be set also when
push-pull mode is enabled. I don't see how the kernel will be able to
notice that that is not going to work. If you have another device that
uses IRQF_TRIGGER_LOW|IRF_ONESHOT|IRQF_SHARED, it will not work with the
MMA8452 device when configured as push-pull.

> Also setting to _LOW in the flags unconditionally is a (historic) bug.
> The problem is that we might not fix it without breaking the existing
> users which omit that flag in DT.

Ok. So let's leave that as is for now.

/Esben

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Joshua Crofts]

On Wed, 15 Jul 2026 13:29:17 +0200
Esben Haabendal <esben@geanix.com> wrote:

...

> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> >> index 7d683686dd9d..a20c02ce0b9c 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)  
> >
> > I know that the defines are completely incorrectly aligned, but please
> > ensure that at least all the defines in this block are aligned.
> >
> > Also, consider sending a patch which aligns all the other defines.  
> 
> How are they incorrectly aligned?
> The all look perfectly fine here (visual tabs space set to 8).
> Should I convert all the tabs used for alignment to spaces?
> 
> AFAICS, I have added the defines with same alignment as the other
> defines in that block. I believe the misalignment is only a visual
> artifact caused by the diff format.

Ah, I meant it as currently the defines look like this:

#define MMA8452_WHO_AM_I			0x0d
#define MMA8452_DATA_CFG			0x0e
#define  MMA8452_DATA_CFG_FS_MASK		GENMASK(1, 0)
#define  MMA8452_DATA_CFG_FS_2G			0

but instead should look like this:

#define MMA8452_WHO_AM_I			0x0d
#define MMA8452_DATA_CFG			0x0e
#define MMA8452_DATA_CFG_FS_MASK		GENMASK(1, 0)
#define MMA8452_DATA_CFG_FS_2G			0

I'm not sure if this was intentional or not in the original implementation.

...

> After reordering member fields, I get this:
> 
> struct mma8452_data {
>         struct i2c_client *        client;               /*     0     8 */
>         struct mutex               lock __attribute__((__aligned__(8))); /*     8    24 */
>         struct iio_mount_matrix    orientation;          /*    32    72 */
>         /* --- cacheline 1 boundary (64 bytes) was 40 bytes ago --- */
>         const struct mma_chip_info  * chip_info;         /*   104     8 */
>         struct regulator *         vdd_reg;              /*   112     8 */
>         struct regulator *         vddio_reg;            /*   120     8 */
>         /* --- cacheline 2 boundary (128 bytes) --- */
>         struct {
>                 __be16             channels[3];          /*   128     6 */
> 
>                 /* XXX 2 bytes hole, try to pack */
> 
>                 __s64              ts __attribute__((__aligned__(8))); /*   136     8 */
>         } __attribute__((__aligned__(8))) buffer __attribute__((__aligned__(8)));        /*   128    16 */
> 
>         /* XXX last struct has 1 hole */
> 
>         int                        sleep_val;            /*   144     4 */
>         u8                         ctrl_reg1;            /*   148     1 */
>         u8                         data_cfg;             /*   149     1 */
>         bool                       open_drain;           /*   150     1 */
> 
>         /* size: 152, cachelines: 3, members: 11 */
>         /* padding: 1 */
>         /* member types with holes: 1, total: 1 */
>         /* forced alignments: 2 */
>         /* last cacheline: 24 bytes */
> } __attribute__((__aligned__(8)));

This looks great!

-- 
Kind regards

CJD

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Esben Haabendal]

"Joshua Crofts" <joshua.crofts1@gmail.com> writes:

> On Wed, 15 Jul 2026 13:29:17 +0200
> Esben Haabendal <esben@geanix.com> wrote:
>
> ...
>
>> >> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> >> index 7d683686dd9d..a20c02ce0b9c 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)
>> >
>> > I know that the defines are completely incorrectly aligned, but please
>> > ensure that at least all the defines in this block are aligned.
>> >
>> > Also, consider sending a patch which aligns all the other defines.
>>
>> How are they incorrectly aligned?
>> The all look perfectly fine here (visual tabs space set to 8).
>> Should I convert all the tabs used for alignment to spaces?
>>
>> AFAICS, I have added the defines with same alignment as the other
>> defines in that block. I believe the misalignment is only a visual
>> artifact caused by the diff format.
>
> Ah, I meant it as currently the defines look like this:
>
> #define MMA8452_WHO_AM_I			0x0d
> #define MMA8452_DATA_CFG			0x0e
> #define  MMA8452_DATA_CFG_FS_MASK		GENMASK(1, 0)
> #define  MMA8452_DATA_CFG_FS_2G			0
>
> but instead should look like this:
>
> #define MMA8452_WHO_AM_I			0x0d
> #define MMA8452_DATA_CFG			0x0e
> #define MMA8452_DATA_CFG_FS_MASK		GENMASK(1, 0)
> #define MMA8452_DATA_CFG_FS_2G			0
>
> I'm not sure if this was intentional or not in the original implementation.

Ah okay. I assumed it was intentional, hinting at the hierarchy of the
defines.
Should I flatten that, or is it okay to leave it as it is?
For what it is worth, I like the current format :)

/Esben

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Joshua Crofts]

On Wed, 15 Jul 2026 14:21:56 +0200
Esben Haabendal <esben@geanix.com> wrote:

> > I'm not sure if this was intentional or not in the original implementation.  
> 
> Ah okay. I assumed it was intentional, hinting at the hierarchy of the
> defines.
> Should I flatten that, or is it okay to leave it as it is?
> For what it is worth, I like the current format :)

Up to you of course, nobody can force you to do anything :P

-- 
Kind regards

CJD

Re: [PATCH 2/2] iio: accel: mma8452: Allow open drain interrupt pin configuration

[Andy Shevchenko]

On Wed, Jul 15, 2026 at 01:35:41PM +0200, Esben Haabendal wrote:
> "Andy Shevchenko" <andriy.shevchenko@intel.com> writes:
> > On Wed, Jul 15, 2026 at 10:07:39AM +0200, Esben Haabendal wrote:

...

> >>  	if (client->irq) {
> >>  		ret = request_threaded_irq(client->irq, NULL, mma8452_interrupt,
> >> -					   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> >> +					   IRQF_TRIGGER_LOW | IRQF_ONESHOT |
> >> +					   data->open_drain ? IRQF_SHARED : 0,
> >>  					   client->name, indio_dev);
> >
> > Why do we care?
> 
> Care about what exactly?

About exclusivity of the interrupt.

> We need to add IRQF_SHARED flag in order to allow shared interrupt, and
> we should not add it when using (the default) push-pull mode.

Why not? How would it make any difference from SW perspective?
Yes, I understand the HW case.

> > The (hidden) problem this will have in the future is that the IRQ core
> > will splat a warning in case that other shared IRQs might be
> > configured with different flags. Putting that flag conditionally makes
> > it a mine field for the users. Instead just unconditionally add that
> > flag and we will get reports as soon as there will be a user that
> > shares the same interrupt pin with some other devices which drivers do
> > not use the same settings.
> 
> If we add the IRQF_SHARED flag unconditionally, it will be set also when
> push-pull mode is enabled. I don't see how the kernel will be able to
> notice that that is not going to work. If you have another device that
> uses IRQF_TRIGGER_LOW|IRF_ONESHOT|IRQF_SHARED, it will not work with the
> MMA8452 device when configured as push-pull.

Right, and why do we care (again)? It's pure DT/FW/HW issue, not an SW issue.
Otherwise it will become a carefully placed mine for the poor user who will
use these flags and try to share an interrupt with the mma8452 device which
has no set property and uses push-pull mode.

Did I miss anything?

-- 
With Best Regards,
Andy Shevchenko