All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drivers: tca8418: Change the interrupt type
@ 2016-11-07 14:40 Maxime Ripard
  2016-11-09  0:04 ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Ripard @ 2016-11-07 14:40 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, Maxime Ripard

The TCA8418 interrupt has a level trigger, not a edge one.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/input/keyboard/tca8418_keypad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
index 9002298698fc..b88b3696a2e1 100644
--- a/drivers/input/keyboard/tca8418_keypad.c
+++ b/drivers/input/keyboard/tca8418_keypad.c
@@ -360,7 +360,7 @@ static int tca8418_keypad_probe(struct i2c_client *client,
 		irq = gpio_to_irq(irq);
 
 	error = devm_request_threaded_irq(dev, irq, NULL, tca8418_irq_handler,
-					  IRQF_TRIGGER_FALLING |
+					  IRQF_TRIGGER_LOW |
 						IRQF_SHARED |
 						IRQF_ONESHOT,
 					  client->name, keypad_data);
-- 
2.10.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: tca8418: Change the interrupt type
  2016-11-07 14:40 [PATCH] drivers: tca8418: Change the interrupt type Maxime Ripard
@ 2016-11-09  0:04 ` Dmitry Torokhov
  2016-11-09  8:02   ` Maxime Ripard
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2016-11-09  0:04 UTC (permalink / raw)
  To: Maxime Ripard; +Cc: linux-input, linux-kernel

On Mon, Nov 07, 2016 at 03:40:24PM +0100, Maxime Ripard wrote:
> The TCA8418 interrupt has a level trigger, not a edge one.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Hmm, maybe we could rely on OF data for trigger type?

> ---
>  drivers/input/keyboard/tca8418_keypad.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index 9002298698fc..b88b3696a2e1 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c
> @@ -360,7 +360,7 @@ static int tca8418_keypad_probe(struct i2c_client *client,
>  		irq = gpio_to_irq(irq);
>  
>  	error = devm_request_threaded_irq(dev, irq, NULL, tca8418_irq_handler,
> -					  IRQF_TRIGGER_FALLING |
> +					  IRQF_TRIGGER_LOW |
>  						IRQF_SHARED |
>  						IRQF_ONESHOT,
>  					  client->name, keypad_data);
> -- 
> 2.10.1
> 

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: tca8418: Change the interrupt type
  2016-11-09  0:04 ` Dmitry Torokhov
@ 2016-11-09  8:02   ` Maxime Ripard
  2016-11-10  4:02     ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Ripard @ 2016-11-09  8:02 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

Hello Dmitry,

On Tue, Nov 08, 2016 at 04:04:00PM -0800, Dmitry Torokhov wrote:
> On Mon, Nov 07, 2016 at 03:40:24PM +0100, Maxime Ripard wrote:
> > The TCA8418 interrupt has a level trigger, not a edge one.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Hmm, maybe we could rely on OF data for trigger type?

We might, even though the i2c core doesn't change the trigger type
when it retrieves the interrupt from the DT.

However, I'm a bit worried about the other probing mechanims (ACPI,
board files) that should be supported as well, and removing the
trigger type from the flags might break those. There's no board files
using it though in the tree, but I don't know about ACPI systems.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: tca8418: Change the interrupt type
  2016-11-09  8:02   ` Maxime Ripard
@ 2016-11-10  4:02     ` Dmitry Torokhov
  2016-11-10  9:13       ` Maxime Ripard
  0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2016-11-10  4:02 UTC (permalink / raw)
  To: Maxime Ripard; +Cc: linux-input, linux-kernel

Hi Maxime,

On Wed, Nov 09, 2016 at 09:02:35AM +0100, Maxime Ripard wrote:
> Hello Dmitry,
> 
> On Tue, Nov 08, 2016 at 04:04:00PM -0800, Dmitry Torokhov wrote:
> > On Mon, Nov 07, 2016 at 03:40:24PM +0100, Maxime Ripard wrote:
> > > The TCA8418 interrupt has a level trigger, not a edge one.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Hmm, maybe we could rely on OF data for trigger type?
> 
> We might, even though the i2c core doesn't change the trigger type
> when it retrieves the interrupt from the DT.

i2c core itself does not, and should not, but irq code does:

of_irq_get() -> irq_create_of_mapping() -> irq_create_fwspec_mapping()
-> irqd_set_trigger_type().

> 
> However, I'm a bit worried about the other probing mechanims (ACPI,
> board files) that should be supported as well, and removing the
> trigger type from the flags might break those. There's no board files
> using it though in the tree, but I don't know about ACPI systems.

The driver is not enabled for ACPI systems, at least not in mainline.

By the way, this is what TCA8418 binding dochas to say:

"- interrupts: IRQ line number, should trigger on falling edge"

so it seems there was at least one system that needed falling edge and
not level interrupt.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: tca8418: Change the interrupt type
  2016-11-10  4:02     ` Dmitry Torokhov
@ 2016-11-10  9:13       ` Maxime Ripard
  2016-11-12  1:24         ` Dmitry Torokhov
  0 siblings, 1 reply; 6+ messages in thread
From: Maxime Ripard @ 2016-11-10  9:13 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2006 bytes --]

