[RFC] QT1070: change the trigger mode of QT1070

[RFC] QT1070: change the trigger mode of QT1070

[Bo Shen]

The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.

Add a workaround for some SOC which can not distinguish the falling
and rising change on I/O lines.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 drivers/input/keyboard/qt1070.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/qt1070.c b/drivers/input/keyboard/qt1070.c
index 0b7b2f8..1855e3d 100644
--- a/drivers/input/keyboard/qt1070.c
+++ b/drivers/input/keyboard/qt1070.c
@@ -201,10 +201,17 @@ 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;
+		/* This is a workaround for some SOC which can not distinguish
+		 * falling and rising change on I/O lines.
+		 */
+		err = request_threaded_irq(client->irq, NULL, qt1070_interrupt,
+			IRQF_TRIGGER_NONE, client->dev.driver->name, data);
+		if (err) {
+			dev_err(&client->dev, "fail to request irq\n");
+			goto err_free_mem;
+		}
 	}
 
 	/* Register the input device */
-- 
1.7.10

Re: [RFC] QT1070: change the trigger mode of QT1070

[Dmitry Torokhov]

Hi Bo,

On Mon, May 07, 2012 at 10:27:33AM +0800, Bo Shen wrote:
> The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
> Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.

Why don't you set up IRQ the way you want in board code instead of
implementing workarounds in the driver?

Thanks.

-- 
Dmitry

Re: [RFC] QT1070: change the trigger mode of QT1070

[javier Martin]

Hi,
the patch works fine for me in i.MX27.

On 7 May 2012 09:04, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> Hi Bo,
>
> On Mon, May 07, 2012 at 10:27:33AM +0800, Bo Shen wrote:
>> The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
>> Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.
>
> Why don't you set up IRQ the way you want in board code instead of
> implementing workarounds in the driver?
>
> Thanks.
>
> --
> Dmitry



-- 
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

RE: [RFC] QT1070: change the trigger mode of QT1070

[Shen, Voice]

Hi Dmitry,
> On Mon, May 07, 2012 at 10:27:33AM +0800, Bo Shen wrote:
> > The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
> > Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.
> 
> Why don't you set up IRQ the way you want in board code instead of
> implementing workarounds in the driver?
Do you mean to use the platform data to pass the irq flag? If yes, when use DT, may be need other modification. If not, any suggestions for how to deal with it in board code?

Best Regards

Re: [RFC] QT1070: change the trigger mode of QT1070

[Josh Wu]

Hello, Dmitry

On 5/7/2012 3:04 PM, Dmitry Torokhov wrote:
> Hi Bo,
>
> On Mon, May 07, 2012 at 10:27:33AM +0800, Bo Shen wrote:
>> The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
>> Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.
> Why don't you set up IRQ the way you want in board code instead of
> implementing workarounds in the driver?

The QT1070 will generate a falling edge interrupt if any valid data 
coming. The workaround is only for the boarding that can handle edge 
interrupt but cannot distinguish rising and falling.
So I think put this trigger set up code to board code will make thing 
more complex.

>
> Thanks.
>

Best Regards,
Josh Wu

Re: [RFC] QT1070: change the trigger mode of QT1070

[javier Martin]

Hi all,
let's take a chip which presents a similar issue like the pca953x [1].
This chip has an IRQ line that is meant to be connected to a SoC input
like the qt1070.

The flags that are used for pca953x are: (IRQF_TRIGGER_LOW | IRQF_ONESHOT)

Can't we do the same for qt1070?
And even more important, can you give me an example of an architecture
in mainline which cannot support these irq flags and still be able to
detect a change from high to low in a GPIO as qt1070 requires?

Moreover, if you try a grep for IRQ_TRIGGER_NONE in the kernel tree
you won't find any driver doing this strange thing.

Do you agree at least that we must change IRQ_TRIGGER_NONE flag into a
more sensible choice?

Regards.

[1] http://lxr.linux.no/#linux+v3.3.4/drivers/gpio/gpio-pca953x.c#L495

-- 
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

Re: [RFC] QT1070: change the trigger mode of QT1070

[Josh Wu]

Hi, Javier

On 5/11/2012 5:13 PM, javier Martin wrote:

> Hi all,
> let's take a chip which presents a similar issue like the pca953x [1].
> This chip has an IRQ line that is meant to be connected to a SoC input
> like the qt1070.
>
> The flags that are used for pca953x are: (IRQF_TRIGGER_LOW | IRQF_ONESHOT)
>
> Can't we do the same for qt1070?

I did a simple test in my board in 2.6.39,  This flag doesn't work. But 
IRQF_TRIGGER_FALLING works fine.
So the question is what is different between this flag with 
IRQF_TRIGGER_FALLING?

> And even more important, can you give me an example of an architecture
> in mainline which cannot support these irq flags and still be able to
> detect a change from high to low in a GPIO as qt1070 requires?

