Dear Mr. Dmitry,
Greetings!
The patch removes unused many APIs call chain for every suspend/resume of the device
if no key press event triggered.
There is a call back function gpio_keys_resume() called for
every suspend/resume of the device. and whenever this function called, it is
reading the status of the key. And gpio_keys_resume() API further calls the
below chain of API irrespective of key press event
APIs call chain:
static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
gpiod_get_value_cansleep(bdata->gpiod);
input_event(input, type, *bdata->code, state);
input_sync(input);
The patch avoid the above APIs call chain if there is no key press event triggered.
It will save the device computational resources, power resources and optimize the suspend/resume time
Signed-off-by: Abhishek Kumar Singh <[email protected]>
---
linux-6.4.11/drivers/input/keyboard/gpio_keys.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/linux-6.4.11/drivers/input/keyboard/gpio_keys.c b/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
index 13a8ba5..cd1609e 100644
--- a/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
+++ b/linux-6.4.11/drivers/input/keyboard/gpio_keys.c
@@ -687,8 +687,12 @@ static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
for (i = 0; i < ddata->pdata->nbuttons; i++) {
struct gpio_button_data *bdata = &ddata->data[i];
- if (bdata->gpiod)
- gpio_keys_gpio_report_event(bdata);
+ if (bdata->gpiod) {
+ struct gpio_keys_button *button = bdata->button;
+ if(button->value) {
+ gpio_keys_gpio_report_event(bdata);
+ }
+ }
}
input_sync(input);
}
---
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India
Hi Abhishek,
On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
>
>
> The patch removes unused many APIs call chain for every suspend/resume of the device?
> if no key press event triggered.
>
>
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
>
>
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
>
>
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time
Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.
I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.
Thanks.
--
Dmitry
Dear Dmitry,
Greetings!!!
Thank you so much for your response.
I checked in detailed again and observed the below points, please help to review
and approve it.
There is ISR "gpio_keys_gpio_isr" which is called when the key state is 1 i.e.
key pressed.
Therefore modified code will not impact on the existing driver code.
//For key pressed event:
<3>[ 549.180072] I[0: swapper/0: 0] gpio_keys_gpio_isr
<3>[ 549.196126] [1: kworker/1:1: 78] gpio_keys_gpio_work_func
<3>[ 549.196198] [1: kworker/1:1: 78] gpio-keys soc:gpio_keys: gpio_keys_gpio_report_event key = 115, value = 1
Performance:
I have calculated the differece between entry & exit time
with modified and without modified code and observed that
0.3ms extra computation time in current scenario in each entry/exit time.
Because below APIs will not be called in every resume functions:
1. static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
2. static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
3. gpiod_get_value_cansleep(bdata->gpiod);
4. input_event(input, type, *bdata->code, state);
5. input_sync(input)
So we can save 0.3ms computation time, resume operations will faster and save battery as well.
With changes:
Line 311960: 07-18 16:50:09.359 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:20:37.573207725 UTC
Line 312626: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:20.503579404 UTC
Line 312627: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:20.503656644 UTC
Line 313301: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:33.865626325 UTC
Line 313302: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:33.865724502 UTC
Line 313572: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:37.678468979 UTC
Line 313573: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:37.678566167 UTC
Line 314209: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:23:05.925340634 UTC
Line 314210: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:23:05.925439384 UTC
Without changes:
Line 372095: 07-18 16:10:24.250 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:43:38.592548979 UTC
Line 372344: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:11.589164226 UTC
Line 372346: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:11.589514955 UTC
Line 372573: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:22.606227138 UTC
Line 372575: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:22.606490107 UTC
Line 372944: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:29.024121927 UTC
Line 372946: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:29.024528958 UTC
Line 373181: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:30.904866770 UTC
Line 373183: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:30.905126353 UTC
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India
--------- Original Message ---------
Sender : [email protected] <[email protected]>
Date : 2023-10-29 07:42 (GMT+5:30)
Title : Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
To : Abhishek Kumar Singh<[email protected]>
CC : [email protected]<[email protected]>, [email protected]<[email protected]>, [email protected]<[email protected]>, SRI-N IT Security<[email protected]>
Hi Abhishek,
On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
>
>
> The patch removes unused many APIs call chain for every suspend/resume of the device
> if no key press event triggered.
>
>
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
>
>
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
>
>
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time
Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.
I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.
Thanks.
--
Dmitry
Thanks and Regards,
Dear Dmitry,
Greetings!!
Could you please help to review and update on this?
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer,
Samsung Electronics, Noida-India
--------- Original Message ---------
Sender : Abhishek Kumar Singh <[email protected]>System Power Part /SRI-Noida/Samsung Electronics
Date : 2023-11-07 13:59 (GMT+5:30)
Title : Re[2]: [PATCH] input: gpio-keys - optimize wakeup sequence.
Dear Dmitry,
Greetings!!!
Thank you so much for your response.
I checked in detailed again and observed the below points, please help to review
and approve it.
There is ISR "gpio_keys_gpio_isr" which is called when the key state is 1 i.e.
key pressed.
Therefore modified code will not impact on the existing driver code.
//For key pressed event:
<3>[ 549.180072] I[0: swapper/0: 0] gpio_keys_gpio_isr
<3>[ 549.196126] [1: kworker/1:1: 78] gpio_keys_gpio_work_func
<3>[ 549.196198] [1: kworker/1:1: 78] gpio-keys soc:gpio_keys: gpio_keys_gpio_report_event key = 115, value = 1
Performance:
I have calculated the differece between entry & exit time
with modified and without modified code and observed that
0.3ms extra computation time in current scenario in each entry/exit time.
Because below APIs will not be called in every resume functions:
1. static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
2. static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
3. gpiod_get_value_cansleep(bdata->gpiod);
4. input_event(input, type, *bdata->code, state);
5. input_sync(input)
So we can save 0.3ms computation time, resume operations will faster and save battery as well.
With changes:
Line 311960: 07-18 16:50:09.359 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:20:37.573207725 UTC
Line 312626: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:20.503579404 UTC
Line 312627: 07-18 16:50:42.123 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:20.503656644 UTC
Line 313301: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:33.865626325 UTC
Line 313302: 07-18 16:52:24.182 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:33.865724502 UTC
Line 313572: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:22:37.678468979 UTC
Line 313573: 07-18 16:52:35.111 root 0 0 I [1: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:22:37.678566167 UTC
Line 314209: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state enrty 2023-07-18 11:23:05.925340634 UTC
Line 314210: 07-18 16:52:43.598 root 0 0 I [0: Binder:699_1:20972] PM: gpio_keys_report_state exit 2023-07-18 11:23:05.925439384 UTC
Without changes:
Line 372095: 07-18 16:10:24.250 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:43:38.592548979 UTC
Line 372344: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:11.589164226 UTC
Line 372346: 07-18 16:13:45.439 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:11.589514955 UTC
Line 372573: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:22.606227138 UTC
Line 372575: 07-18 16:14:13.414 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:22.606490107 UTC
Line 372944: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:29.024121927 UTC
Line 372946: 07-18 16:14:26.732 root 0 0 I [1: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:29.024528958 UTC
Line 373181: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state enrty 2023-07-18 10:44:30.904866770 UTC
Line 373183: 07-18 16:14:30.790 root 0 0 I [0: Binder:702_2:18137] PM: gpio_keys_report_state exit 2023-07-18 10:44:30.905126353 UTC
Thanks and Regards,
Abhishek Kumar Singh
Sr. Chief Engineer, Samsung Electronics, Noida-India
--------- Original Message ---------
Sender : [email protected] <[email protected]>
Date : 2023-10-29 07:42 (GMT+5:30)
Title : Re: [PATCH] input: gpio-keys - optimize wakeup sequence.
To : Abhishek Kumar Singh<[email protected]>
CC : [email protected]<[email protected]>, [email protected]<[email protected]>, [email protected]<[email protected]>, SRI-N IT Security<[email protected]>
Hi Abhishek,
On Thu, Oct 26, 2023 at 11:23:20AM +0530, Abhishek Kumar Singh wrote:
> Dear Mr. Dmitry,
> Greetings!
>
>
> The patch removes unused many APIs call chain for every suspend/resume of the device
> if no key press event triggered.
>
>
> There is a call back function gpio_keys_resume() called for
> every suspend/resume of the device. and whenever this function called, it is
> reading the status of the key. And gpio_keys_resume() API further calls the
> below chain of API irrespective of key press event
>
>
> APIs call chain:
> static void gpio_keys_report_state(struct gpio_keys_drvdata *ddata)
> static void gpio_keys_gpio_report_event(struct gpio_button_data *bdata)
> gpiod_get_value_cansleep(bdata->gpiod);
> input_event(input, type, *bdata->code, state);
> input_sync(input);
>
>
> The patch avoid the above APIs call chain if there is no key press event triggered.
> It will save the device computational resources, power resources and optimize the suspend/resume time
Unfortunately it also breaks the driver as button->value does not hold
the current state of the GPIO but rather set one via device tree so that
the driver can use that value when sending EV_ABS events. So with
typical GPIO-backed keys or buttons you change results in no events
reported on resume.
I also wonder what kind of measurements you did on improvements to
suspend/resume time with your change.
Thanks.
--
Dmitry
Thanks and Regards,
Thanks and Regards,