* qt1070: Why IRQF_TRIGGER_NONE? @ 2012-05-02 8:35 javier Martin 2012-05-03 1:41 ` Josh Wu 0 siblings, 1 reply; 7+ messages in thread From: javier Martin @ 2012-05-02 8:35 UTC (permalink / raw) To: linux-input; +Cc: Wolfram Sang, Jean Delvare, Axel Lin, Dmitry Torokhov Hi, I was testing qt1070 driver with my Visstrim SM10 board when I found out IRQs where not triggered properly. I had to come up with the following patch in order to solve it: diff --git a/drivers/input/keyboard/qt1070.c b/drivers/input/keyboard/qt1070.c index 0b7b2f8..e8b6ae3 100644 --- a/drivers/input/keyboard/qt1070.c +++ b/drivers/input/keyboard/qt1070.c @@ -201,7 +201,7 @@ static int __devinit qt1070_probe(struct i2c_client *client, msleep(QT1070_RESET_TIME); err = request_threaded_irq(client->irq, NULL, qt1070_interrupt, - IRQF_TRIGGER_NONE, client->dev.driver->name, data); + IRQF_TRIGGER_FALLING, client->dev.driver->name, data); if (err) { dev_err(&client->dev, "fail to request irq\n"); goto err_free_mem; According to the datasheet (p10) "The CHANGE line is active low and signals when there is a change in state in the Detection or Input status bytes. It is cleared (allowed to float high) when the host reads the status bytes." As I understand, this means that we have to monitor falling edges here. Is there any reason to register the IRQ as IRQF_TRIGGER_NONE? Would you accept the above patch instead, provided I send it in the required format? Regards. -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: qt1070: Why IRQF_TRIGGER_NONE? 2012-05-02 8:35 qt1070: Why IRQF_TRIGGER_NONE? javier Martin @ 2012-05-03 1:41 ` Josh Wu 2012-05-03 5:15 ` Shen, Voice 0 siblings, 1 reply; 7+ messages in thread From: Josh Wu @ 2012-05-03 1:41 UTC (permalink / raw) To: javier Martin, Shen, Voice Cc: linux-input, Wolfram Sang, Jean Delvare, Axel Lin, Dmitry Torokhov On 5/2/2012 4:35 PM, javier Martin wrote: > Hi, > I was testing qt1070 driver with my Visstrim SM10 board when I found > out IRQs where not triggered properly. > > I had to come up with the following patch in order to solve it: > > diff --git a/drivers/input/keyboard/qt1070.c b/drivers/input/keyboard/qt1070.c > index 0b7b2f8..e8b6ae3 100644 > --- a/drivers/input/keyboard/qt1070.c > +++ b/drivers/input/keyboard/qt1070.c > @@ -201,7 +201,7 @@ static int __devinit qt1070_probe(struct i2c_client *client, > msleep(QT1070_RESET_TIME); > > err = request_threaded_irq(client->irq, NULL, qt1070_interrupt, > - IRQF_TRIGGER_NONE, client->dev.driver->name, data); > + IRQF_TRIGGER_FALLING, client->dev.driver->name, data); > if (err) { > dev_err(&client->dev, "fail to request irq\n"); > goto err_free_mem; > > > According to the datasheet (p10) "The CHANGE line is active low and > signals when there is a change in state in the Detection or > Input status bytes. It is cleared (allowed to float high) when the > host reads the status bytes." As I understand, this means that we have > to monitor falling edges here. > > Is there any reason to register the IRQ as IRQF_TRIGGER_NONE? Would > you accept the above patch instead, provided I send it in the required > format? I add Voice Shen to the mail loop, who is writing above code. Voice, will that IRQF_TRIGGER_XXXX changes impact other platforms? Best Regards, Josh Wu > > Regards. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: qt1070: Why IRQF_TRIGGER_NONE? 2012-05-03 1:41 ` Josh Wu @ 2012-05-03 5:15 ` Shen, Voice 2012-05-03 6:14 ` Jean Delvare 0 siblings, 1 reply; 7+ messages in thread From: Shen, Voice @ 2012-05-03 5:15 UTC (permalink / raw) To: Wu, Josh, javier Martin Cc: linux-input@vger.kernel.org, Wolfram Sang, Jean Delvare, Axel Lin, Dmitry Torokhov Hi All, Some information as following, According to the datasheet of qt1070, we can use IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW (I think this is the best) for IRQ flag. However, the IRQ line is a GPIO of a SOC. Some SOC can detect the level change of the GPIO, while can not distinguish the falling or rising. So, the IRQ flag depends on the trigger mode of GPIO line. Maybe use the "flags" element in "struct i2c_board_info" to pass the IRQ flag, or add another element named "irqflags" into "struct i2c_board_info". I think this will be better, but I am not sure whether this is a good solution. BTW, I am reworking the driver for DT (device tree) support. Best Regards, Voice Shen -----Original Message----- From: Josh Wu [mailto:josh.wu@atmel.com] Sent: Thursday, May 03, 2012 9:41 申波 To: javier Martin; Shen, Voice Cc: linux-input@vger.kernel.org; Wolfram Sang; Jean Delvare; Axel Lin; Dmitry Torokhov Subject: Re: qt1070: Why IRQF_TRIGGER_NONE? On 5/2/2012 4:35 PM, javier Martin wrote: > Hi, > I was testing qt1070 driver with my Visstrim SM10 board when I found > out IRQs where not triggered properly. > > I had to come up with the following patch in order to solve it: > > diff --git a/drivers/input/keyboard/qt1070.c b/drivers/input/keyboard/qt1070.c > index 0b7b2f8..e8b6ae3 100644 > --- a/drivers/input/keyboard/qt1070.c > +++ b/drivers/input/keyboard/qt1070.c > @@ -201,7 +201,7 @@ static int __devinit qt1070_probe(struct i2c_client *client, > msleep(QT1070_RESET_TIME); > > err = request_threaded_irq(client->irq, NULL, qt1070_interrupt, > - IRQF_TRIGGER_NONE, client->dev.driver->name, data); > + IRQF_TRIGGER_FALLING, client->dev.driver->name, data); > if (err) { > dev_err(&client->dev, "fail to request irq\n"); > goto err_free_mem; > > > According to the datasheet (p10) "The CHANGE line is active low and > signals when there is a change in state in the Detection or > Input status bytes. It is cleared (allowed to float high) when the > host reads the status bytes." As I understand, this means that we have > to monitor falling edges here. > > Is there any reason to register the IRQ as IRQF_TRIGGER_NONE? Would > you accept the above patch instead, provided I send it in the required > format? I add Voice Shen to the mail loop, who is writing above code. Voice, will that IRQF_TRIGGER_XXXX changes impact other platforms? Best Regards, Josh Wu > > Regards. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: qt1070: Why IRQF_TRIGGER_NONE? 2012-05-03 5:15 ` Shen, Voice @ 2012-05-03 6:14 ` Jean Delvare 2012-05-04 2:07 ` Shen, Voice 0 siblings, 1 reply; 7+ messages in thread From: Jean Delvare @ 2012-05-03 6:14 UTC (permalink / raw) To: Shen, Voice Cc: Wu, Josh, javier Martin, linux-input, Wolfram Sang, Axel Lin, Dmitry Torokhov On Thu, 3 May 2012 05:15:14 +0000, Shen, Voice wrote: > Hi All, > Some information as following, > > According to the datasheet of qt1070, we can use IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW (I think this is the best) for IRQ flag. However, the IRQ line is a GPIO of a SOC. Some SOC can detect the level change of the GPIO, while can not distinguish the falling or rising. So, the IRQ flag depends on the trigger mode of GPIO line. > > Maybe use the "flags" element in "struct i2c_board_info" to pass the IRQ flag, or add another element named "irqflags" into "struct i2c_board_info". I think this will be better, but I am not sure whether this is a good solution. i2c_board_info.flags is for I2C client flags, please do not abuse it for IRQ information. I have no objection to an irq_flag member being added, however I remember past discussions where people argued whether it was the right thing to do or whether the IRQ mode was best set by platform initialization code. Part of that discussion was archived here: http://marc.info/?t=128743170300002&r=1&w=2 Said discussion did not result in any code being merged as I don't think we came to an agreement. Feel free to restart the discussion with the interested people. -- Jean Delvare ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: qt1070: Why IRQF_TRIGGER_NONE? 2012-05-03 6:14 ` Jean Delvare @ 2012-05-04 2:07 ` Shen, Voice 2012-05-04 7:06 ` javier Martin 0 siblings, 1 reply; 7+ messages in thread From: Shen, Voice @ 2012-05-04 2:07 UTC (permalink / raw) To: Jean Delvare, javier Martin Cc: Wu, Josh, linux-input@vger.kernel.org, Wolfram Sang, Axel Lin, Dmitry Torokhov Hi Jean, Thank you for your information. Although the irq_flag can not be added into "struct i2c_board_info" till now, I will try to find other solution. Thanks again. Hi Javier, As to the IRQ flag depends on SOC. We try to find other solution. Please describe you issue in detail. And what's the mode does your SOC support. Best Regards, Voice Shen -----Original Message----- From: Jean Delvare [mailto:khali@linux-fr.org] Sent: Thursday, May 03, 2012 14:15 申波 To: Shen, Voice Cc: Wu, Josh; javier Martin; linux-input@vger.kernel.org; Wolfram Sang; Axel Lin; Dmitry Torokhov Subject: Re: qt1070: Why IRQF_TRIGGER_NONE? On Thu, 3 May 2012 05:15:14 +0000, Shen, Voice wrote: > Hi All, > Some information as following, > > According to the datasheet of qt1070, we can use IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW (I think this is the best) for IRQ flag. However, the IRQ line is a GPIO of a SOC. Some SOC can detect the level change of the GPIO, while can not distinguish the falling or rising. So, the IRQ flag depends on the trigger mode of GPIO line. > > Maybe use the "flags" element in "struct i2c_board_info" to pass the IRQ flag, or add another element named "irqflags" into "struct i2c_board_info". I think this will be better, but I am not sure whether this is a good solution. i2c_board_info.flags is for I2C client flags, please do not abuse it for IRQ information. I have no objection to an irq_flag member being added, however I remember past discussions where people argued whether it was the right thing to do or whether the IRQ mode was best set by platform initialization code. Part of that discussion was archived here: http://marc.info/?t=128743170300002&r=1&w=2 Said discussion did not result in any code being merged as I don't think we came to an agreement. Feel free to restart the discussion with the interested people. -- Jean Delvare ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: qt1070: Why IRQF_TRIGGER_NONE? 2012-05-04 2:07 ` Shen, Voice @ 2012-05-04 7:06 ` javier Martin 2012-05-04 7:56 ` Dmitry Torokhov 0 siblings, 1 reply; 7+ messages in thread From: javier Martin @ 2012-05-04 7:06 UTC (permalink / raw) To: Shen, Voice Cc: Jean Delvare, Wu, Josh, linux-input@vger.kernel.org, Wolfram Sang, Axel Lin, Dmitry Torokhov Hi all, thank you for your comments. On 4 May 2012 04:07, Shen, Voice <Voice.Shen@atmel.com> wrote: > Hi Jean, > Thank you for your information. Although the irq_flag can not be added into "struct i2c_board_info" till now, I will try to find other solution. > > Thanks again. > > Hi Javier, > As to the IRQ flag depends on SOC. We try to find other solution. Please describe you issue in detail. And what's the mode does your SOC support. As I stated, with the current 'IRQF_TRIGGER_NONE' flag, this driver doesn't work in i.MX27. Furthermore, I don't think it works on any platform since as you previously pointed according to the datasheet of qt1070, we can use either IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW. But I don't know what is the sense of using 'IRQF_TRIGGER_NONE' here. My SoC, an i.MX27, theoretically supports both RQF_TRIGGER_FALLING and IRQF_TRIGGER_LOW but I've only tested the former. If we take a look at a similar device, a pca953x they simply use the following flags: (IRQF_TRIGGER_LOW | IRQF_ONESHOT) http://lxr.linux.no/#linux+v3.3.4/drivers/gpio/gpio-pca953x.c#L495 We could do the same here. It's true there can be SoC's which doesn't support IRQF_TRIGGER_LOW flag, but if this is the case it means they don't support this HW feature then. So you are not breaking anything. > Best Regards, > Voice Shen > -----Original Message----- > From: Jean Delvare [mailto:khali@linux-fr.org] > Sent: Thursday, May 03, 2012 14:15 申波 > To: Shen, Voice > Cc: Wu, Josh; javier Martin; linux-input@vger.kernel.org; Wolfram Sang; Axel Lin; Dmitry Torokhov > Subject: Re: qt1070: Why IRQF_TRIGGER_NONE? > > On Thu, 3 May 2012 05:15:14 +0000, Shen, Voice wrote: >> Hi All, >> Some information as following, >> >> According to the datasheet of qt1070, we can use IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW (I think this is the best) for IRQ flag. However, the IRQ line is a GPIO of a SOC. Some SOC can detect the level change of the GPIO, while can not distinguish the falling or rising. So, the IRQ flag depends on the trigger mode of GPIO line. >> >> Maybe use the "flags" element in "struct i2c_board_info" to pass the IRQ flag, or add another element named "irqflags" into "struct i2c_board_info". I think this will be better, but I am not sure whether this is a good solution. > > i2c_board_info.flags is for I2C client flags, please do not abuse it > for IRQ information. > > I have no objection to an irq_flag member being added, however I > remember past discussions where people argued whether it was the right > thing to do or whether the IRQ mode was best set by platform > initialization code. Part of that discussion was archived here: > http://marc.info/?t=128743170300002&r=1&w=2 > Said discussion did not result in any code being merged as I don't > think we came to an agreement. Feel free to restart the discussion with > the interested people. > > -- > Jean Delvare -- Javier Martin Vista Silicon S.L. CDTUC - FASE C - Oficina S-345 Avda de los Castros s/n 39005- Santander. Cantabria. Spain +34 942 25 32 60 www.vista-silicon.com -- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: qt1070: Why IRQF_TRIGGER_NONE? 2012-05-04 7:06 ` javier Martin @ 2012-05-04 7:56 ` Dmitry Torokhov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Torokhov @ 2012-05-04 7:56 UTC (permalink / raw) To: javier Martin Cc: Shen, Voice, Jean Delvare, Wu, Josh, linux-input@vger.kernel.org, Wolfram Sang, Axel Lin Hi Javier, On Fri, May 04, 2012 at 09:06:16AM +0200, javier Martin wrote: > Hi all, > thank you for your comments. > > On 4 May 2012 04:07, Shen, Voice <Voice.Shen@atmel.com> wrote: > > Hi Jean, > > Thank you for your information. Although the irq_flag can not be added into "struct i2c_board_info" till now, I will try to find other solution. > > > > Thanks again. > > > > Hi Javier, > > As to the IRQ flag depends on SOC. We try to find other solution. Please describe you issue in detail. And what's the mode does your SOC support. > > As I stated, with the current 'IRQF_TRIGGER_NONE' flag, this driver > doesn't work in i.MX27. Furthermore, I don't think it works on any > platform since as you previously pointed according to the datasheet of > qt1070, we can use either IRQF_TRIGGER_FALLING or IRQF_TRIGGER_LOW. > But I don't know what is the sense of using 'IRQF_TRIGGER_NONE' here. IRQF_TRIGGER_NONE == 0, i.e. "use whatever method board code set up". -- Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-04 7:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-05-02 8:35 qt1070: Why IRQF_TRIGGER_NONE? javier Martin 2012-05-03 1:41 ` Josh Wu 2012-05-03 5:15 ` Shen, Voice 2012-05-03 6:14 ` Jean Delvare 2012-05-04 2:07 ` Shen, Voice 2012-05-04 7:06 ` javier Martin 2012-05-04 7:56 ` Dmitry Torokhov
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).