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@xxxxxxxxx> writes:
> > On Wed, Jul 15, 2026 at 01:35:41PM +0200, Esben Haabendal wrote:
> >> "Andy Shevchenko" <andriy.shevchenko@xxxxxxxxx> 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