* [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep
@ 2016-10-19 23:41 Dmitry Torokhov
2016-10-19 23:43 ` Dmitry Torokhov
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2016-10-19 23:41 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Hans de Goede, Geert Uytterhoeven, Aaron Lu,
Mika Westerberg, Linus Walleij
It does not matter if given GPIO may sleep or not when reading state,
polling is always done in a non-atomic context, so we should always
be able to simply use gpiod_get_value_cansleep().
Also let's note in the logs when we fail to read gpio state.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
index daef8ea..3c79158 100644
--- a/drivers/input/keyboard/gpio_keys_polled.c
+++ b/drivers/input/keyboard/gpio_keys_polled.c
@@ -34,7 +34,6 @@ struct gpio_keys_button_data {
int last_state;
int count;
int threshold;
- int can_sleep;
};
struct gpio_keys_polled_dev {
@@ -76,16 +75,17 @@ static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
{
int state;
- if (bdata->can_sleep)
- state = !!gpiod_get_value_cansleep(bdata->gpiod);
- else
- state = !!gpiod_get_value(bdata->gpiod);
-
- gpio_keys_button_event(dev, button, state);
+ state = gpiod_get_value_cansleep(bdata->gpiod);
+ if (unlikely(state < 0)) {
+ dev_err(input->dev.parent,
+ "failed to get gpio state: %d\n", state);
+ } else {
+ gpio_keys_button_event(dev, button, state);
- if (state != bdata->last_state) {
- bdata->count = 0;
- bdata->last_state = state;
+ if (state != bdata->last_state) {
+ bdata->count = 0;
+ bdata->last_state = state;
+ }
}
}
@@ -341,7 +341,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
}
}
- bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
bdata->last_state = -1;
bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
pdata->poll_interval);
--
2.8.0.rc3.226.g39d4020
--
Dmitry
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep
2016-10-19 23:41 [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep Dmitry Torokhov
@ 2016-10-19 23:43 ` Dmitry Torokhov
2016-10-20 9:00 ` Hans de Goede
2016-10-20 10:45 ` Mika Westerberg
2016-10-20 12:23 ` Linus Walleij
2 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2016-10-19 23:43 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Hans de Goede, Geert Uytterhoeven, Aaron Lu,
Mika Westerberg, Linus Walleij
On Wed, Oct 19, 2016 at 04:41:07PM -0700, Dmitry Torokhov wrote:
> It does not matter if given GPIO may sleep or not when reading state,
> polling is always done in a non-atomic context, so we should always
> be able to simply use gpiod_get_value_cansleep().
>
> Also let's note in the logs when we fail to read gpio state.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> index daef8ea..3c79158 100644
> --- a/drivers/input/keyboard/gpio_keys_polled.c
> +++ b/drivers/input/keyboard/gpio_keys_polled.c
> @@ -34,7 +34,6 @@ struct gpio_keys_button_data {
> int last_state;
> int count;
> int threshold;
> - int can_sleep;
> };
>
> struct gpio_keys_polled_dev {
> @@ -76,16 +75,17 @@ static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
> {
> int state;
>
> - if (bdata->can_sleep)
> - state = !!gpiod_get_value_cansleep(bdata->gpiod);
> - else
> - state = !!gpiod_get_value(bdata->gpiod);
> -
> - gpio_keys_button_event(dev, button, state);
> + state = gpiod_get_value_cansleep(bdata->gpiod);
> + if (unlikely(state < 0)) {
> + dev_err(input->dev.parent,
Umm, this should have been dev->input->dev.parent
> + "failed to get gpio state: %d\n", state);
> + } else {
> + gpio_keys_button_event(dev, button, state);
>
> - if (state != bdata->last_state) {
> - bdata->count = 0;
> - bdata->last_state = state;
> + if (state != bdata->last_state) {
> + bdata->count = 0;
> + bdata->last_state = state;
> + }
> }
> }
>
> @@ -341,7 +341,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
> }
> }
>
> - bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
> bdata->last_state = -1;
> bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
> pdata->poll_interval);
> --
> 2.8.0.rc3.226.g39d4020
>
>
> --
> Dmitry
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep
2016-10-19 23:43 ` Dmitry Torokhov
@ 2016-10-20 9:00 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2016-10-20 9:00 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input
Cc: linux-kernel, Geert Uytterhoeven, Aaron Lu, Mika Westerberg,
Linus Walleij
Hi,
On 20-10-16 01:43, Dmitry Torokhov wrote:
> On Wed, Oct 19, 2016 at 04:41:07PM -0700, Dmitry Torokhov wrote:
>> It does not matter if given GPIO may sleep or not when reading state,
>> polling is always done in a non-atomic context, so we should always
>> be able to simply use gpiod_get_value_cansleep().
>>
>> Also let's note in the logs when we fail to read gpio state.
>>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>> drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
>> 1 file changed, 10 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
>> index daef8ea..3c79158 100644
>> --- a/drivers/input/keyboard/gpio_keys_polled.c
>> +++ b/drivers/input/keyboard/gpio_keys_polled.c
>> @@ -34,7 +34,6 @@ struct gpio_keys_button_data {
>> int last_state;
>> int count;
>> int threshold;
>> - int can_sleep;
>> };
>>
>> struct gpio_keys_polled_dev {
>> @@ -76,16 +75,17 @@ static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
>> {
>> int state;
>>
>> - if (bdata->can_sleep)
>> - state = !!gpiod_get_value_cansleep(bdata->gpiod);
>> - else
>> - state = !!gpiod_get_value(bdata->gpiod);
>> -
>> - gpio_keys_button_event(dev, button, state);
>> + state = gpiod_get_value_cansleep(bdata->gpiod);
>> + if (unlikely(state < 0)) {
>> + dev_err(input->dev.parent,
>
> Umm, this should have been dev->input->dev.parent
With that fixed this patch looks good to me and is:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
>
>> + "failed to get gpio state: %d\n", state);
>> + } else {
>> + gpio_keys_button_event(dev, button, state);
>>
>> - if (state != bdata->last_state) {
>> - bdata->count = 0;
>> - bdata->last_state = state;
>> + if (state != bdata->last_state) {
>> + bdata->count = 0;
>> + bdata->last_state = state;
>> + }
>> }
>> }
>>
>> @@ -341,7 +341,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
>> }
>> }
>>
>> - bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
>> bdata->last_state = -1;
>> bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
>> pdata->poll_interval);
>> --
>> 2.8.0.rc3.226.g39d4020
>>
>>
>> --
>> Dmitry
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep
2016-10-19 23:41 [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep Dmitry Torokhov
2016-10-19 23:43 ` Dmitry Torokhov
@ 2016-10-20 10:45 ` Mika Westerberg
2016-10-20 21:53 ` Dmitry Torokhov
2016-10-20 12:23 ` Linus Walleij
2 siblings, 1 reply; 6+ messages in thread
From: Mika Westerberg @ 2016-10-20 10:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, linux-kernel, Hans de Goede, Geert Uytterhoeven,
Aaron Lu, Linus Walleij
On Wed, Oct 19, 2016 at 04:41:07PM -0700, Dmitry Torokhov wrote:
> It does not matter if given GPIO may sleep or not when reading state,
> polling is always done in a non-atomic context, so we should always
> be able to simply use gpiod_get_value_cansleep().
>
> Also let's note in the logs when we fail to read gpio state.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
> 1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> index daef8ea..3c79158 100644
> --- a/drivers/input/keyboard/gpio_keys_polled.c
> +++ b/drivers/input/keyboard/gpio_keys_polled.c
> @@ -34,7 +34,6 @@ struct gpio_keys_button_data {
> int last_state;
> int count;
> int threshold;
> - int can_sleep;
> };
>
> struct gpio_keys_polled_dev {
> @@ -76,16 +75,17 @@ static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
> {
> int state;
>
> - if (bdata->can_sleep)
> - state = !!gpiod_get_value_cansleep(bdata->gpiod);
> - else
> - state = !!gpiod_get_value(bdata->gpiod);
> -
> - gpio_keys_button_event(dev, button, state);
> + state = gpiod_get_value_cansleep(bdata->gpiod);
> + if (unlikely(state < 0)) {
Is this unlikely() really bringing any performance benefits here?
Otherwise this patch looks good to me (sans the below line which you
mentioned in your followup email).
> + dev_err(input->dev.parent,
> + "failed to get gpio state: %d\n", state);
> + } else {
> + gpio_keys_button_event(dev, button, state);
>
> - if (state != bdata->last_state) {
> - bdata->count = 0;
> - bdata->last_state = state;
> + if (state != bdata->last_state) {
> + bdata->count = 0;
> + bdata->last_state = state;
> + }
> }
> }
>
> @@ -341,7 +341,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
> }
> }
>
> - bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
> bdata->last_state = -1;
> bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
> pdata->poll_interval);
> --
> 2.8.0.rc3.226.g39d4020
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep
2016-10-20 10:45 ` Mika Westerberg
@ 2016-10-20 21:53 ` Dmitry Torokhov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Torokhov @ 2016-10-20 21:53 UTC (permalink / raw)
To: Mika Westerberg
Cc: linux-input, linux-kernel, Hans de Goede, Geert Uytterhoeven,
Aaron Lu, Linus Walleij
On Thu, Oct 20, 2016 at 01:45:19PM +0300, Mika Westerberg wrote:
> On Wed, Oct 19, 2016 at 04:41:07PM -0700, Dmitry Torokhov wrote:
> > It does not matter if given GPIO may sleep or not when reading state,
> > polling is always done in a non-atomic context, so we should always
> > be able to simply use gpiod_get_value_cansleep().
> >
> > Also let's note in the logs when we fail to read gpio state.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
> > 1 file changed, 10 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/input/keyboard/gpio_keys_polled.c b/drivers/input/keyboard/gpio_keys_polled.c
> > index daef8ea..3c79158 100644
> > --- a/drivers/input/keyboard/gpio_keys_polled.c
> > +++ b/drivers/input/keyboard/gpio_keys_polled.c
> > @@ -34,7 +34,6 @@ struct gpio_keys_button_data {
> > int last_state;
> > int count;
> > int threshold;
> > - int can_sleep;
> > };
> >
> > struct gpio_keys_polled_dev {
> > @@ -76,16 +75,17 @@ static void gpio_keys_polled_check_state(struct input_polled_dev *dev,
> > {
> > int state;
> >
> > - if (bdata->can_sleep)
> > - state = !!gpiod_get_value_cansleep(bdata->gpiod);
> > - else
> > - state = !!gpiod_get_value(bdata->gpiod);
> > -
> > - gpio_keys_button_event(dev, button, state);
> > + state = gpiod_get_value_cansleep(bdata->gpiod);
> > + if (unlikely(state < 0)) {
>
> Is this unlikely() really bringing any performance benefits here?
Not really, I dropped it.
>
> Otherwise this patch looks good to me (sans the below line which you
> mentioned in your followup email).
>
> > + dev_err(input->dev.parent,
> > + "failed to get gpio state: %d\n", state);
> > + } else {
> > + gpio_keys_button_event(dev, button, state);
> >
> > - if (state != bdata->last_state) {
> > - bdata->count = 0;
> > - bdata->last_state = state;
> > + if (state != bdata->last_state) {
> > + bdata->count = 0;
> > + bdata->last_state = state;
> > + }
> > }
> > }
> >
> > @@ -341,7 +341,6 @@ static int gpio_keys_polled_probe(struct platform_device *pdev)
> > }
> > }
> >
> > - bdata->can_sleep = gpiod_cansleep(bdata->gpiod);
> > bdata->last_state = -1;
> > bdata->threshold = DIV_ROUND_UP(button->debounce_interval,
> > pdata->poll_interval);
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> >
> > --
> > Dmitry
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep
2016-10-19 23:41 [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep Dmitry Torokhov
2016-10-19 23:43 ` Dmitry Torokhov
2016-10-20 10:45 ` Mika Westerberg
@ 2016-10-20 12:23 ` Linus Walleij
2 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2016-10-20 12:23 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linux Input, linux-kernel@vger.kernel.org, Hans de Goede,
Geert Uytterhoeven, Aaron Lu, Mika Westerberg
On Thu, Oct 20, 2016 at 1:41 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> It does not matter if given GPIO may sleep or not when reading state,
> polling is always done in a non-atomic context, so we should always
> be able to simply use gpiod_get_value_cansleep().
>
> Also let's note in the logs when we fail to read gpio state.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Apart from the other comments mentioned:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-10-20 21:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-19 23:41 [PATCH] Input: gpio_keys_polled - always use gpiod_get_value_cansleep Dmitry Torokhov
2016-10-19 23:43 ` Dmitry Torokhov
2016-10-20 9:00 ` Hans de Goede
2016-10-20 10:45 ` Mika Westerberg
2016-10-20 21:53 ` Dmitry Torokhov
2016-10-20 12:23 ` Linus Walleij
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).