[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

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

[Esben Haabendal]

On Wednesday, 15 July 2026 at 16:56, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

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

The chip such does not prevent the configuration, and does not prevent
sharing the interrupt. The irq pin in the device can be configured in either
push-pull mode or open-drain mode.
In order to allow sharing interrupt line with other chips, the chip
must be configured with irq pin in open-drain mode, which is what the
drive-open-drain device-tree property I am adding in this series enables
developers to do.
If/when you add this property to the device-tree, the irq pin will be
in open-drain mode, and the IRQF_SHARED flag can (and should IMHO) be set
as the interrupt line can be shared.
If/when the drive-open-drain property is not set (existing device-trees
and current only supported behavior), the chip cannot support sharing the
interrupt line, and adding the IRQF_SHARED flag would be a bug, as the
chip irq pin requires an exclusive interrupt input.

Maybe the confusion is caused by the fact that the drive-open-drain
does not describe how the irq pin IS behaving, but rather how it SHOULD
behave, and possibly required because of the HW designed having tied the
irq pin to an irq input together with other irq sources.

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

Yes. We agree on that.

But I do believe that my patch is that painful runtime experience.

Adding IRQF_SHARED flag when irq pin is configured in push-pull mode
will be painful. I know, that was my initial approach. Does really not
work, and can lead to wasting time on debugging the HW.
 
> > 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.

Missed which IRQ flags?

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

Of-course.

And adding IRQF_SHARED is a contract that says that the interrupt can be
shared. I can only see that it would be a bug to add that flag in cases
where we know that the interrupt really cannot be shared.

As for the IRQF_TRIGGER_LOW flag being set, I think that looks correct
as long as we do not support switching to active high irq polarity.

But on that topic, the chip actually supports selecting between active
low and high irq polarity. Should we implement something along the lines
of what is done in smi330_setup_irq() in iio/imu/smi330/smi330_core.c?
And thereby allow using the device with active high irq?

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

Why do we need to drop the IRQF_TRIGGER_LOW flag because IRQF_SHARED is
set?

Users as in those specifying the system device-tree?

/Esben

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

[Andy Shevchenko]

On Thu, Jul 16, 2026 at 04:51:34PM +0000, Esben Haabendal wrote:
> On Wednesday, 15 July 2026 at 16:56, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > 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.
> 
> The chip such does not prevent the configuration, and does not prevent
> sharing the interrupt. The irq pin in the device can be configured in either
> push-pull mode or open-drain mode.
> In order to allow sharing interrupt line with other chips, the chip
> must be configured with irq pin in open-drain mode, which is what the
> drive-open-drain device-tree property I am adding in this series enables
> developers to do.
> If/when you add this property to the device-tree, the irq pin will be
> in open-drain mode, and the IRQF_SHARED flag can (and should IMHO) be set
> as the interrupt line can be shared.
> If/when the drive-open-drain property is not set (existing device-trees
> and current only supported behavior), the chip cannot support sharing the
> interrupt line, and adding the IRQF_SHARED flag would be a bug, as the
> chip irq pin requires an exclusive interrupt input.
> 
> Maybe the confusion is caused by the fact that the drive-open-drain
> does not describe how the irq pin IS behaving, but rather how it SHOULD
> behave, and possibly required because of the HW designed having tied the
> irq pin to an irq input together with other irq sources.

Why do we care of push-pull then? Can't we always make it open-drain?

> > > > 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.
> 
> Yes. We agree on that.
> 
> But I do believe that my patch is that painful runtime experience.
> 
> Adding IRQF_SHARED flag when irq pin is configured in push-pull mode
> will be painful. I know, that was my initial approach. Does really not
> work, and can lead to wasting time on debugging the HW.
>  
> > > 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.
> 
> Missed which IRQ flags?

_LOW, for instance.

> > Shared interrupts it's also a contract with all stakeholders on keeping
> > the same flags for all devices.
> 
> Of-course.
> 
> And adding IRQF_SHARED is a contract that says that the interrupt can be
> shared. I can only see that it would be a bug to add that flag in cases
> where we know that the interrupt really cannot be shared.
> 
> As for the IRQF_TRIGGER_LOW flag being set, I think that looks correct
> as long as we do not support switching to active high irq polarity.

No, it's not. It overrides whatever platform wants to have. It prevents
PCB level designs from anything else.

> But on that topic, the chip actually supports selecting between active
> low and high irq polarity. Should we implement something along the lines
> of what is done in smi330_setup_irq() in iio/imu/smi330/smi330_core.c?
> And thereby allow using the device with active high irq?

...

> > 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.
> 
> Why do we need to drop the IRQF_TRIGGER_LOW flag because IRQF_SHARED is
> set?
> 
> Users as in those specifying the system device-tree?

No, because like I said, the SHARED is a *contract*, it's not just a setting.
And that contract includes the similarities on the IRQ flags. One may not
have two devices with different trigger level, it's a common sense, right?

So, SHARED must exclude anything that comes from the platform description or
IRQ chip controller (limitations).

So, without addressing that, it makes a little sense to add SHARED.

...

I would accept this patch without adding SHARED, perhaps. But still question
why we can't use the OD instead of PP for the pin to begin with? Do we have
real HW that uses IRQ input without a pull-up?

-- 
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 Thu, Jul 16, 2026 at 04:51:34PM +0000, Esben Haabendal wrote:
>> On Wednesday, 15 July 2026 at 16:56, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
>> > 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.
>>
>> The chip such does not prevent the configuration, and does not prevent
>> sharing the interrupt. The irq pin in the device can be configured in either
>> push-pull mode or open-drain mode.
>> In order to allow sharing interrupt line with other chips, the chip
>> must be configured with irq pin in open-drain mode, which is what the
>> drive-open-drain device-tree property I am adding in this series enables
>> developers to do.
>> If/when you add this property to the device-tree, the irq pin will be
>> in open-drain mode, and the IRQF_SHARED flag can (and should IMHO) be set
>> as the interrupt line can be shared.
>> If/when the drive-open-drain property is not set (existing device-trees
>> and current only supported behavior), the chip cannot support sharing the
>> interrupt line, and adding the IRQF_SHARED flag would be a bug, as the
>> chip irq pin requires an exclusive interrupt input.
>>
>> Maybe the confusion is caused by the fact that the drive-open-drain
>> does not describe how the irq pin IS behaving, but rather how it SHOULD
>> behave, and possibly required because of the HW designed having tied the
>> irq pin to an irq input together with other irq sources.
>
> Why do we care of push-pull then? Can't we always make it open-drain?

No. Open-drain requires an external pull-up resistor. If there are no
external pull-up resistor, open-drain will not work, and interrupt
sharing is not possible.

>> > > > 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.
>>
>> Yes. We agree on that.
>>
>> But I do believe that my patch is that painful runtime experience.
>>
>> Adding IRQF_SHARED flag when irq pin is configured in push-pull mode
>> will be painful. I know, that was my initial approach. Does really not
>> work, and can lead to wasting time on debugging the HW.
>>
>> > > 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.
>>
>> Missed which IRQ flags?
>
> _LOW, for instance.

All devices sharing an interrupt really do need to agree on these
flags. Do you known any drivers that does not specify the trigger type
when requesting an irq?.
You cannot share an interrupt line if not all devices have the
same trigger mode. So if one device on a shared interrupt has _LOW, all
the other devices need to have that. Not just the flags, but the actual
triggering mode of the device. Some chips might support more than one
mode, others only one specific mode.

>> > Shared interrupts it's also a contract with all stakeholders on keeping
>> > the same flags for all devices.
>>
>> Of-course.
>>
>> And adding IRQF_SHARED is a contract that says that the interrupt can be
>> shared. I can only see that it would be a bug to add that flag in cases
>> where we know that the interrupt really cannot be shared.
>>
>> As for the IRQF_TRIGGER_LOW flag being set, I think that looks correct
>> as long as we do not support switching to active high irq polarity.
>
> No, it's not. It overrides whatever platform wants to have.

The trigger type is not just a platform thing. It describes how the
device behaves. Device that only support one triggering mode must set
the flag that describes that. Devices that can be configured for
different triggering modes can implement that, and should then configure
the device for the chosen mode and set the flags accordingly.

> It prevents PCB level designs from anything else.

Yes. In this case, the current driver does not support designs that
requires the mma8452 compatible chip to be configured for active high
irq trigger. We could and probably should address that.

But that is a different issue than what I am doing here. And I don't
have hardware readily available where this can be tested, so I would
prefer to let somebody else do that.

I am adding support for designs that require the mma8452 compatible chip
to be configured for open-drain, while staying backwards compatible and
allowing support for designs without external pull-up.

In all cases, the driver should register the interrupt handle with the
IRQF_TRIGGER_* flag that matches what the chip is configured for.
I believe leaving it out is invalid.

>> But on that topic, the chip actually supports selecting between active
>> low and high irq polarity. Should we implement something along the lines
>> of what is done in smi330_setup_irq() in iio/imu/smi330/smi330_core.c?
>> And thereby allow using the device with active high irq?
>
> ...
>
>> > 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.
>>
>> Why do we need to drop the IRQF_TRIGGER_LOW flag because IRQF_SHARED is
>> set?
>>
>> Users as in those specifying the system device-tree?
>
> No, because like I said, the SHARED is a *contract*, it's not just a setting.

Absolutely. That is the key here.

IQRF_SHARED is a contract, where the driver promises to return IRQ_NONE
if the interrupt is not for that device. This is already implemented
by mma8452_interrupt().

> And that contract includes the similarities on the IRQ flags. One may not
> have two devices with different trigger level, it's a common sense,
> right?

Right.

And that is exactly why we have to specify the IRQF_TRIGGER_* flag that
the irq output is configured for. The driver will not switch to the
desired trigger level based on what other devices on the same interrupt
uses. You have to configure/specify the trigger mode and leve and set
the IRQF_TRIGGER_* flag accordingly.

> So, SHARED must exclude anything that comes from the platform description or
> IRQ chip controller (limitations).

Now you got me confused again. The IRQF_TRIGGER_LOW flag given to
request_threaded_irq() is not coming from platform description or IRQ
chip controller. It describes the trigger mode that the chip is
excercising in the current implementation.

The kernel would not know how to configure the IRQ chip controller, and
in case of shared interrupts, would not be able to check that the
devices sharing the interrupt is configured correctly.

> So, without addressing that, it makes a little sense to add SHARED.

Not true.

Let me take my system here. I have multiple chips / irq sources
connected to the same interrupt input on the CPU. They are all using
level low triggering (which hardware and system engineers have ofcourse
been observant enough to ensure).

Without my change, the mma8452 driver does not support configuring the
chip to use open-drain, so it will be pulling the interrupt line high,
interrupting irq for all interrupt sources.

With my change, I can now configure the device to use open-drain,
register the interrupt, and everything works nicely.

And the change is backwards compatible, as you have to specify
drive-open-drain to switch from PP to OD.

How does that not make sense?

> I would accept this patch without adding SHARED, perhaps.

Without IRQF_SHARED, I cannot use it with the hardware I have which are
sharing the interrupt.

> But still question why we can't use the OD instead of PP for the pin
> to begin with?

Because HW needs to be designed for OD. It is perfectly valid to design
hardware that relies on the default mode of mma8452 and compatible chips
to use PP. Switching to OD will not work for such systems without
external pull-up.

> Do we have real HW that uses IRQ input without a pull-up?

I certainly have had such designs many times in the past.

Why should we implement the driver so that we cannot support such valid
HW designs?

/Esben

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

[Andy Shevchenko]

On Fri, Jul 17, 2026 at 02:02:50PM +0200, Esben Haabendal wrote:
> "Andy Shevchenko" <andriy.shevchenko@intel.com> writes:
> > On Thu, Jul 16, 2026 at 04:51:34PM +0000, Esben Haabendal wrote:
> >> On Wednesday, 15 July 2026 at 16:56, Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> >> > 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.
> >>
> >> The chip such does not prevent the configuration, and does not prevent
> >> sharing the interrupt. The irq pin in the device can be configured in either
> >> push-pull mode or open-drain mode.
> >> In order to allow sharing interrupt line with other chips, the chip
> >> must be configured with irq pin in open-drain mode, which is what the
> >> drive-open-drain device-tree property I am adding in this series enables
> >> developers to do.
> >> If/when you add this property to the device-tree, the irq pin will be
> >> in open-drain mode, and the IRQF_SHARED flag can (and should IMHO) be set
> >> as the interrupt line can be shared.
> >> If/when the drive-open-drain property is not set (existing device-trees
> >> and current only supported behavior), the chip cannot support sharing the
> >> interrupt line, and adding the IRQF_SHARED flag would be a bug, as the
> >> chip irq pin requires an exclusive interrupt input.
> >>
> >> Maybe the confusion is caused by the fact that the drive-open-drain
> >> does not describe how the irq pin IS behaving, but rather how it SHOULD
> >> behave, and possibly required because of the HW designed having tied the
> >> irq pin to an irq input together with other irq sources.
> >
> > Why do we care of push-pull then? Can't we always make it open-drain?
> 
> No. Open-drain requires an external pull-up resistor. If there are no
> external pull-up resistor, open-drain will not work, and interrupt
> sharing is not possible.

Right. My question was basically to understand if we have the HW (platforms to
which this sensor is attached) that relies on this pin to be PP (in current
configuration)?

> >> > > > 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.
> >>
> >> Yes. We agree on that.
> >>
> >> But I do believe that my patch is that painful runtime experience.
> >>
> >> Adding IRQF_SHARED flag when irq pin is configured in push-pull mode
> >> will be painful. I know, that was my initial approach. Does really not
> >> work, and can lead to wasting time on debugging the HW.
> >>
> >> > > 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.
> >>
> >> Missed which IRQ flags?
> >
> > _LOW, for instance.
> 
> All devices sharing an interrupt really do need to agree on these
> flags. Do you known any drivers that does not specify the trigger type
> when requesting an irq?.

Huh?! Majority of the device drivers! And that's how it should have been from
day 1, that's why I call this a pre-existing bug in this driver which we may
not address :-(. But we may address it when use SHARED interrupts.

> You cannot share an interrupt line if not all devices have the
> same trigger mode. So if one device on a shared interrupt has _LOW, all
> the other devices need to have that. Not just the flags, but the actual
> triggering mode of the device. Some chips might support more than one
> mode, others only one specific mode.

Yes.

> >> > Shared interrupts it's also a contract with all stakeholders on keeping
> >> > the same flags for all devices.
> >>
> >> Of-course.
> >>
> >> And adding IRQF_SHARED is a contract that says that the interrupt can be
> >> shared. I can only see that it would be a bug to add that flag in cases
> >> where we know that the interrupt really cannot be shared.
> >>
> >> As for the IRQF_TRIGGER_LOW flag being set, I think that looks correct
> >> as long as we do not support switching to active high irq polarity.
> >
> > No, it's not. It overrides whatever platform wants to have.
> 
> The trigger type is not just a platform thing. It describes how the
> device behaves.

No, it describes how this certain device is connected to the certain platform.
It's a combination of what device is capable of and how platform design makes
use of it.

> Device that only support one triggering mode must set
> the flag that describes that. Devices that can be configured for
> different triggering modes can implement that, and should then configure
> the device for the chosen mode and set the flags accordingly.

Yes, but not in the device drivers!

> > It prevents PCB level designs from anything else.
> 
> Yes. In this case, the current driver does not support designs that
> requires the mma8452 compatible chip to be configured for active high
> irq trigger. We could and probably should address that.

Exactly!

> But that is a different issue than what I am doing here. And I don't
> have hardware readily available where this can be tested, so I would
> prefer to let somebody else do that.

No objection on the specific issue, but...

> I am adding support for designs that require the mma8452 compatible chip
> to be configured for open-drain, while staying backwards compatible and
> allowing support for designs without external pull-up.

...the shared case *is* different. And I do not want the disaster (a bug)
to spread.

> In all cases, the driver should register the interrupt handle with the
> IRQF_TRIGGER_* flag that matches what the chip is configured for.
> I believe leaving it out is invalid.

Again, this chip is not a single party in the equation. Nobody prevents
to put a NOT gate on PCB to support this device and say other that wants
inverted polarity while sharing the same interrupt line.

> >> But on that topic, the chip actually supports selecting between active
> >> low and high irq polarity. Should we implement something along the lines
> >> of what is done in smi330_setup_irq() in iio/imu/smi330/smi330_core.c?
> >> And thereby allow using the device with active high irq?

...

> >> > 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.
> >>
> >> Why do we need to drop the IRQF_TRIGGER_LOW flag because IRQF_SHARED is
> >> set?
> >>
> >> Users as in those specifying the system device-tree?
> >
> > No, because like I said, the SHARED is a *contract*, it's not just a setting.
> 
> Absolutely. That is the key here.
> 
> IQRF_SHARED is a contract, where the driver promises to return IRQ_NONE
> if the interrupt is not for that device. This is already implemented
> by mma8452_interrupt().
> 
> > And that contract includes the similarities on the IRQ flags. One may not
> > have two devices with different trigger level, it's a common sense,
> > right?
> 
> Right.
> 
> And that is exactly why we have to specify the IRQF_TRIGGER_* flag that
> the irq output is configured for.

No, definitely the opposite. We may *not* specify this flag in the driver,
we have to rely on the _platform_ configuration. PCB level.

> The driver will not switch to the
> desired trigger level based on what other devices on the same interrupt
> uses. You have to configure/specify the trigger mode and leve and set
> the IRQF_TRIGGER_* flag accordingly.

Yes, but not in the driver!

> > So, SHARED must exclude anything that comes from the platform description or
> > IRQ chip controller (limitations).
> 
> Now you got me confused again. The IRQF_TRIGGER_LOW flag given to
> request_threaded_irq() is not coming from platform description or IRQ
> chip controller. It describes the trigger mode that the chip is
> excercising in the current implementation.

And this is wrong. It's not device driver business to define this. More
accurate it's not only of the device in question and how it works.

> The kernel would not know how to configure the IRQ chip controller, and
> in case of shared interrupts, would not be able to check that the
> devices sharing the interrupt is configured correctly.

Huh?! The kernel is responsible to program IRQ chip and the respective
device drivers in accordance with the platform design (and protocol to
supply it to the OS is called DT or ACPI or ...).

There are several limitations for this:
- IRQ chip that might not support certain trigger configurations
- device that can only take subset of trigger configurations
- in shared line case all of the devices need to agree on that
  on a _platform_ level

> > So, without addressing that, it makes a little sense to add SHARED.
> 
> Not true.

True. Obviously we have a disagreement here and I tried hard to explain
my point above.

> Let me take my system here. I have multiple chips / irq sources
> connected to the same interrupt input on the CPU. They are all using
> level low triggering (which hardware and system engineers have ofcourse
> been observant enough to ensure).

And if not? Isn't it possible to make them share an interrupt line?
Of course it's very much possible, but in the platform description
the both devices will need to have the same one, so for the second
one it will be a bit of a lie (due to an external NOT gate or a latch
or any other PCB level tricks).

> Without my change, the mma8452 driver does not support configuring the
> chip to use open-drain, so it will be pulling the interrupt line high,
> interrupting irq for all interrupt sources.

I even don't consider this, this is obvious. And you are correct here.

> With my change, I can now configure the device to use open-drain,
> register the interrupt, and everything works nicely.

Nope, it depends on the other stakeholders.

> And the change is backwards compatible, as you have to specify
> drive-open-drain to switch from PP to OD.
> 
> How does that not make sense?

Please, read again what I wrote.

> > I would accept this patch without adding SHARED, perhaps.
> 
> Without IRQF_SHARED, I cannot use it with the hardware I have which are
> sharing the interrupt.

True, but with _LOW it makes a little sense to use SHARED.

> > But still question why we can't use the OD instead of PP for the pin
> > to begin with?
> 
> Because HW needs to be designed for OD. It is perfectly valid to design
> hardware that relies on the default mode of mma8452 and compatible chips
> to use PP. Switching to OD will not work for such systems without
> external pull-up.
> 
> > Do we have real HW that uses IRQ input without a pull-up?
> 
> I certainly have had such designs many times in the past.

Okay.

> Why should we implement the driver so that we cannot support such valid
> HW designs?

This is exactly my question as your patch limits this to _LOW.
And this is a big issue with it.

...

P.S.
I'm on vacation, but I disagree on the approach taken in this patch.
I leave it to you and Jonathan and others to settle down.

Here is my formal NAK (to this form of the patch, as I said, w/o shared
support it might be okay to go).

-- 
With Best Regards,
Andy Shevchenko