On Wed, Nov 09, 2016 at 08:02:08PM -0800, Dmitry Torokhov wrote:
> On Wed, Nov 09, 2016 at 09:02:35AM +0100, Maxime Ripard wrote:
> > Hello Dmitry,
> > 
> > On Tue, Nov 08, 2016 at 04:04:00PM -0800, Dmitry Torokhov wrote:
> > > On Mon, Nov 07, 2016 at 03:40:24PM +0100, Maxime Ripard wrote:
> > > > The TCA8418 interrupt has a level trigger, not a edge one.
> > > > 
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > 
> > > Hmm, maybe we could rely on OF data for trigger type?
> > 
> > We might, even though the i2c core doesn't change the trigger type
> > when it retrieves the interrupt from the DT.
> 
> i2c core itself does not, and should not, but irq code does:
> 
> of_irq_get() -> irq_create_of_mapping() -> irq_create_fwspec_mapping()
> -> irqd_set_trigger_type().

Ah, indeed, I overlooked that. I wonder why platform_get_irq does it
then.

> 
> > 
> > However, I'm a bit worried about the other probing mechanims (ACPI,
> > board files) that should be supported as well, and removing the
> > trigger type from the flags might break those. There's no board files
> > using it though in the tree, but I don't know about ACPI systems.
> 
> The driver is not enabled for ACPI systems, at least not in mainline.

Ok. Good.

> By the way, this is what TCA8418 binding dochas to say:
> 
> "- interrupts: IRQ line number, should trigger on falling edge"
> 
> so it seems there was at least one system that needed falling edge and
> not level interrupt.

I don't know, looking at the datasheet, it really looks like it's
level triggered to me, and we were actually seeing issues when set in
edge.

http://www.ti.com/lit/ds/symlink/tca8418.pdf
Especially page 20 and 33.

My understanding is that in input, the chip will trigger on edges, but
the line coming from that device to the SoC will be level triggered.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] drivers: tca8418: Change the interrupt type
  2016-11-10  9:13       ` Maxime Ripard
@ 2016-11-12  1:24         ` Dmitry Torokhov
  0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2016-11-12  1:24 UTC (permalink / raw)
  To: Maxime Ripard; +Cc: linux-input, linux-kernel

On Thu, Nov 10, 2016 at 10:13:02AM +0100, Maxime Ripard wrote:
> On Wed, Nov 09, 2016 at 08:02:08PM -0800, Dmitry Torokhov wrote:
> > On Wed, Nov 09, 2016 at 09:02:35AM +0100, Maxime Ripard wrote:
> > > Hello Dmitry,
> > > 
> > > On Tue, Nov 08, 2016 at 04:04:00PM -0800, Dmitry Torokhov wrote:
> > > > On Mon, Nov 07, 2016 at 03:40:24PM +0100, Maxime Ripard wrote:
> > > > > The TCA8418 interrupt has a level trigger, not a edge one.
> > > > > 
> > > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > > 
> > > > Hmm, maybe we could rely on OF data for trigger type?
> > > 
> > > We might, even though the i2c core doesn't change the trigger type
> > > when it retrieves the interrupt from the DT.
> > 
> > i2c core itself does not, and should not, but irq code does:
> > 
> > of_irq_get() -> irq_create_of_mapping() -> irq_create_fwspec_mapping()
> > -> irqd_set_trigger_type().
> 
> Ah, indeed, I overlooked that. I wonder why platform_get_irq does it
> then.
> 
> > 
> > > 
> > > However, I'm a bit worried about the other probing mechanims (ACPI,
> > > board files) that should be supported as well, and removing the
> > > trigger type from the flags might break those. There's no board files
> > > using it though in the tree, but I don't know about ACPI systems.
> > 
> > The driver is not enabled for ACPI systems, at least not in mainline.
> 
> Ok. Good.
> 
> > By the way, this is what TCA8418 binding dochas to say:
> > 
> > "- interrupts: IRQ line number, should trigger on falling edge"
> > 
> > so it seems there was at least one system that needed falling edge and
> > not level interrupt.
> 
> I don't know, looking at the datasheet, it really looks like it's
> level triggered to me, and we were actually seeing issues when set in
> edge.
> 
> http://www.ti.com/lit/ds/symlink/tca8418.pdf
> Especially page 20 and 33.
> 
> My understanding is that in input, the chip will trigger on edges, but
> the line coming from that device to the SoC will be level triggered.

The issue is that it is not necessarily connected directly to the
SoC/AP. It could be on a daughterboard with any number of converters,
possibly inverting polarity, or doing level->edge, etc. So the only sane
solution is to leave it to device tree to describe the setup to the
driver.

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-12  1:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-07 14:40 [PATCH] drivers: tca8418: Change the interrupt type Maxime Ripard
2016-11-09  0:04 ` Dmitry Torokhov
2016-11-09  8:02   ` Maxime Ripard
2016-11-10  4:02     ` Dmitry Torokhov
2016-11-10  9:13       ` Maxime Ripard
2016-11-12  1:24         ` Dmitry Torokhov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.