[PATCH] Input: rotary-encoder: use request_any_context_irq and gpio_get_value_cansleep

[PATCH] Input: rotary-encoder: use request_any_context_irq and gpio_get_value_cansleep

[Timo Teräs]

This allows to use GPIO expanders behind I2C or SPI bus.

Signed-off-by: Timo Teräs <timo.teras@iki.fi>
---
 drivers/input/misc/rotary_encoder.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
index 8aee719..8bedd7b 100644
--- a/drivers/input/misc/rotary_encoder.c
+++ b/drivers/input/misc/rotary_encoder.c
@@ -48,8 +48,8 @@ struct rotary_encoder {
 
 static int rotary_encoder_get_state(const struct rotary_encoder_platform_data *pdata)
 {
-	int a = !!gpio_get_value(pdata->gpio_a);
-	int b = !!gpio_get_value(pdata->gpio_b);
+	int a = !!gpio_get_value_cansleep(pdata->gpio_a);
+	int b = !!gpio_get_value_cansleep(pdata->gpio_b);
 
 	a ^= pdata->inverted_a;
 	b ^= pdata->inverted_b;
@@ -335,18 +335,18 @@ static int rotary_encoder_probe(struct platform_device *pdev)
 		goto exit_free_gpio_b;
 	}
 
-	err = request_irq(encoder->irq_a, handler,
-			  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			  DRV_NAME, encoder);
-	if (err) {
+	err = request_any_context_irq(encoder->irq_a, handler,
+				      IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				      DRV_NAME, encoder);
+	if (err < 0) {
 		dev_err(dev, "unable to request IRQ %d\n", encoder->irq_a);
 		goto exit_free_gpio_b;
 	}
 
-	err = request_irq(encoder->irq_b, handler,
-			  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
-			  DRV_NAME, encoder);
-	if (err) {
+	err = request_any_context_irq(encoder->irq_b, handler,
+				      IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				      DRV_NAME, encoder);
+	if (err < 0) {
 		dev_err(dev, "unable to request IRQ %d\n", encoder->irq_b);
 		goto exit_free_irq_a;
 	}
-- 
2.6.4


--
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: [PATCH] Input: rotary-encoder: use request_any_context_irq and gpio_get_value_cansleep

[Dmitry Torokhov]

On Wed, Dec 23, 2015 at 08:41:55PM +0200, Timo Teräs wrote:
> This allows to use GPIO expanders behind I2C or SPI bus.
> 
> Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> ---
>  drivers/input/misc/rotary_encoder.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/input/misc/rotary_encoder.c b/drivers/input/misc/rotary_encoder.c
> index 8aee719..8bedd7b 100644
> --- a/drivers/input/misc/rotary_encoder.c
> +++ b/drivers/input/misc/rotary_encoder.c
> @@ -48,8 +48,8 @@ struct rotary_encoder {
>  
>  static int rotary_encoder_get_state(const struct rotary_encoder_platform_data *pdata)
>  {
> -	int a = !!gpio_get_value(pdata->gpio_a);
> -	int b = !!gpio_get_value(pdata->gpio_b);
> +	int a = !!gpio_get_value_cansleep(pdata->gpio_a);
> +	int b = !!gpio_get_value_cansleep(pdata->gpio_b);
>  
>  	a ^= pdata->inverted_a;
>  	b ^= pdata->inverted_b;
> @@ -335,18 +335,18 @@ static int rotary_encoder_probe(struct platform_device *pdev)
>  		goto exit_free_gpio_b;
>  	}
>  
> -	err = request_irq(encoder->irq_a, handler,
> -			  IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> -			  DRV_NAME, encoder);
> -	if (err) {
> +	err = request_any_context_irq(encoder->irq_a, handler,
> +				      IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
> +				      DRV_NAME, encoder);
> +	if (err < 0) {

This is wrong. If you are saying that you can use any context IRQ you
can get hard irq, but in rotary_encoder_get_state() (which is called
from IRQ handler) you are using sleeping gpio accessors.

I guess you need to explicitly request threaded IRQs.

Thanks.

-- 
Dmitry
--
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: [PATCH] Input: rotary-encoder: use request_any_context_irq and gpio_get_value_cansleep

[Timo Teras]

On Thu, 7 Jan 2016 10:08:16 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> On Wed, Dec 23, 2015 at 08:41:55PM +0200, Timo Teräs wrote:
> > This allows to use GPIO expanders behind I2C or SPI bus.
> > 
> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
> > ---
> >  drivers/input/misc/rotary_encoder.c | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/input/misc/rotary_encoder.c
> > b/drivers/input/misc/rotary_encoder.c index 8aee719..8bedd7b 100644
> > --- a/drivers/input/misc/rotary_encoder.c
> > +++ b/drivers/input/misc/rotary_encoder.c
> > @@ -48,8 +48,8 @@ struct rotary_encoder {
> >  
> >  static int rotary_encoder_get_state(const struct
> > rotary_encoder_platform_data *pdata) {
> > -	int a = !!gpio_get_value(pdata->gpio_a);
> > -	int b = !!gpio_get_value(pdata->gpio_b);
> > +	int a = !!gpio_get_value_cansleep(pdata->gpio_a);
> > +	int b = !!gpio_get_value_cansleep(pdata->gpio_b);
> >  
> >  	a ^= pdata->inverted_a;
> >  	b ^= pdata->inverted_b;
> > @@ -335,18 +335,18 @@ static int rotary_encoder_probe(struct
> > platform_device *pdev) goto exit_free_gpio_b;
> >  	}
> >  
> > -	err = request_irq(encoder->irq_a, handler,
> > -			  IRQF_TRIGGER_RISING |
> > IRQF_TRIGGER_FALLING,
> > -			  DRV_NAME, encoder);
> > -	if (err) {
> > +	err = request_any_context_irq(encoder->irq_a, handler,
> > +				      IRQF_TRIGGER_RISING |
> > IRQF_TRIGGER_FALLING,
> > +				      DRV_NAME, encoder);
> > +	if (err < 0) {  
> 
> This is wrong. If you are saying that you can use any context IRQ you
> can get hard irq, but in rotary_encoder_get_state() (which is called
> from IRQ handler) you are using sleeping gpio accessors.
> 
> I guess you need to explicitly request threaded IRQs.

This was based on commit 94a8cab8caaa56824981c17b6898b73627e8382f which
did the exact same change for gpio_keys.c.

I think this was based on the assumption that gpio_get_value_cansleep()
never sleeps when it's hard irq based. And I thought this is why
gpio_get_value_cansleep() can really sleep only when the irq is running
in the threaded context in first place.

And that's what I followed.

Then again looking again, seems that all similar uses have been changed
to use workqueue or timer - but primarily due for debouncing - not
that the above code would have been incorrect (at least according to
commitlog). 

But on second thought, I wonder if there's any races due to having two
gpios affect the rotary encoder state. What if the IRQs run on
different CPUs on SMP box.

Would it make sense to convert the actual state reading code to be work
item, and just schedule the work in an any context irq?

Thanks,
Timo
--
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: [PATCH] Input: rotary-encoder: use request_any_context_irq and gpio_get_value_cansleep

[Dmitry Torokhov]

On Thu, Jan 7, 2016 at 12:25 PM, Timo Teras <timo.teras@iki.fi> wrote:
> On Thu, 7 Jan 2016 10:08:16 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>
>> On Wed, Dec 23, 2015 at 08:41:55PM +0200, Timo Teräs wrote:
>> > This allows to use GPIO expanders behind I2C or SPI bus.
>> >
>> > Signed-off-by: Timo Teräs <timo.teras@iki.fi>
>> > ---
>> >  drivers/input/misc/rotary_encoder.c | 20 ++++++++++----------
>> >  1 file changed, 10 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/drivers/input/misc/rotary_encoder.c
>> > b/drivers/input/misc/rotary_encoder.c index 8aee719..8bedd7b 100644
>> > --- a/drivers/input/misc/rotary_encoder.c
>> > +++ b/drivers/input/misc/rotary_encoder.c
>> > @@ -48,8 +48,8 @@ struct rotary_encoder {
>> >
>> >  static int rotary_encoder_get_state(const struct
>> > rotary_encoder_platform_data *pdata) {
>> > -   int a = !!gpio_get_value(pdata->gpio_a);
>> > -   int b = !!gpio_get_value(pdata->gpio_b);
>> > +   int a = !!gpio_get_value_cansleep(pdata->gpio_a);
>> > +   int b = !!gpio_get_value_cansleep(pdata->gpio_b);
>> >
>> >     a ^= pdata->inverted_a;
>> >     b ^= pdata->inverted_b;
>> > @@ -335,18 +335,18 @@ static int rotary_encoder_probe(struct
>> > platform_device *pdev) goto exit_free_gpio_b;
>> >     }
>> >
>> > -   err = request_irq(encoder->irq_a, handler,
>> > -                     IRQF_TRIGGER_RISING |
>> > IRQF_TRIGGER_FALLING,
>> > -                     DRV_NAME, encoder);
>> > -   if (err) {
>> > +   err = request_any_context_irq(encoder->irq_a, handler,
>> > +                                 IRQF_TRIGGER_RISING |
>> > IRQF_TRIGGER_FALLING,
>> > +                                 DRV_NAME, encoder);
>> > +   if (err < 0) {
>>
>> This is wrong. If you are saying that you can use any context IRQ you
>> can get hard irq, but in rotary_encoder_get_state() (which is called
>> from IRQ handler) you are using sleeping gpio accessors.
>>
>> I guess you need to explicitly request threaded IRQs.
>
> This was based on commit 94a8cab8caaa56824981c17b6898b73627e8382f which
> did the exact same change for gpio_keys.c.
>
> I think this was based on the assumption that gpio_get_value_cansleep()
> never sleeps when it's hard irq based. And I thought this is why
> gpio_get_value_cansleep() can really sleep only when the irq is running
> in the threaded context in first place.
>
> And that's what I followed.
>
> Then again looking again, seems that all similar uses have been changed
> to use workqueue or timer - but primarily due for debouncing - not
> that the above code would have been incorrect (at least according to
> commitlog).

Yes, commit log did not address why the call was changed to
cansleep(), but because it is called from workqueue context it is OK
to be used.

>
> But on second thought, I wonder if there's any races due to having two
> gpios affect the rotary encoder state. What if the IRQs run on
> different CPUs on SMP box.

Yes, you are right, there is a race.

>
> Would it make sense to convert the actual state reading code to be work
> item, and just schedule the work in an any context irq?

Or make them both threaded IRQs and protect form concurrent execution
with a mutex.

Thanks.

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