For now, AT91SAM9M10 can only detect GPIO input change, it cannot tell 
is falling or rising. So in theory it cannot support falling flags.
The strange thing is, currently we use IRQF_TRIGGER_NONE in 2.6.39 
kernel for both AT91SAM9M10 and 9X5 chips, and they all work fine. So I 
need check the code of GPIO part.

>
> Moreover, if you try a grep for IRQ_TRIGGER_NONE in the kernel tree
> you won't find any driver doing this strange thing.
>
> Do you agree at least that we must change IRQ_TRIGGER_NONE flag into a
> more sensible choice?

Yes. I agreed.

>
> Regards.
>
> [1] http://lxr.linux.no/#linux+v3.3.4/drivers/gpio/gpio-pca953x.c#L495
>

Best Regards,
Josh Wu

Re: [RFC] QT1070: change the trigger mode of QT1070

[javier Martin]

Hi,

On 11 May 2012 13:33, Josh Wu <josh.wu@atmel.com> wrote:
> Hi, Javier
>
>
> On 5/11/2012 5:13 PM, javier Martin wrote:
>
>> Hi all,
>> let's take a chip which presents a similar issue like the pca953x [1].
>> This chip has an IRQ line that is meant to be connected to a SoC input
>> like the qt1070.
>>
>> The flags that are used for pca953x are: (IRQF_TRIGGER_LOW | IRQF_ONESHOT)
>>
>> Can't we do the same for qt1070?
>
>
> I did a simple test in my board in 2.6.39,  This flag doesn't work. But
> IRQF_TRIGGER_FALLING works fine.
> So the question is what is different between this flag with
> IRQF_TRIGGER_FALLING?

I don't know but IRQF_TRIGGER_FALLING works for me too.

>> And even more important, can you give me an example of an architecture
>> in mainline which cannot support these irq flags and still be able to
>> detect a change from high to low in a GPIO as qt1070 requires?
>
>
> For now, AT91SAM9M10 can only detect GPIO input change, it cannot tell is
> falling or rising. So in theory it cannot support falling flags.
> The strange thing is, currently we use IRQF_TRIGGER_NONE in 2.6.39 kernel
> for both AT91SAM9M10 and 9X5 chips, and they all work fine. So I need check
> the code of GPIO part.
>
>
>>
>> Moreover, if you try a grep for IRQ_TRIGGER_NONE in the kernel tree
>> you won't find any driver doing this strange thing.
>>
>> Do you agree at least that we must change IRQ_TRIGGER_NONE flag into a
>> more sensible choice?
>
>
> Yes. I agreed.
>
>>
>> Regards.
>>
>> [1] http://lxr.linux.no/#linux+v3.3.4/drivers/gpio/gpio-pca953x.c#L495




-- 
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

Re: [RFC] QT1070: change the trigger mode of QT1070

[Bo Shen]

Hi Javier,

On 5/11/2012 20:47, javier Martin wrote:
> Hi,
>
> On 11 May 2012 13:33, Josh Wu<josh.wu@atmel.com>  wrote:
>> Hi, Javier
>>
>>
>> On 5/11/2012 5:13 PM, javier Martin wrote:
>>
>>> Hi all,
>>> let's take a chip which presents a similar issue like the pca953x [1].
>>> This chip has an IRQ line that is meant to be connected to a SoC input
>>> like the qt1070.
>>>
>>> The flags that are used for pca953x are: (IRQF_TRIGGER_LOW | IRQF_ONESHOT)
>>>
>>> Can't we do the same for qt1070?
>>
>> I did a simple test in my board in 2.6.39,  This flag doesn't work. But
>> IRQF_TRIGGER_FALLING works fine.
>> So the question is what is different between this flag with
>> IRQF_TRIGGER_FALLING?
> I don't know but IRQF_TRIGGER_FALLING works for me too.

There is no need to change the driver of QT1070. Please try the 
suggestion of Dmitry. Using the irq_set_irq_type() to set the trigger 
mode of the GPIO.

>
>>> And even more important, can you give me an example of an architecture
>>> in mainline which cannot support these irq flags and still be able to
>>> detect a change from high to low in a GPIO as qt1070 requires?
>>
>> For now, AT91SAM9M10 can only detect GPIO input change, it cannot tell is
>> falling or rising. So in theory it cannot support falling flags.
>> The strange thing is, currently we use IRQF_TRIGGER_NONE in 2.6.39 kernel
>> for both AT91SAM9M10 and 9X5 chips, and they all work fine. So I need check
>> the code of GPIO part.
>>
>>
>>> Moreover, if you try a grep for IRQ_TRIGGER_NONE in the kernel tree
>>> you won't find any driver doing this strange thing.
>>>
>>> Do you agree at least that we must change IRQ_TRIGGER_NONE flag into a
>>> more sensible choice?
>>
>> Yes. I agreed.
>>
>>> Regards.
>>>
>>> [1] http://lxr.linux.no/#linux+v3.3.4/drivers/gpio/gpio-pca953x.c#L495
>
>
>

Re: [RFC] QT1070: change the trigger mode of QT1070

