On 21/02/2018 01:15, Jan Kundrát wrote:
I think that it would be better to explicitly error out if irq_active_high and open_drain are both set. When I sent my attempt at this, I did it like this:
Which is something I want to do. As I've got a inverter in between :)
For the mcp the open-drain config bit takes precedence over active-high.
Really needs to be fixed by fixing up the irq request, but I still haven't got a good
fix for the irq request polarity based on active-high in this driver.
I can't see how to fix it without breaking existing dt interface.
I was told that the appropriate way forward are device drivers which do not specify the IRQ polarity. Apparently, people are supposed to do that in their DT.
yes, That's what I've been told as well
So, in this context:
- pinctrl-mcp23s08.c should only specify IRQF_ONESHOT | IRQF_SHARED
yeap.
- the DT should use an appropriate IRQ_TYPE_LEVEL_LOW or IRQ_TYPE_LEVEL_HIGH based on what the CPU expects to see on its IRQ pins
yeap
- the DT must also set a property to configure the MCP23xxx device to *generate* an IRQ by the active-high flag, or another flag for an open-drain active-low
IRQ output suitable for connecting directly to an interrupt line which gets shared by several open-drain IRQ producers
- whoever supplies the DT must now check that their settings "make sense"
Yes, this means that people might have to update their DTs. To me, that makes sense. If you ask me, the DT is already sort-of broken because it's using
IRQF_SHARED with a push-pull IRQ output. Yes, one can fix that with extra transistors, but that sounds quite ugly, doesn't it. The current heuristics only work
for some users, and they don't work for you and me.
I think IRQF_SHARED is fine as a push pull irq output can be shared via and AND / OR gate.
The only reason not to IRQF_SHARED is when you can't determine if the IRQ is from this device.
ie: There's no register to read in the potential source to see if the IRQ is asserted.
I dug a bit and found some reasoning [1] for why it was done like that back in 2014. Adding Alexander Stein to Cc as he wrote that code.
Phil, is that approach something that you agree with? Are you going to submit a patch for this? Or should I do that?
Yes, It'd be nice to remove the IO_CON read in probe and explicitly set the IO_CON register purely on the DT.
With kind regards,
Jan
[1] https://www.spinics.net/lists/devicetree/msg60203.html
Hmm, the description in the patch is what I want the driver to do.
On Thu, Nov 27, 2014 at 3:36 PM, Alexander Stein
What I need to do, is to change the interrupt polarity (to active high) at MCP23S17 while retaining the interrupt polarity on the controller as active-low.
But that's not what it does. As irqflags is set to match the chip output polarity in irq_setup
(Well it certainly doesn't for me).
Problem with changing the DT behaviour is, I believe it's consider an ABI and shouldn't be changed.
--
Regards
Phil Reid
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
