On Thu, Jan 7, 2016 at 12:25 PM, Timo Teras <timo.teras@xxxxxx> wrote:
> On Thu, 7 Jan 2016 10:08:16 -0800
> Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx> 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@xxxxxx>
>> > ---
>> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
