* [PATCH] input: gpio-keys - optimize wakeup sequence. [not found] <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p4> @ 2023-10-26 5:53 ` Abhishek Kumar Singh 2023-10-29 2:11 ` dmitry.torokhov 0 siblings, 1 reply; 6+ messages in thread From: Abhishek Kumar Singh @ 2023-10-26 5:53 UTC (permalink / raw) To: dmitry.torokhov@gmail.com Cc: robh@kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, SRI-N IT Security, Abhishek Kumar Singh [-- Attachment #1: Type: text/plain, Size: 2037 bytes --] 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 <abhi1.singh@samsung.com> --- 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 [-- Attachment #2: input_keys_optimized.zip --] [-- Type: application/octet-stream, Size: 998 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] input: gpio-keys - optimize wakeup sequence. 2023-10-26 5:53 ` [PATCH] input: gpio-keys - optimize wakeup sequence Abhishek Kumar Singh @ 2023-10-29 2:11 ` dmitry.torokhov 2023-11-07 8:29 ` Re[2]: " Abhishek Kumar Singh 0 siblings, 1 reply; 6+ messages in thread From: dmitry.torokhov @ 2023-10-29 2:11 UTC (permalink / raw) To: Abhishek Kumar Singh Cc: robh@kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, SRI-N IT Security 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re[2]: [PATCH] input: gpio-keys - optimize wakeup sequence. 2023-10-29 2:11 ` dmitry.torokhov @ 2023-11-07 8:29 ` Abhishek Kumar Singh 2023-11-20 3:59 ` Re[3]: " Abhishek Kumar Singh 0 siblings, 1 reply; 6+ messages in thread From: Abhishek Kumar Singh @ 2023-11-07 8:29 UTC (permalink / raw) To: dmitry.torokhov@gmail.com Cc: robh@kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, SRI-N IT Security 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 : dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> Date : 2023-10-29 07:42 (GMT+5:30) Title : Re: [PATCH] input: gpio-keys - optimize wakeup sequence. To : Abhishek Kumar Singh<abhi1.singh@samsung.com> CC : robh@kernel.org<robh@kernel.org>, linux-input@vger.kernel.org<linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org<linux-kernel@vger.kernel.org>, SRI-N IT Security<sri-n.itsec@samsung.com> 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, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re[3]: [PATCH] input: gpio-keys - optimize wakeup sequence. 2023-11-07 8:29 ` Re[2]: " Abhishek Kumar Singh @ 2023-11-20 3:59 ` Abhishek Kumar Singh 0 siblings, 0 replies; 6+ messages in thread From: Abhishek Kumar Singh @ 2023-11-20 3:59 UTC (permalink / raw) To: Abhishek Kumar Singh, dmitry.torokhov@gmail.com Cc: robh@kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, SRI-N IT Security 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 <abhi1.singh@samsung.com>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 : dmitry.torokhov@gmail.com <dmitry.torokhov@gmail.com> Date : 2023-10-29 07:42 (GMT+5:30) Title : Re: [PATCH] input: gpio-keys - optimize wakeup sequence. To : Abhishek Kumar Singh<abhi1.singh@samsung.com> CC : robh@kernel.org<robh@kernel.org>, linux-input@vger.kernel.org<linux-input@vger.kernel.org>, linux-kernel@vger.kernel.org<linux-kernel@vger.kernel.org>, SRI-N IT Security<sri-n.itsec@samsung.com> 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, ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p2>]
* [PATCH] input: gpio-keys - optimize wakeup sequence. [not found] <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p2> @ 2023-10-17 10:34 ` Abhishek Kumar Singh 2023-10-18 21:49 ` Gareth Randall 0 siblings, 1 reply; 6+ messages in thread From: Abhishek Kumar Singh @ 2023-10-17 10:34 UTC (permalink / raw) To: dmitry.torokhov@gmail.com Cc: robh@kernel.org, linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, SRI-N IT Security [-- Attachment #1: Type: text/plain, Size: 1193 bytes --] Dear Mr. Dmitry, Greetings! This patch is related to optimization in input key event driver of Kernel module. Suggested change to avoid the many APIs call chain if there is 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 is 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); Suggested changes to 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" Please help to review the attached patch and integrate in main line kernel code. Thanks and Regards, Abhishek Kumar Singh Sr. Chief Engineer, Samsung Electronics, Noida-India [-- Attachment #2: input_keys_optimized.zip --] [-- Type: application/octet-stream, Size: 998 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] input: gpio-keys - optimize wakeup sequence. 2023-10-17 10:34 ` Abhishek Kumar Singh @ 2023-10-18 21:49 ` Gareth Randall 0 siblings, 0 replies; 6+ messages in thread From: Gareth Randall @ 2023-10-18 21:49 UTC (permalink / raw) To: abhi1.singh Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, SRI-N IT Security Dear Mr Singh, I am not a maintainer but can point out some issues you need to resolve with this post. 1. Put the patch in the body of the email and not in an attachment. 2. You need a "Signed-off-by:" line. 3. The email needs to start with a description of the what the patch resolves. Don't put "suggested changes". There are probably other issues as well but I hope this helps you to get started. Note that I am not involved in the review process. Yours, Gareth On 17/10/2023 11:34, Abhishek Kumar Singh wrote: > Dear Mr. Dmitry, > > Greetings! > > > > This patch is related to optimization in input key event driver of Kernel module. > > Suggested change to avoid the many APIs call chain if there is 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 is 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); > > > > > Suggested changes to 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" > > > Please help to review the attached patch and integrate in main line kernel code. > > > > > > Thanks and Regards, > Abhishek Kumar Singh > Sr. Chief Engineer, Samsung Electronics, Noida-India ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-20 4:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p4>
2023-10-26 5:53 ` [PATCH] input: gpio-keys - optimize wakeup sequence Abhishek Kumar Singh
2023-10-29 2:11 ` dmitry.torokhov
2023-11-07 8:29 ` Re[2]: " Abhishek Kumar Singh
2023-11-20 3:59 ` Re[3]: " Abhishek Kumar Singh
[not found] <CGME20231017103415epcms5p2f8f5b28a8f5d71055622b82f71b0fc93@epcms5p2>
2023-10-17 10:34 ` Abhishek Kumar Singh
2023-10-18 21:49 ` Gareth Randall
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox