Input: gpio_keys_polled - always use gpiod_get_value_cansleep

PATCH In review Dmitry Torokhov 4 participants 6 messages Oct 19, 2016 – Oct 20, 2016

Subsystems (2)

HID Input drivers

lore

Share & tools

lore

Dmitry Torokhov

author maintainer

2016-10-19 23:41 UTC #0 lore raw

PATCH 1/1 Input: gpio_keys_polled - always use gpiod_get_value_cansleep

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 <[email protected]>

---
 drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

drivers/input/keyboard/gpio_keys_polled.c +10 ~~-11~~

index daef8ea..3c79158 100644
@@ -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);

Signature · 5 lines

2.8.0.rc3.226.g39d4020


-- 
Dmitry

Dmitry Torokhov

author maintainer

2016-10-19 23:43 UTC #1 lore raw

Quoted 26 lines · Dmitry Torokhov

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 <[email protected]>
---
 drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/input/keyboard/gpio_keys_polled.c +0 -1
index daef8ea..3c79158 100644
@@ -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;

drivers/input/keyboard/gpio_keys_polled.c static void gpio_keys_polled_check_state(struct input_polled_dev *dev, · line 80

Quoted 10 lines

drivers/input/keyboard/gpio_keys_polled.c +3 -6

 
-	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

Quoted 28 lines

drivers/input/keyboard/gpio_keys_polled.c +7 -4

+			"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);

Signature · 5 lines

2.8.0.rc3.226.g39d4020


-- 
Dmitry

Signature · 1 line

Dmitry

Hans de Goede

Reviewed-by

2016-10-20 09:00 UTC #2 lore raw

Hi,

Quoted 28 lines · Dmitry Torokhov

Quoted 27 lines · Dmitry Torokhov
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 <[email protected]>
---
 drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/input/keyboard/gpio_keys_polled.c +0 -1
index daef8ea..3c79158 100644
@@ -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;
 

Quoted 11 lines

drivers/input/keyboard/gpio_keys_polled.c static void gpio_keys_polled_check_state(struct input_polled_dev *dev, · line 80
Quoted 9 lines
drivers/input/keyboard/gpio_keys_polled.c +3 -6
-	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 <[email protected]>

Regards,

Hans

Quoted 30 lines

Quoted 29 lines

drivers/input/keyboard/gpio_keys_polled.c +7 -5

+			"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

Mika Westerberg

2016-10-20 10:47 UTC #3 lore raw

In reply to Dmitry Torokhov

Quoted 25 lines · Dmitry Torokhov

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 <[email protected]>
---
 drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/input/keyboard/gpio_keys_polled.c +0 -1
index daef8ea..3c79158 100644
@@ -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,
 {

drivers/input/keyboard/gpio_keys_polled.c static void gpio_keys_polled_check_state(struct input_polled_dev *dev, · line 79

Quoted 10 lines

drivers/input/keyboard/gpio_keys_polled.c +2 -6

 	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).

Quoted 29 lines

drivers/input/keyboard/gpio_keys_polled.c +8 -4

+		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);

Signature · 5 lines

2.8.0.rc3.226.g39d4020


-- 
Dmitry

Dmitry Torokhov

author maintainer

2016-10-20 21:53 UTC #4 lore raw

Quoted 28 lines · Mika Westerberg

Quoted 27 lines · Dmitry Torokhov
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 <[email protected]>
---
 drivers/input/keyboard/gpio_keys_polled.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)
drivers/input/keyboard/gpio_keys_polled.c +0 -1
index daef8ea..3c79158 100644
@@ -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;
 

Quoted 10 lines

drivers/input/keyboard/gpio_keys_polled.c static void gpio_keys_polled_check_state(struct input_polled_dev *dev, · line 79
Quoted 8 lines
drivers/input/keyboard/gpio_keys_polled.c +2 -6
-	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.

Quoted 33 lines

Otherwise this patch looks good to me (sans the below line which you mentioned in your followup email).

Quoted 29 lines

drivers/input/keyboard/gpio_keys_polled.c +8 -4

+		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);

Signature · 5 lines

2.8.0.rc3.226.g39d4020


-- 
Dmitry

Signature · 1 line

Dmitry

Linus Walleij

Acked-by on v1 1/1

2016-10-20 12:23 UTC #5 lore raw

In reply to Dmitry Torokhov

Quoted 7 lines · Dmitry Torokhov

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 <[email protected]>

Apart from the other comments mentioned:

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij