| Message ID | 1450896115-12422-1-git-send-email-timo.teras@iki.fi (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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.
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
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.
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; }
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(-)