[v4,3/3] pinctrl: mcp23s08: add open drain configuration for irq output

The mcp23s08 series device can be configured for wired and interrupts
using an external pull-up and open drain output via the IOCON_ODR bit.
And "drive-open-drain" property to enable this.

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Thanks for looking into this.

Is there a reason why the DT documentation is updated in a separate commit?

>  	if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror ||
> -	     mcp->irq_active_high) {
> +	     mcp->irq_active_high || open_drain) {

This `if` is getting quite complex. I don't think that saving one SPI write 
transaction is worth the effort here. What about just setting the chip to a 
known-good state unconditionally by removing that `if`?

The condition also looks wrong to me (as-is in the kernel now). If the chip 
is somehow configured to use active-high IRQ and the DT does not ask for 
either active-high or open-drain IRQ, then the reconfiguration gets skipped 
and the chip runs with wrong settings.

>  		/* mcp23s17 has IOCON twice, make sure they are in sync */
>  		status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
>  		status |= IOCON_HAEN | (IOCON_HAEN << 8);
> @@ -898,6 +900,9 @@ static int mcp23s08_probe_one(struct 
> mcp23s08 *mcp, struct device *dev,
>  		if (mirror)
>  			status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
>  
> +		if (open_drain)
> +			status |= IOCON_ODR | (IOCON_ODR << 8);
> +
>  		if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
>  			status |= IOCON_INTCC | (IOCON_INTCC << 8);

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:

+		if (irq_active_high && irq_open_drain) {
+			dev_err(dev, "microchip,irq-open-drain and "
+				"drive-active-high are mutually exclusive\n");
+			ret = -EINVAL;
+			goto fail;
+		} else if (irq_active_high) {
+			...
+		} else if (irq_open_drain) {
+			...
+		}

But anyway, even as-is, the patch is a net improvement, so:

Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz>

With kind regards,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 20/02/2018 05:43, Jan Kundrát wrote:
> Thanks for looking into this.
> 
> Is there a reason why the DT documentation is updated in a separate commit?
> 
>>      if ((status & IOCON_SEQOP) || !(status & IOCON_HAEN) || mirror ||
>> -         mcp->irq_active_high) {
>> +         mcp->irq_active_high || open_drain) {
> 
> This `if` is getting quite complex. I don't think that saving one SPI write transaction is worth the effort here. What about just setting the chip to a 
> known-good state unconditionally by removing that `if`?
> 
> The condition also looks wrong to me (as-is in the kernel now). If the chip is somehow configured to use active-high IRQ and the DT does not ask for either 
> active-high or open-drain IRQ, then the reconfiguration gets skipped and the chip runs with wrong settings.

I agree, I don't think we should be preserving any of the bits in the chip config.
But I think we need that as a separate patch for people to mull over.

> 
>>          /* mcp23s17 has IOCON twice, make sure they are in sync */
>>          status &= ~(IOCON_SEQOP | (IOCON_SEQOP << 8));
>>          status |= IOCON_HAEN | (IOCON_HAEN << 8);
>> @@ -898,6 +900,9 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
>>          if (mirror)
>>              status |= IOCON_MIRROR | (IOCON_MIRROR << 8);
>>
>> +        if (open_drain)
>> +            status |= IOCON_ODR | (IOCON_ODR << 8);
>> +
>>          if (type == MCP_TYPE_S18 || type == MCP_TYPE_018)
>>              status |= IOCON_INTCC | (IOCON_INTCC << 8);
> 
> 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.


> 
> +        if (irq_active_high && irq_open_drain) {
> +            dev_err(dev, "microchip,irq-open-drain and "
> +                "drive-active-high are mutually exclusive\n");
> +            ret = -EINVAL;
> +            goto fail;
> +        } else if (irq_active_high) {
> +            ...
> +        } else if (irq_open_drain) {
> +            ...
> +        }
> 
> But anyway, even as-is, the patch is a net improvement, so:
> 
> Reviewed-by: Jan Kundrát <jan.kundrat@cesnet.cz>
> 
> With kind regards,
> Jan
> 
>

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

So, in this context:

- pinctrl-mcp23s08.c should only specify IRQF_ONESHOT | IRQF_SHARED
- 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
- 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 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?

With kind regards,
Jan

[1] https://www.spinics.net/lists/devicetree/msg60203.html
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

On Tuesday, February 20, 2018, 6:15:46 PM CET Jan Kundrát wrote:
> 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.
> 
> So, in this context:
> 
> - pinctrl-mcp23s08.c should only specify IRQF_ONESHOT | IRQF_SHARED
> - 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
> - 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. 

Well, that is the exact situation on the board I had to deal with. The MCP was 
attached to a transisitor inverting the signal. The output on MCP has to be 
push-pull as there was no pull-up oder -down. But the line connecting the 
inverter to the CPU was an open-drain one and this line was actually a shared 
IRQ line. So of course the line bewteen MCP and inverter cannot be shared, but 
the IRQ used on CPU is actually shared. How can this be respresented in DT?

Best reagrds,
Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 21/02/2018 15:11, Alexander Stein wrote:
> On Tuesday, February 20, 2018, 6:15:46 PM CET Jan Kundrát wrote:
>> 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.
>>
>> So, in this context:
>>
>> - pinctrl-mcp23s08.c should only specify IRQF_ONESHOT | IRQF_SHARED
>> - 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
>> - 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.
> 
> Well, that is the exact situation on the board I had to deal with. The MCP was
> attached to a transisitor inverting the signal. The output on MCP has to be
> push-pull as there was no pull-up oder -down. But the line connecting the
> inverter to the CPU was an open-drain one and this line was actually a shared
> IRQ line. So of course the line bewteen MCP and inverter cannot be shared, but
> the IRQ used on CPU is actually shared. How can this be respresented in DT?
> 
G'day Alexandar,
It can't at the moment. Linux W. proposed modelling a inverter. Basically a irqchip driver
that flips the polarity of the interrupt. I was going to give it a go when I found time.
But it's not a high priority for me.

In your patch at: https://www.spinics.net/lists/devicetree/msg58208.html
This bit in mcp23s08_irq_setup
+	if (mcp->irq_active_high)
+		irqflags |= IRQF_TRIGGER_HIGH;
+	else
+		irqflags |= IRQF_TRIGGER_LOW;

is causing me problems.

It overrides the setting from the dt for me.
Probably due to this bit in __setup_irq:
	/*
	 * If the trigger type is not specified by the caller,
	 * then use the default for this interrupt.
	 */
	if (!(new->flags & IRQF_TRIGGER_MASK))
		new->flags |= irqd_get_trigger_type(&desc->irq_data);


Based on your description I would have thought the same.
With a transisitor inverting the line you'd want to register irq active_low,
while driving active_high to turn the transitor on.
Was this the case?

On středa 21. února 2018 8:11:25 CET, Alexander Stein wrote:
> Well, that is the exact situation on the board I had to deal 
> with. The MCP was 
> attached to a transisitor inverting the signal. The output on MCP has to be 
> push-pull as there was no pull-up oder -down. But the line connecting the 
> inverter to the CPU was an open-drain one and this line was 
> actually a shared 
> IRQ line. So of course the line bewteen MCP and inverter cannot 
> be shared, but 
> the IRQ used on CPU is actually shared. How can this be respresented in DT?

What I propose is to have it like this:

	gpio@1 {
		compatible = "microchip,mcp23s17";
		irq-active-high;
		interrupt-parent = <&gpio0>;
		interrupts = <123 IRQ_TYPE_LEVEL_LOW>;
		// ...
	};

In other words, the "interrupt-parent" and "interrupts" DT statements 
specify how the upstream interrupt controller should be set up, while other 
properties define settings of the IRQ source.

But as Phil points out, such a change would break the DT ABI. This means 
that we should preserve the semantics of any existing DT. We can do that by 
introducing another property. Here's a rough sketch:

#define MCP23S08_IRQ_OPEN_DRAIN 1
#define MCP23S08_IRQ_PUSH_PULL_ACTIVE_LOW 2
#define MCP23S08_IRQ_PUSH_PULL_ACTIVE_HIGH 3

	irqflags = IRQF_SHARED | IRQF_ONESHOT;

	if (new_irq_mode != -1) {
		/* irqflags are only set to IRQF_SHARED | IRQF_ONESHOT,
		nothing else */
	} else if (irq_active_high) {
		/* an existing property */
		irqflags |= IRQF_ACTIVE_HIGH;
	} else {
		irqflags |= IRQF_ACTIVE_LOW;
	}

A "new-style" DT then should look like this for your scenario with an 
electrical invertor:

	gpio@1 {
		compatible = "microchip,mcp23s17";
		irq-mode = MCP23S08_IRQ_PUSH_PULL_ACTIVE_HIGH;
		interrupt-parent = <&gpio0>;
		interrupts = <123 IRQ_TYPE_LEVEL_LOW>;
		// ...
	};

A downside of this approach is that it's using a custom enum, not a 
standard property like drive-open-drain. Pick your poison.

Cheers,
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Mon, Feb 19, 2018 at 10:25 AM, Phil Reid <preid@electromag.com.au> wrote:

> The mcp23s08 series device can be configured for wired and interrupts
> using an external pull-up and open drain output via the IOCON_ODR bit.
> And "drive-open-drain" property to enable this.
>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Patch applied with Jan's review tag as it is (as he says)
a net improvement.

But as the discussion shows, maybe we need more patching
on top of this.

Anyways nice to get this in so we can have some base to
work from.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html