| Message ID | 20161019234107.GA2927@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > -- 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 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 -- 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, 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 -- 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, 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
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);
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(-)