diff mbox series

[1/4] staging: nvec: make keyboard init synchronous

Message ID 20240405140906.77831-2-marvin24@gmx.de
State Superseded
Headers show
Series Improve robustnes during initialization | expand

Commit Message

Marc Dietrich April 5, 2024, 2:09 p.m. UTC 
Improve initialization stability by waiting for command completion before
sending the next one.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/nvec_kbd.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

--
2.43.0

Comments

Thierry Reding April 5, 2024, 2:38 p.m. UTC | #1 
On Fri Apr 5, 2024 at 4:09 PM CEST, Marc Dietrich wrote:
> Improve initialization stability by waiting for command completion before
> sending the next one.
>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  drivers/staging/nvec/nvec_kbd.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec_kbd.c b/drivers/staging/nvec/nvec_kbd.c
> index f9a1da952c0a..6b203d28b8a9 100644
> --- a/drivers/staging/nvec/nvec_kbd.c
> +++ b/drivers/staging/nvec/nvec_kbd.c
> @@ -113,6 +113,7 @@ static int nvec_kbd_probe(struct platform_device *pdev)
>  		cnfg_wake[] = { NVEC_KBD, CNFG_WAKE, true, true },
>  		cnfg_wake_key_reporting[] = { NVEC_KBD, CNFG_WAKE_KEY_REPORTING,
>  						true };
> +	struct nvec_msg *msg;
>
>  	j = 0;
>
> @@ -148,15 +149,20 @@ static int nvec_kbd_probe(struct platform_device *pdev)
>  	nvec_register_notifier(nvec, &keys_dev.notifier, 0);
>
>  	/* Enable keyboard */
> -	nvec_write_async(nvec, enable_kbd, 2);
> +	nvec_write_sync(nvec, enable_kbd, 2, &msg);
> +	nvec_msg_free(nvec, msg);
>
>  	/* configures wake on special keys */
> -	nvec_write_async(nvec, cnfg_wake, 4);
> +	nvec_write_sync(nvec, cnfg_wake, 4, &msg);
> +	nvec_msg_free(nvec, msg);
> +
>  	/* enable wake key reporting */
> -	nvec_write_async(nvec, cnfg_wake_key_reporting, 3);
> +	nvec_write_sync(nvec, cnfg_wake_key_reporting, 3, &msg);
> +	nvec_msg_free(nvec, msg);
>
>  	/* Disable caps lock LED */
> -	nvec_write_async(nvec, clear_leds, sizeof(clear_leds));
> +	nvec_write_sync(nvec, clear_leds, sizeof(clear_leds), &msg);
> +	nvec_msg_free(nvec, msg);

I wonder if perhaps some of this duplication can be folded into
nvec_write_sync(). It seems a bit unnecessary to have to first get hold
of the last message only to immediately free it.

If nvec_write_sync() were allowed to take a NULL as msg parameter, then
we could check if this special case and simply do the nvec_msg_free()
from there directly. Not sure if it's really worth it, though.

Thierry
Dan Carpenter April 5, 2024, 3:15 p.m. UTC | #2 
On Fri, Apr 05, 2024 at 04:09:03PM +0200, Marc Dietrich wrote:
> Improve initialization stability by waiting for command completion before
> sending the next one.
> 

Presumably you experienced an issue with this in real life.  Can you
describe what that looked like in your commit message?  Should this
commit be sent to stable?

regards,
dan carpenter
Dan Carpenter April 5, 2024, 3:19 p.m. UTC | #3 
On Fri, Apr 05, 2024 at 06:15:54PM +0300, Dan Carpenter wrote:
> On Fri, Apr 05, 2024 at 04:09:03PM +0200, Marc Dietrich wrote:
> > Improve initialization stability by waiting for command completion before
> > sending the next one.
> > 
> 
> Presumably you experienced an issue with this in real life.  Can you
> describe what that looked like in your commit message?  Should this
> commit be sent to stable?

Actually, some of this information is in the cover letter but the cover
letter isn't preserved in the git log so it's better to put it in the
commits themselves.

regards,
dan carpenter
Marc Dietrich April 6, 2024, 12:26 p.m. UTC | #4 
Hi Dan,

On Fri, 5 Apr 2024, Dan Carpenter wrote:

> On Fri, Apr 05, 2024 at 06:15:54PM +0300, Dan Carpenter wrote:
>> On Fri, Apr 05, 2024 at 04:09:03PM +0200, Marc Dietrich wrote:
>>> Improve initialization stability by waiting for command completion before
>>> sending the next one.
>>>
>>
>> Presumably you experienced an issue with this in real life.  Can you
>> describe what that looked like in your commit message?  Should this
>> commit be sent to stable?
>
> Actually, some of this information is in the cover letter but the cover
> letter isn't preserved in the git log so it's better to put it in the
> commits themselves.

ok, I'm going to send a new version of the series which hopefully
addresses yous and Thierry's comments.

Best regards,

Marc
diff mbox series

Patch

diff --git a/drivers/staging/nvec/nvec_kbd.c b/drivers/staging/nvec/nvec_kbd.c
index f9a1da952c0a..6b203d28b8a9 100644
--- a/drivers/staging/nvec/nvec_kbd.c
+++ b/drivers/staging/nvec/nvec_kbd.c
@@ -113,6 +113,7 @@  static int nvec_kbd_probe(struct platform_device *pdev)
 		cnfg_wake[] = { NVEC_KBD, CNFG_WAKE, true, true },
 		cnfg_wake_key_reporting[] = { NVEC_KBD, CNFG_WAKE_KEY_REPORTING,
 						true };
+	struct nvec_msg *msg;

 	j = 0;

@@ -148,15 +149,20 @@  static int nvec_kbd_probe(struct platform_device *pdev)
 	nvec_register_notifier(nvec, &keys_dev.notifier, 0);

 	/* Enable keyboard */
-	nvec_write_async(nvec, enable_kbd, 2);
+	nvec_write_sync(nvec, enable_kbd, 2, &msg);
+	nvec_msg_free(nvec, msg);

 	/* configures wake on special keys */
-	nvec_write_async(nvec, cnfg_wake, 4);
+	nvec_write_sync(nvec, cnfg_wake, 4, &msg);
+	nvec_msg_free(nvec, msg);
+
 	/* enable wake key reporting */
-	nvec_write_async(nvec, cnfg_wake_key_reporting, 3);
+	nvec_write_sync(nvec, cnfg_wake_key_reporting, 3, &msg);
+	nvec_msg_free(nvec, msg);

 	/* Disable caps lock LED */
-	nvec_write_async(nvec, clear_leds, sizeof(clear_leds));
+	nvec_write_sync(nvec, clear_leds, sizeof(clear_leds), &msg);
+	nvec_msg_free(nvec, msg);

 	return 0;
 }