[v2,6/7] pinctrl: mcp23s08: remove hardcoded irq polarity in irq_setup

Message ID 1507266491-73971-7-git-send-email-preid@electromag.com.au
State New
Headers show
Series
  • pinctrl: mcp32s08: add support for mcp23018
Related show

Commit Message

Phil Reid Oct. 6, 2017, 5:08 a.m.
The irq_active_high flag is for controlling the polarity of the output
from the mcp23s08 series devices. The polarity of the irq could be altered
by additional logic (eg inverters) between the device and irq input device.
The device-tree already allows for this as the irq can be specified in the
binding. So hardcoding it in the driver is overly restrictive.

There are no inkernel users of the mcp23s08 driver with irq's.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 5 -----
 1 file changed, 5 deletions(-)

Comments

Sebastian Reichel Oct. 8, 2017, 9:14 p.m. | #1
Hi,

On Fri, Oct 06, 2017 at 01:08:10PM +0800, Phil Reid wrote:
> The irq_active_high flag is for controlling the polarity of the output
> from the mcp23s08 series devices. The polarity of the irq could be altered
> by additional logic (eg inverters) between the device and irq input device.
> The device-tree already allows for this as the irq can be specified in the
> binding. So hardcoding it in the driver is overly restrictive.
> 
> There are no inkernel users of the mcp23s08 driver with irq's.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

> ---
>  drivers/pinctrl/pinctrl-mcp23s08.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> index 150f216..8dceaa1 100644
> --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> @@ -630,11 +630,6 @@ static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
>  	int err;
>  	unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED;
>  
> -	if (mcp->irq_active_high)
> -		irqflags |= IRQF_TRIGGER_HIGH;
> -	else
> -		irqflags |= IRQF_TRIGGER_LOW;
> -
>  	err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL,
>  					mcp23s08_irq,
>  					irqflags, dev_name(chip->parent), mcp);
> -- 
> 1.8.3.1
>
Linus Walleij Oct. 11, 2017, 8:17 a.m. | #2
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote:

> The irq_active_high flag is for controlling the polarity of the output
> from the mcp23s08 series devices. The polarity of the irq could be altered
> by additional logic (eg inverters) between the device and irq input device.
> The device-tree already allows for this as the irq can be specified in the
> binding. So hardcoding it in the driver is overly restrictive.
>
> There are no inkernel users of the mcp23s08 driver with irq's.
>
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Look this up from the irq descriptor as discussed in 4/7.

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
Linus Walleij Oct. 12, 2017, 9:04 a.m. | #3
On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote:

> The polarity of the irq could be altered
> by additional logic (eg inverters) between the device and irq input device.

Marc Zyngier has pointed out that inverters should be modeled
using hierarchichal irq domains. You would need to add a new
irqchip to invert the line.

See this PDF:
https://elinux.org/images/8/8c/Zyngier.pdf

drivers/irqchips/irq-uniphier-aidet.c supports inversion of IRQs
for example. For this it uses irq_domain_create_hierarchy()

I guess it would be helpful with a reusable generic "inverter
irq chip", that you can just enable and slam into your device
tree to tell the system that a line is statically inverted, i.e.
non-programmable logic on the board.

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
Phil Reid Oct. 12, 2017, 2:27 p.m. | #4
On 12/10/2017 17:04, Linus Walleij wrote:
> On Fri, Oct 6, 2017 at 7:08 AM, Phil Reid <preid@electromag.com.au> wrote:
> 
>> The polarity of the irq could be altered
>> by additional logic (eg inverters) between the device and irq input device.
> 
> Marc Zyngier has pointed out that inverters should be modeled
> using hierarchichal irq domains. You would need to add a new
> irqchip to invert the line.
> 
> See this PDF:
> https://elinux.org/images/8/8c/Zyngier.pdf
> 
> drivers/irqchips/irq-uniphier-aidet.c supports inversion of IRQs
> for example. For this it uses irq_domain_create_hierarchy()
> 
> I guess it would be helpful with a reusable generic "inverter
> irq chip", that you can just enable and slam into your device
> tree to tell the system that a line is statically inverted, i.e.
> non-programmable logic on the board.
> 
G'day Linus.
Thanks for the pointers, I'll see what I can come up with.

Patch

diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
index 150f216..8dceaa1 100644
--- a/drivers/pinctrl/pinctrl-mcp23s08.c
+++ b/drivers/pinctrl/pinctrl-mcp23s08.c
@@ -630,11 +630,6 @@  static int mcp23s08_irq_setup(struct mcp23s08 *mcp)
 	int err;
 	unsigned long irqflags = IRQF_ONESHOT | IRQF_SHARED;
 
-	if (mcp->irq_active_high)
-		irqflags |= IRQF_TRIGGER_HIGH;
-	else
-		irqflags |= IRQF_TRIGGER_LOW;
-
 	err = devm_request_threaded_irq(chip->parent, mcp->irq, NULL,
 					mcp23s08_irq,
 					irqflags, dev_name(chip->parent), mcp);