| Message ID | 1407372486-25881-1-git-send-email-javier.martinez@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, On 07.08.2014 02:48, Javier Martinez Canillas wrote: > The Atmel maXTouch driver assumed that the IRQ type flags will > always be passed using platform data but this is not true when > booting using Device Trees. In these setups the interrupt type > was ignored by the driver when requesting an IRQ. > > This means that it will fail if a machine specified other type > than IRQ_TYPE_NONE. The right approach is to get the IRQ flags > that was parsed by OF from the "interrupt" Device Tree propery. Have you observed an actual failure due to this? I believe that irq_of_parse_and_map() already sets up IRQ trigger type based on DT data, by calling irq_create_of_mapping() which in turn calls irq_set_irq_type(). > > Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 03b8571..0fb56c9 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -22,6 +22,7 @@ > #include <linux/i2c.h> > #include <linux/i2c/atmel_mxt_ts.h> > #include <linux/input/mt.h> > +#include <linux/irq.h> > #include <linux/interrupt.h> > #include <linux/of.h> > #include <linux/slab.h> > @@ -2130,6 +2131,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > struct mxt_data *data; > const struct mxt_platform_data *pdata; > int error; > + unsigned long irqflags; > > pdata = dev_get_platdata(&client->dev); > if (!pdata) { > @@ -2156,8 +2158,13 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > init_completion(&data->reset_completion); > init_completion(&data->crc_completion); > > + if (client->dev.of_node) > + irqflags = irq_get_trigger_type(client->irq); It might be a bit cleaner to just assign the flags to pdata->irqflags in mxt_parse_dt() instead. That would also account for the fact that pdata, if provided, should have priority over DT. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Tomasz, Thanks a lot for your feedback. On 08/07/2014 03:14 AM, Tomasz Figa wrote: > Hi Javier, > > > Have you observed an actual failure due to this? I believe that Yes, I found this issue since the driver was not taking into account the value defined in the edge/level type cells from the "interrupts" DT property. Only doing the change in the following patch was not enough: [PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad [0]. > irq_of_parse_and_map() already sets up IRQ trigger type based on DT > data, by calling irq_create_of_mapping() which in turn calls > irq_set_irq_type(). > Right but somehow when the IRQ is actually requested the type is overwritten by the value passed to request_threaded_irq() and interrupts are not being generated by the device without this patch. Do you think that this is a bug in the "interrupt-parent" irqchip driver or the IRQ core? I'm not that familiar with the IRQ subsystem. >> >> + if (client->dev.of_node) >> + irqflags = irq_get_trigger_type(client->irq); > > It might be a bit cleaner to just assign the flags to pdata->irqflags in > mxt_parse_dt() instead. That would also account for the fact that pdata, > if provided, should have priority over DT. > You are totally right, also this will break if CONFIG_OF is not enabled since dev.of_node will not be defined. While this already is taken into account for mxt_parse_dt() by defining an empty function. I'll change it in v2 if getting the flags from the driver is the right approach instead of fixing the irqchip driver or the IRQ core. > Best regards, > Tomasz > Best regards, Javier [0]: http://www.spinics.net/lists/kernel/msg1802099.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 07, 2014 at 03:47:24AM +0200, Javier Martinez Canillas wrote: > Hello Tomasz, > > Thanks a lot for your feedback. > > On 08/07/2014 03:14 AM, Tomasz Figa wrote: > > Hi Javier, > > > > > > Have you observed an actual failure due to this? I believe that > > Yes, I found this issue since the driver was not taking into account the value > defined in the edge/level type cells from the "interrupts" DT property. > > Only doing the change in the following patch was not enough: > > [PATCH 1/1] ARM: dts: Add Peach Pit and Pi dts entry for atmel touchpad [0]. > > > irq_of_parse_and_map() already sets up IRQ trigger type based on DT > > data, by calling irq_create_of_mapping() which in turn calls > > irq_set_irq_type(). > > > > Right but somehow when the IRQ is actually requested the type is overwritten by > the value passed to request_threaded_irq() and interrupts are not being > generated by the device without this patch. > > Do you think that this is a bug in the "interrupt-parent" irqchip driver or the > IRQ core? I'm not that familiar with the IRQ subsystem. No, this is clearly driver fault - it smashed previously done setup with new flags. > > >> > >> + if (client->dev.of_node) > >> + irqflags = irq_get_trigger_type(client->irq); > > > > It might be a bit cleaner to just assign the flags to pdata->irqflags in > > mxt_parse_dt() instead. That would also account for the fact that pdata, > > if provided, should have priority over DT. > > > > You are totally right, also this will break if CONFIG_OF is not enabled since > dev.of_node will not be defined. While this already is taken into account for > mxt_parse_dt() by defining an empty function. > > I'll change it in v2 if getting the flags from the driver is the right approach Yes, please. > instead of fixing the irqchip driver or the IRQ core. Thanks.
Hello Dmitry, On 08/07/2014 08:09 AM, Dmitry Torokhov wrote: >> >> > irq_of_parse_and_map() already sets up IRQ trigger type based on DT >> > data, by calling irq_create_of_mapping() which in turn calls >> > irq_set_irq_type(). >> > >> >> Right but somehow when the IRQ is actually requested the type is overwritten by >> the value passed to request_threaded_irq() and interrupts are not being >> generated by the device without this patch. >> >> Do you think that this is a bug in the "interrupt-parent" irqchip driver or the >> IRQ core? I'm not that familiar with the IRQ subsystem. > > No, this is clearly driver fault - it smashed previously done setup with new > flags. > Thanks a lot for the clarification. That was my understanding as well but wanted to be sure. >> > >> > It might be a bit cleaner to just assign the flags to pdata->irqflags in >> > mxt_parse_dt() instead. That would also account for the fact that pdata, >> > if provided, should have priority over DT. >> > >> >> You are totally right, also this will break if CONFIG_OF is not enabled since >> dev.of_node will not be defined. While this already is taken into account for >> mxt_parse_dt() by defining an empty function. >> >> I'll change it in v2 if getting the flags from the driver is the right approach > > Yes, please. > Just posted a v2 [0] with Tomasz suggestion and the patch is indeed a lot cleaner. Best regards, Javier [0]: https://lkml.org/lkml/2014/8/7/82 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 07/08/14 01:48, Javier Martinez Canillas wrote: > The Atmel maXTouch driver assumed that the IRQ type flags will > always be passed using platform data but this is not true when > booting using Device Trees. In these setups the interrupt type > was ignored by the driver when requesting an IRQ. > > This means that it will fail if a machine specified other type > than IRQ_TYPE_NONE. The right approach is to get the IRQ flags > that was parsed by OF from the "interrupt" Device Tree propery. I do not believe you are correct about this. There is a bit of code here: http://lxr.free-electrons.com/source/kernel/irq/manage.c?v=3.16#L1172 which means that in the device tree case, if we call request_threaded_irq() with no trigger bits set, it will trust whatever it already there. I did test this back in July and it appeared to work correctly. Have you tested this change is actually necessary? -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Aug 07, 2014 at 09:49:49AM +0200, Javier Martinez Canillas wrote: > Hello Dmitry, > > On 08/07/2014 08:09 AM, Dmitry Torokhov wrote: > >> > >> > irq_of_parse_and_map() already sets up IRQ trigger type based on DT > >> > data, by calling irq_create_of_mapping() which in turn calls > >> > irq_set_irq_type(). > >> > > >> > >> Right but somehow when the IRQ is actually requested the type is overwritten by > >> the value passed to request_threaded_irq() and interrupts are not being > >> generated by the device without this patch. > >> > >> Do you think that this is a bug in the "interrupt-parent" irqchip driver or the > >> IRQ core? I'm not that familiar with the IRQ subsystem. > > > > No, this is clearly driver fault - it smashed previously done setup with new > > flags. > > > > Thanks a lot for the clarification. That was my understanding as well but wanted > to be sure. Actually, I take this back. In mainline everything as it should: if pdata does not specify particular trigger the flags requested end up being IRQF_ONESHOT, which should preserve trigger bits previously set up by the board or OF code. In Chrome kernel we have: /* Default to falling edge if no platform data provided */ irqflags = data->pdata ? data->pdata->irqflags : IRQF_TRIGGER_FALLING; error = request_threaded_irq(client->irq, NULL, mxt_interrupt, irqflags | IRQF_ONESHOT, client->name, data); which I believe should go away. If it is needed on ACPI systems we need to figure out how to do things we can do with OF there. Thanks.
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 03b8571..0fb56c9 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -22,6 +22,7 @@ #include <linux/i2c.h> #include <linux/i2c/atmel_mxt_ts.h> #include <linux/input/mt.h> +#include <linux/irq.h> #include <linux/interrupt.h> #include <linux/of.h> #include <linux/slab.h> @@ -2130,6 +2131,7 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) struct mxt_data *data; const struct mxt_platform_data *pdata; int error; + unsigned long irqflags; pdata = dev_get_platdata(&client->dev); if (!pdata) { @@ -2156,8 +2158,13 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) init_completion(&data->reset_completion); init_completion(&data->crc_completion); + if (client->dev.of_node) + irqflags = irq_get_trigger_type(client->irq); + else + irqflags = pdata->irqflags; + error = request_threaded_irq(client->irq, NULL, mxt_interrupt, - pdata->irqflags | IRQF_ONESHOT, + irqflags | IRQF_ONESHOT, client->name, data); if (error) { dev_err(&client->dev, "Failed to register interrupt\n");
The Atmel maXTouch driver assumed that the IRQ type flags will always be passed using platform data but this is not true when booting using Device Trees. In these setups the interrupt type was ignored by the driver when requesting an IRQ. This means that it will fail if a machine specified other type than IRQ_TYPE_NONE. The right approach is to get the IRQ flags that was parsed by OF from the "interrupt" Device Tree propery. Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk> --- drivers/input/touchscreen/atmel_mxt_ts.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)