[javier Martin]

Yes,
that works for me too.

All right then. Thank you for your suggestion Dmitry.

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

Re: [RFC] QT1070: change the trigger mode of QT1070

[javier Martin]

On 14 May 2012 08:59, javier Martin <javier.martin@vista-silicon.com> wrote:
> Yes,
> that works for me too.
>
> All right then. Thank you for your suggestion Dmitry.
>
> Regards

Oh, and I almost forgot.
This solution works properly but it doesn't seem too device friendly
don't you think?

-- 
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

Re: [RFC] QT1070: change the trigger mode of QT1070

[Dmitry Torokhov]

Hi Javier,

On Fri, May 11, 2012 at 11:13:41AM +0200, javier Martin wrote:
> 
> Moreover, if you try a grep for IRQ_TRIGGER_NONE in the kernel tree
> you won't find any driver doing this strange thing.

That is because most of the driver simply use '0', which is the same
thing - trust board code to perform setup using irq_set+_irq_type().

Thanks.

-- 
Dmitry

Re: [RFC] QT1070: change the trigger mode of QT1070

[Dmitry Torokhov]

On Fri, May 11, 2012 at 04:28:22PM +0800, Josh Wu wrote:
> Hello, Dmitry
> 
> On 5/7/2012 3:04 PM, Dmitry Torokhov wrote:
> >Hi Bo,
> >
> >On Mon, May 07, 2012 at 10:27:33AM +0800, Bo Shen wrote:
> >>The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
> >>Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.
> >Why don't you set up IRQ the way you want in board code instead of
> >implementing workarounds in the driver?
> 
> The QT1070 will generate a falling edge interrupt if any valid data
> coming. The workaround is only for the boarding that can handle edge
> interrupt but cannot distinguish rising and falling.
> So I think put this trigger set up code to board code will make
> thing more complex.

In the board code it is only a matter of doing irq_set_irq_type(). AT
this time you know exactly how chip is wired and whether it needs level
or edge interupts.

Thanks.

-- 
Dmitry

Re: [RFC] QT1070: change the trigger mode of QT1070

[Bo Shen]

On 5/12/2012 0:08, Dmitry Torokhov wrote:
> On Fri, May 11, 2012 at 04:28:22PM +0800, Josh Wu wrote:
>> Hello, Dmitry
>>
>> On 5/7/2012 3:04 PM, Dmitry Torokhov wrote:
>>> Hi Bo,
>>>
>>> On Mon, May 07, 2012 at 10:27:33AM +0800, Bo Shen wrote:
>>>> The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
>>>> Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.
>>> Why don't you set up IRQ the way you want in board code instead of
>>> implementing workarounds in the driver?
>> The QT1070 will generate a falling edge interrupt if any valid data
>> coming. The workaround is only for the boarding that can handle edge
>> interrupt but cannot distinguish rising and falling.
>> So I think put this trigger set up code to board code will make
>> thing more complex.
> In the board code it is only a matter of doing irq_set_irq_type(). AT
> this time you know exactly how chip is wired and whether it needs level
> or edge interupts.

Hi Dmitry,
   Thanks, this API works fine. So, there is no need to change the 
driver code of QT1070.

> Thanks.
>

Re: [RFC] QT1070: change the trigger mode of QT1070

[Josh Wu]

Hi, Bo

On 5/7/2012 10:27 AM, Bo Shen wrote:

> The default trigger mode of QT1070 is IRQF_TRIGGER_LOW,
> Using TRQF_TRIGGER_FALLING to replace IRQF_TRIGGER_LOW | IRQF_ONESHOT.
>
> Add a workaround for some SOC which can not distinguish the falling
> and rising change on I/O lines.
>
> Signed-off-by: Bo Shen<voice.shen@atmel.com>
> ---
>   drivers/input/keyboard/qt1070.c |   13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/qt1070.c b/drivers/input/keyboard/qt1070.c
> index 0b7b2f8..1855e3d 100644
> --- a/drivers/input/keyboard/qt1070.c
> +++ b/drivers/input/keyboard/qt1070.c
> @@ -201,10 +201,17 @@ 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;
> +		/* This is a workaround for some SOC which can not distinguish
> +		 * falling and rising change on I/O lines.
> +		 */
> +		err = request_threaded_irq(client->irq, NULL, qt1070_interrupt,
> +			IRQF_TRIGGER_NONE, client->dev.driver->name, data);

I think here using IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING is better 
than using IRQF_TRIGGER_NONE.
Since for the QT1070 driver it can handle irq interrupt of falling (can 
read valid data from QT1070) or rising (read but no valid data).
But if set to IRQF_TRIGGER_NONE, for some hardware it maybe caused by 
IRQF_TRIGGER_HIGH or IRQF_TRIGGER_LOW. That is not what you expected.

> +		if (err) {
> +			dev_err(&client->dev, "fail to request irq\n");
> +			goto err_free_mem;
> +		}
>   	}
>
>   	/* Register the input device */

Best Regards,
Josh Wu