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

[Esben Haabendal <esben@geanix.com>]

"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