triggering an gpio-irq on both edges

triggering an gpio-irq on both edges

[Uwe Kleine-König]

Hello,

My motivation for this mail is that currently mxs_gpio_set_irq_type
doesn't support type=IRQ_TYPE_EDGE_RISING|IRQ_TYPE_EDGE_FALLING because
the hardware doesn't support it.

The gpio-keys driver on the other hand wants a trigger in both
directions.

The mxc-gpio driver implements the missing hardware functionallity in
software (gpio_set_irq_type):

	set_irq_type:
		if (both edges requested)
			if (current value of gpio == 0)
				trigger on rising edge
			else
				trigger on falling edge

	handle_irq:
		if (both edges requested)
			trigger on other edge


This is racy though: If there is an edge after reading out the gpio
value and completion of the edge direction programming, you miss
interrupts. Also just switching the direction in the irq handler is
also wrong for the same reason.

I guess there are more machines than mxc and mxs that have the same
problem.

In my opinion this calls for a wrapper, something like:

	int gpio_irq_emulate_both_edges_set_irq_type(struct irq_data *d,
		unsigned int type, int (*set_irq_type)(...),
		int (*get_value)(...), struct something *privdata)
	{
		...
	}

	int gpio_irq_emulate_both_edges_handler(...)
	{

	}

Does this sound good and possible? I didn't find something like that
already implemented, but maybe I missed it?

Any thoughts (and be it only "The xyz driver is affected, too.") are
welcome.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

triggering an gpio-irq on both edges

[Philipp Zabel]

Am Donnerstag, den 13.09.2012, 10:19 +0200 schrieb Uwe Kleine-K?nig:
> Hello,
> 
> My motivation for this mail is that currently mxs_gpio_set_irq_type
> doesn't support type=IRQ_TYPE_EDGE_RISING|IRQ_TYPE_EDGE_FALLING because
> the hardware doesn't support it.
> 
> The gpio-keys driver on the other hand wants a trigger in both
> directions.
> 
> The mxc-gpio driver implements the missing hardware functionallity in
> software (gpio_set_irq_type):
> 
> 	set_irq_type:
> 		if (both edges requested)
> 			if (current value of gpio == 0)
> 				trigger on rising edge
> 			else
> 				trigger on falling edge
> 
> 	handle_irq:
> 		if (both edges requested)
> 			trigger on other edge
> 

The ASIC3 driver in drivers/mfd/asic3.c does the same in
asic3_irq_demux, asic3_irq_flip_edge and asic3_gpio_irq_type.

> This is racy though: If there is an edge after reading out the gpio
> value and completion of the edge direction programming, you miss
> interrupts. Also just switching the direction in the irq handler is
> also wrong for the same reason.
>
> I guess there are more machines than mxc and mxs that have the same
> problem.
> 
> In my opinion this calls for a wrapper, something like:
> 
> 	int gpio_irq_emulate_both_edges_set_irq_type(struct irq_data *d,
> 		unsigned int type, int (*set_irq_type)(...),
> 		int (*get_value)(...), struct something *privdata)
> 	{
> 		...
> 	}
> 
> 	int gpio_irq_emulate_both_edges_handler(...)
> 	{
> 
> 	}
> 
> Does this sound good and possible? I didn't find something like that
> already implemented, but maybe I missed it?
> 
> Any thoughts (and be it only "The xyz driver is affected, too.") are
> welcome.

regards
Philipp

triggering an gpio-irq on both edges

[Linus Walleij]

On Thu, Sep 13, 2012 at 10:19 AM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:

> I guess there are more machines than mxc and mxs that have the same
> problem.

The COH901 GPIO driver has the same issue, see
drivers/pinctrl/pinctrl-coh901.c, functions named
u300_toggle_trigger() etc.

It will toggle the trigger from rising to falling on every
arriving IRQ by calling u300_toggle_trigger() from
fastpath in the IRQ handler.

Theoretically it is racy, but since it's used for GPIO
and these signals tend to not be super-fast we're safe.
It's used for card detection on SD cards and such.

> In my opinion this calls for a wrapper, something like:
>
>         int gpio_irq_emulate_both_edges_set_irq_type(struct irq_data *d,
>                 unsigned int type, int (*set_irq_type)(...),
>                 int (*get_value)(...), struct something *privdata)
>         {
>                 ...
>         }
>
>         int gpio_irq_emulate_both_edges_handler(...)
>         {
>
>         }
>
> Does this sound good and possible? I didn't find something like that
> already implemented, but maybe I missed it?

I've thought about this too but never thought there was something
generic enough to do about it...

Yours,
Linus Walleij