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

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

Ok.

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

Not adding the IRQF_SHARED flag prevents use with shared interrupts. I
think we are on the same page on that.

Unconditional adding IRQF_SHARED flag would allow configurations where
other devices share interrupt line with mma8452 compatible chip
configured with push-pull, resulting in broken or unpredictable results.
I don't see why we should not care about that.

> 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)?

Why we care that the system as a whole (SW on top of HW) will not work?

If we don't care about that, why do we even have this IRQF_SHARED flag?
The only purpose of that is to tell the kernel that this particular
device / interrupt will work with shared interrupt or not.

Isn't that exactly what I do with this change? Nothing more, nothing less.

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

I don't get how you see it like that. Adding IRQF_SHARED unconditionally
would create exactly the mine field you are talking about. Poor users
can specify a system configuration (DT) that tries to use a shared
interrupt line, but configures the mma8452 compatible chip in push-pull.
The poor user will not only be poor, but also unhappy.

By applying the IRQF_SHARED dynamically, the kernel will be able to fail
in a controlled manner instead, making it much less painful to create a
working system configuration (DT)

Going back to your poor user story above, if the poor user tries to
share the interrupt pin with the mma8452 device, it will get an error
(just as it is with the kernel today). The device does not support
shared interrupts. Reading the DT bindings documentation, the user
should be able to find the drive-open-drain property, and add that to
the device-tree to make things work. Maybe even consider if that is
compatible with the hardware being used.

If IRQF_SHARED was set unconditionally, the user would not get an error,
but most likely would get a system where no irq's were raised for the
other chips. I sincerely believe that debugging this is much more
painful than reading device-tree bindings.

> Did I miss anything?

I don't know. Maybe I am missing the obvious here.

/Esben

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

[Andy Shevchenko]

On Wed, Jul 15, 2026 at 04:25:20PM +0200, Esben Haabendal wrote:
> "Andy Shevchenko" <andriy.shevchenko@intel.com> writes:
> > 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.
> 
> Ok.
> 
> >> 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?
> 
> Not adding the IRQF_SHARED flag prevents use with shared interrupts. I
> think we are on the same page on that.
> 
> Unconditional adding IRQF_SHARED flag would allow configurations where
> other devices share interrupt line with mma8452 compatible chip
> configured with push-pull, resulting in broken or unpredictable results.
> I don't see why we should not care about that.

But it's not their problem! If it's this device that prevents this
configuration, it should have a check. With this code it just hides
and changing a DT property will lead to kernel warning.

> > 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)?
> 
> Why we care that the system as a whole (SW on top of HW) will not work?
> 
> If we don't care about that, why do we even have this IRQF_SHARED flag?
> The only purpose of that is to tell the kernel that this particular
> device / interrupt will work with shared interrupt or not.
> 
> Isn't that exactly what I do with this change? Nothing more, nothing less.
> 
> > 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.
> 
> I don't get how you see it like that. Adding IRQF_SHARED unconditionally
> would create exactly the mine field you are talking about. Poor users
> can specify a system configuration (DT) that tries to use a shared
> interrupt line, but configures the mma8452 compatible chip in push-pull.
> The poor user will not only be poor, but also unhappy.

> By applying the IRQF_SHARED dynamically, the kernel will be able to fail
> in a controlled manner instead, making it much less painful to create a
> working system configuration (DT)

And my point that we need to make less painful runtime experience.

> Going back to your poor user story above, if the poor user tries to
> share the interrupt pin with the mma8452 device, it will get an error
> (just as it is with the kernel today). The device does not support
> shared interrupts. Reading the DT bindings documentation, the user
> should be able to find the drive-open-drain property, and add that to
> the device-tree to make things work. Maybe even consider if that is
> compatible with the hardware being used.
> 
> If IRQF_SHARED was set unconditionally, the user would not get an error,
> but most likely would get a system where no irq's were raised for the
> other chips. I sincerely believe that debugging this is much more
> painful than reading device-tree bindings.
> 
> > Did I miss anything?
> 
> I don't know. Maybe I am missing the obvious here.

I consider the case when shared interrupt is enabled on both devices, but
second one (driver) missed the same IRQ flags. This becomes a warning in
IRQ core.

Shared interrupts it's also a contract with all stakeholders on keeping
the same flags for all devices.

...

After looking into genirq code I don't see other way how to handle this.
It looks like we need to address the TRIGGER_LOW first, in other words
we need drop that flag when IRQF_SHARED is set, and leave it to users
to setup IRQ trigger properly on all sides.

-- 
With Best Regards,
Andy Shevchenko