* [PATCH] input: eglx_ts, remove irq trigger flags @ 2015-03-12 14:50 Markus Pargmann 2015-03-12 15:18 ` Philipp Zabel 0 siblings, 1 reply; 7+ messages in thread From: Markus Pargmann @ 2015-03-12 14:50 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input, linux-kernel, kernel, Markus Pargmann The trigger settings for a given irq are parsed from DT. Defining them as flag for devm_request_threaded_irq() overwrites these settings. This results in wrong trigger settings for boards which have different irq triggers. Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- drivers/input/touchscreen/egalax_ts.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c index 4c56299284ef..b0e6448b743c 100644 --- a/drivers/input/touchscreen/egalax_ts.c +++ b/drivers/input/touchscreen/egalax_ts.c @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, error = devm_request_threaded_irq(&client->dev, client->irq, NULL, egalax_ts_interrupt, - IRQF_TRIGGER_LOW | IRQF_ONESHOT, + IRQF_ONESHOT, "egalax_ts", ts); if (error < 0) { dev_err(&client->dev, "Failed to register interrupt\n"); -- 2.1.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] input: eglx_ts, remove irq trigger flags 2015-03-12 14:50 [PATCH] input: eglx_ts, remove irq trigger flags Markus Pargmann @ 2015-03-12 15:18 ` Philipp Zabel 2015-03-12 15:37 ` Markus Pargmann 0 siblings, 1 reply; 7+ messages in thread From: Philipp Zabel @ 2015-03-12 15:18 UTC (permalink / raw) To: Markus Pargmann; +Cc: Dmitry Torokhov, linux-kernel, kernel, linux-input Hi Markus, Am Donnerstag, den 12.03.2015, 15:50 +0100 schrieb Markus Pargmann: > The trigger settings for a given irq are parsed from DT. Defining them > as flag for devm_request_threaded_irq() overwrites these settings. This > results in wrong trigger settings for boards which have different irq > triggers. > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > --- > drivers/input/touchscreen/egalax_ts.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > index 4c56299284ef..b0e6448b743c 100644 > --- a/drivers/input/touchscreen/egalax_ts.c > +++ b/drivers/input/touchscreen/egalax_ts.c > @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, > > error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > egalax_ts_interrupt, > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > + IRQF_ONESHOT, > "egalax_ts", ts); > if (error < 0) { > dev_err(&client->dev, "Failed to register interrupt\n"); There are three device trees which have eeti,egalax_ts nodes with interrupt flags 0: arch/arm/boot/dts/imx53-tx53-x13x.dts (twice), arch/arm/boot/dts/imx6dl-tx6u-811x.dts, and arch/arm/boot/dts/imx6q-tx6q-1110.dts. Will these still work after this change? regards Philipp ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] input: eglx_ts, remove irq trigger flags 2015-03-12 15:18 ` Philipp Zabel @ 2015-03-12 15:37 ` Markus Pargmann 2015-03-12 16:28 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: Markus Pargmann @ 2015-03-12 15:37 UTC (permalink / raw) To: Philipp Zabel; +Cc: Dmitry Torokhov, linux-kernel, kernel, linux-input [-- Attachment #1: Type: text/plain, Size: 1886 bytes --] Hi, On Thu, Mar 12, 2015 at 04:18:03PM +0100, Philipp Zabel wrote: > Hi Markus, > > Am Donnerstag, den 12.03.2015, 15:50 +0100 schrieb Markus Pargmann: > > The trigger settings for a given irq are parsed from DT. Defining them > > as flag for devm_request_threaded_irq() overwrites these settings. This > > results in wrong trigger settings for boards which have different irq > > triggers. > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > --- > > drivers/input/touchscreen/egalax_ts.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > > index 4c56299284ef..b0e6448b743c 100644 > > --- a/drivers/input/touchscreen/egalax_ts.c > > +++ b/drivers/input/touchscreen/egalax_ts.c > > @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, > > > > error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > egalax_ts_interrupt, > > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > + IRQF_ONESHOT, > > "egalax_ts", ts); > > if (error < 0) { > > dev_err(&client->dev, "Failed to register interrupt\n"); > > There are three device trees which have eeti,egalax_ts nodes with > interrupt flags 0: > > arch/arm/boot/dts/imx53-tx53-x13x.dts (twice), > arch/arm/boot/dts/imx6dl-tx6u-811x.dts, and > arch/arm/boot/dts/imx6q-tx6q-1110.dts. > > Will these still work after this change? Oh right, thanks, these should be fixed as well. Best regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] input: eglx_ts, remove irq trigger flags 2015-03-12 15:37 ` Markus Pargmann @ 2015-03-12 16:28 ` Dmitry Torokhov 2015-03-13 7:14 ` Markus Pargmann 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2015-03-12 16:28 UTC (permalink / raw) To: Markus Pargmann; +Cc: Philipp Zabel, linux-kernel, kernel, linux-input On Thu, Mar 12, 2015 at 04:37:01PM +0100, Markus Pargmann wrote: > Hi, > > On Thu, Mar 12, 2015 at 04:18:03PM +0100, Philipp Zabel wrote: > > Hi Markus, > > > > Am Donnerstag, den 12.03.2015, 15:50 +0100 schrieb Markus Pargmann: > > > The trigger settings for a given irq are parsed from DT. Defining them > > > as flag for devm_request_threaded_irq() overwrites these settings. This > > > results in wrong trigger settings for boards which have different irq > > > triggers. > > > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > --- > > > drivers/input/touchscreen/egalax_ts.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > > > index 4c56299284ef..b0e6448b743c 100644 > > > --- a/drivers/input/touchscreen/egalax_ts.c > > > +++ b/drivers/input/touchscreen/egalax_ts.c > > > @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, > > > > > > error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > > egalax_ts_interrupt, > > > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > > + IRQF_ONESHOT, > > > "egalax_ts", ts); > > > if (error < 0) { > > > dev_err(&client->dev, "Failed to register interrupt\n"); > > > > There are three device trees which have eeti,egalax_ts nodes with > > interrupt flags 0: > > > > arch/arm/boot/dts/imx53-tx53-x13x.dts (twice), > > arch/arm/boot/dts/imx6dl-tx6u-811x.dts, and > > arch/arm/boot/dts/imx6q-tx6q-1110.dts. > > > > Will these still work after this change? > > Oh right, thanks, these should be fixed as well. If by fixing you mean changing DTS I do not think we can do that. Maybe the driver should check if there is non-empty trigger flags in the interrupt description and fall back to IRQF_TRIGGER_LOW if they are absent. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] input: eglx_ts, remove irq trigger flags 2015-03-12 16:28 ` Dmitry Torokhov @ 2015-03-13 7:14 ` Markus Pargmann 2015-03-18 16:50 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: Markus Pargmann @ 2015-03-13 7:14 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Philipp Zabel, linux-kernel, kernel, linux-input [-- Attachment #1: Type: text/plain, Size: 3259 bytes --] On Thu, Mar 12, 2015 at 09:28:13AM -0700, Dmitry Torokhov wrote: > On Thu, Mar 12, 2015 at 04:37:01PM +0100, Markus Pargmann wrote: > > Hi, > > > > On Thu, Mar 12, 2015 at 04:18:03PM +0100, Philipp Zabel wrote: > > > Hi Markus, > > > > > > Am Donnerstag, den 12.03.2015, 15:50 +0100 schrieb Markus Pargmann: > > > > The trigger settings for a given irq are parsed from DT. Defining them > > > > as flag for devm_request_threaded_irq() overwrites these settings. This > > > > results in wrong trigger settings for boards which have different irq > > > > triggers. > > > > > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > > --- > > > > drivers/input/touchscreen/egalax_ts.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > > > > index 4c56299284ef..b0e6448b743c 100644 > > > > --- a/drivers/input/touchscreen/egalax_ts.c > > > > +++ b/drivers/input/touchscreen/egalax_ts.c > > > > @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, > > > > > > > > error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > > > egalax_ts_interrupt, > > > > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > > > + IRQF_ONESHOT, > > > > "egalax_ts", ts); > > > > if (error < 0) { > > > > dev_err(&client->dev, "Failed to register interrupt\n"); > > > > > > There are three device trees which have eeti,egalax_ts nodes with > > > interrupt flags 0: > > > > > > arch/arm/boot/dts/imx53-tx53-x13x.dts (twice), > > > arch/arm/boot/dts/imx6dl-tx6u-811x.dts, and > > > arch/arm/boot/dts/imx6q-tx6q-1110.dts. > > > > > > Will these still work after this change? > > > > Oh right, thanks, these should be fixed as well. > > If by fixing you mean changing DTS I do not think we can do that. Maybe > the driver should check if there is non-empty trigger flags in the > interrupt description and fall back to IRQF_TRIGGER_LOW if they are > absent. I think this is more of a driver/dts bug. The devicetree binding documentation for the egalax_ts [1] does not describe the interrupts flags that should be set or that the flag should be 0. Even the example shows the interrupt flags being '2'(trigger high-to-low edge). These flags are described by the interrupt-controller. The generic interrupt-controller bindings describe the second parameter for interrupts as interrupt trigger flags [2]. So I think having a '0' as second parameter in the 'interrupts' field is a wrong representation of the actual hardware. This of course works as long as the driver ignores the trigger settings from DT which is not specified in the bindings. Best Regards, Markus [1] Documentation/devicetree/bindings/input/touchscreen/egalax-ts.txt [2] Documentation/devicetree/bindings/interrupt-controller/interrupts.txt -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] input: eglx_ts, remove irq trigger flags 2015-03-13 7:14 ` Markus Pargmann @ 2015-03-18 16:50 ` Dmitry Torokhov 2015-03-24 10:46 ` Markus Pargmann 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Torokhov @ 2015-03-18 16:50 UTC (permalink / raw) To: Markus Pargmann; +Cc: Philipp Zabel, linux-kernel, kernel, linux-input On Fri, Mar 13, 2015 at 08:14:47AM +0100, Markus Pargmann wrote: > On Thu, Mar 12, 2015 at 09:28:13AM -0700, Dmitry Torokhov wrote: > > On Thu, Mar 12, 2015 at 04:37:01PM +0100, Markus Pargmann wrote: > > > Hi, > > > > > > On Thu, Mar 12, 2015 at 04:18:03PM +0100, Philipp Zabel wrote: > > > > Hi Markus, > > > > > > > > Am Donnerstag, den 12.03.2015, 15:50 +0100 schrieb Markus Pargmann: > > > > > The trigger settings for a given irq are parsed from DT. Defining them > > > > > as flag for devm_request_threaded_irq() overwrites these settings. This > > > > > results in wrong trigger settings for boards which have different irq > > > > > triggers. > > > > > > > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > > > --- > > > > > drivers/input/touchscreen/egalax_ts.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > > > > > index 4c56299284ef..b0e6448b743c 100644 > > > > > --- a/drivers/input/touchscreen/egalax_ts.c > > > > > +++ b/drivers/input/touchscreen/egalax_ts.c > > > > > @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, > > > > > > > > > > error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > > > > egalax_ts_interrupt, > > > > > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > > > > + IRQF_ONESHOT, > > > > > "egalax_ts", ts); > > > > > if (error < 0) { > > > > > dev_err(&client->dev, "Failed to register interrupt\n"); > > > > > > > > There are three device trees which have eeti,egalax_ts nodes with > > > > interrupt flags 0: > > > > > > > > arch/arm/boot/dts/imx53-tx53-x13x.dts (twice), > > > > arch/arm/boot/dts/imx6dl-tx6u-811x.dts, and > > > > arch/arm/boot/dts/imx6q-tx6q-1110.dts. > > > > > > > > Will these still work after this change? > > > > > > Oh right, thanks, these should be fixed as well. > > > > If by fixing you mean changing DTS I do not think we can do that. Maybe > > the driver should check if there is non-empty trigger flags in the > > interrupt description and fall back to IRQF_TRIGGER_LOW if they are > > absent. > > I think this is more of a driver/dts bug. The devicetree binding > documentation for the egalax_ts [1] does not describe the interrupts > flags that should be set or that the flag should be 0. Even the example > shows the interrupt flags being '2'(trigger high-to-low edge). These > flags are described by the interrupt-controller. The generic > interrupt-controller bindings describe the second parameter for > interrupts as interrupt trigger flags [2]. > > So I think having a '0' as second parameter in the 'interrupts' field is a > wrong representation of the actual hardware. This of course works as > long as the driver ignores the trigger settings from DT which is not > specified in the bindings. The point is that we should not break boards with existing DTS. Yes, DTS may be wrong, not optimal, whatever, but if it used to work we need to keep it working. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] input: eglx_ts, remove irq trigger flags 2015-03-18 16:50 ` Dmitry Torokhov @ 2015-03-24 10:46 ` Markus Pargmann 0 siblings, 0 replies; 7+ messages in thread From: Markus Pargmann @ 2015-03-24 10:46 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: Philipp Zabel, linux-kernel, kernel, linux-input [-- Attachment #1: Type: text/plain, Size: 3832 bytes --] On Wed, Mar 18, 2015 at 09:50:03AM -0700, Dmitry Torokhov wrote: > On Fri, Mar 13, 2015 at 08:14:47AM +0100, Markus Pargmann wrote: > > On Thu, Mar 12, 2015 at 09:28:13AM -0700, Dmitry Torokhov wrote: > > > On Thu, Mar 12, 2015 at 04:37:01PM +0100, Markus Pargmann wrote: > > > > Hi, > > > > > > > > On Thu, Mar 12, 2015 at 04:18:03PM +0100, Philipp Zabel wrote: > > > > > Hi Markus, > > > > > > > > > > Am Donnerstag, den 12.03.2015, 15:50 +0100 schrieb Markus Pargmann: > > > > > > The trigger settings for a given irq are parsed from DT. Defining them > > > > > > as flag for devm_request_threaded_irq() overwrites these settings. This > > > > > > results in wrong trigger settings for boards which have different irq > > > > > > triggers. > > > > > > > > > > > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de> > > > > > > --- > > > > > > drivers/input/touchscreen/egalax_ts.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/input/touchscreen/egalax_ts.c b/drivers/input/touchscreen/egalax_ts.c > > > > > > index 4c56299284ef..b0e6448b743c 100644 > > > > > > --- a/drivers/input/touchscreen/egalax_ts.c > > > > > > +++ b/drivers/input/touchscreen/egalax_ts.c > > > > > > @@ -218,7 +218,7 @@ static int egalax_ts_probe(struct i2c_client *client, > > > > > > > > > > > > error = devm_request_threaded_irq(&client->dev, client->irq, NULL, > > > > > > egalax_ts_interrupt, > > > > > > - IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > > > > > + IRQF_ONESHOT, > > > > > > "egalax_ts", ts); > > > > > > if (error < 0) { > > > > > > dev_err(&client->dev, "Failed to register interrupt\n"); > > > > > > > > > > There are three device trees which have eeti,egalax_ts nodes with > > > > > interrupt flags 0: > > > > > > > > > > arch/arm/boot/dts/imx53-tx53-x13x.dts (twice), > > > > > arch/arm/boot/dts/imx6dl-tx6u-811x.dts, and > > > > > arch/arm/boot/dts/imx6q-tx6q-1110.dts. > > > > > > > > > > Will these still work after this change? > > > > > > > > Oh right, thanks, these should be fixed as well. > > > > > > If by fixing you mean changing DTS I do not think we can do that. Maybe > > > the driver should check if there is non-empty trigger flags in the > > > interrupt description and fall back to IRQF_TRIGGER_LOW if they are > > > absent. > > > > I think this is more of a driver/dts bug. The devicetree binding > > documentation for the egalax_ts [1] does not describe the interrupts > > flags that should be set or that the flag should be 0. Even the example > > shows the interrupt flags being '2'(trigger high-to-low edge). These > > flags are described by the interrupt-controller. The generic > > interrupt-controller bindings describe the second parameter for > > interrupts as interrupt trigger flags [2]. > > > > So I think having a '0' as second parameter in the 'interrupts' field is a > > wrong representation of the actual hardware. This of course works as > > long as the driver ignores the trigger settings from DT which is not > > specified in the bindings. > > The point is that we should not break boards with existing DTS. Yes, DTS > may be wrong, not optimal, whatever, but if it used to work we need to > keep it working. I don't really like this as this wrong behaviour is not defined in the binding. However I will add a check for an empty interrupt trigger to not break the old DTS files. Best regards, Markus -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-24 10:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-12 14:50 [PATCH] input: eglx_ts, remove irq trigger flags Markus Pargmann 2015-03-12 15:18 ` Philipp Zabel 2015-03-12 15:37 ` Markus Pargmann 2015-03-12 16:28 ` Dmitry Torokhov 2015-03-13 7:14 ` Markus Pargmann 2015-03-18 16:50 ` Dmitry Torokhov 2015-03-24 10:46 ` Markus Pargmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).