[PATCH 0/4] Improve robustnes during initialization

[PATCH 0/4] Improve robustnes during initialization

[Marc Dietrich]

This series against 6.9-rc2 improves robustnes of the keyboard and
touchpad initialization with the goal to eliminate the ugly delay
it the i2c client controller ISR.

Marc Dietrich (4):
  staging: nvec: make keyboard init synchronous
  staging: nvec: make touchpad init synchronous
  staging: nvec: make i2c controller register writes robust
  staging: nvec: update TODO

 drivers/staging/nvec/TODO       |  1 -
 drivers/staging/nvec/nvec.c     | 39 +++++++++++++++++++--------------
 drivers/staging/nvec/nvec_kbd.c | 14 ++++++++----
 drivers/staging/nvec/nvec_ps2.c | 31 +++++++++++++++++---------
 4 files changed, 53 insertions(+), 32 deletions(-)

--
2.43.0

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

[Marc Dietrich]

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

 	return 0;
 }
--
2.43.0

[PATCH 2/4] staging: nvec: make touchpad init synchronous

[Marc Dietrich]

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_ps2.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/nvec/nvec_ps2.c b/drivers/staging/nvec/nvec_ps2.c
index cb6d71b8dc83..f34016c4a26b 100644
--- a/drivers/staging/nvec/nvec_ps2.c
+++ b/drivers/staging/nvec/nvec_ps2.c
@@ -60,16 +60,6 @@ static void ps2_stopstreaming(struct serio *ser_dev)
 	nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
 }

-static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
-{
-	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
-
-	buf[2] = cmd & 0xff;
-
-	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
-	return nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
-}
-
 static int nvec_ps2_notifier(struct notifier_block *nb,
 			     unsigned long event_type, void *data)
 {
@@ -98,6 +88,27 @@ static int nvec_ps2_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }

+static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
+{
+	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
+	struct nvec_msg *msg;
+	int ret;
+
+	buf[2] = cmd & 0xff;
+
+	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
+
+	ret = nvec_write_sync(ps2_dev.nvec, buf, sizeof(buf), &msg);
+	if (ret < 0)
+		return ret;
+
+	nvec_ps2_notifier(NULL, NVEC_PS2, msg->data);
+
+	nvec_msg_free(ps2_dev.nvec, msg);
+
+	return 0;
+}
+
 static int nvec_mouse_probe(struct platform_device *pdev)
 {
 	struct nvec_chip *nvec = dev_get_drvdata(pdev->dev.parent);
--
2.43.0

[PATCH 3/4] staging: nvec: make i2c controller register writes robust

[Marc Dietrich]

The i2c controller needs to read back the data written to its registers.
This way we can avoid the long delay in the interrupt handler.

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

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 282a664c9176..9914c30b6933 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec)
 		(uint)nvec->tx->size, nvec->tx->data[1]);
 }

+/**
+ * i2c_writel - savely write to an i2c client controller register
+ @ val: value to be written
+ @ reg: register to write to
+ */
+
+static void i2c_writel(u32 val, void *reg)
+{
+	writel_relaxed(val, reg);
+
+	/* read back register to make sure that register writes completed */
+	readl_relaxed(reg);
+}
+
 /**
  * nvec_interrupt - Interrupt handler
  * @irq: The IRQ
@@ -599,7 +613,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 	if ((status & RNW) == 0) {
 		received = readl(nvec->base + I2C_SL_RCVD);
 		if (status & RCVD)
-			writel(0, nvec->base + I2C_SL_RCVD);
+			i2c_writel(0, nvec->base + I2C_SL_RCVD);
 	}

 	if (status == (I2C_SL_IRQ | RCVD))
@@ -691,7 +705,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)

 	/* Send data if requested, but not on end of transmission */
 	if ((status & (RNW | END_TRANS)) == RNW)
-		writel(to_send, nvec->base + I2C_SL_RCVD);
+		i2c_writel(to_send, nvec->base + I2C_SL_RCVD);

 	/* If we have send the first byte */
 	if (status == (I2C_SL_IRQ | RNW | RCVD))
@@ -708,15 +722,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 		status & RCVD ? " RCVD" : "",
 		status & RNW ? " RNW" : "");

-	/*
-	 * TODO: replace the udelay with a read back after each writel above
-	 * in order to work around a hardware issue, see i2c-tegra.c
-	 *
-	 * Unfortunately, this change causes an intialisation issue with the
-	 * touchpad, which needs to be fixed first.
-	 */
-	udelay(100);
-
 	return IRQ_HANDLED;
 }

@@ -732,15 +737,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)

 	val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
 	    (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
-	writel(val, nvec->base + I2C_CNFG);
+	i2c_writel(val, nvec->base + I2C_CNFG);

 	clk_set_rate(nvec->i2c_clk, 8 * 80000);

-	writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
-	writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
+	i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
+	i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);

-	writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
-	writel(0, nvec->base + I2C_SL_ADDR2);
+	i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
+	i2c_writel(0, nvec->base + I2C_SL_ADDR2);

 	enable_irq(nvec->irq);
 }
@@ -749,7 +754,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
 static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
 {
 	disable_irq(nvec->irq);
-	writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
+	i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
 	clk_disable_unprepare(nvec->i2c_clk);
 }
 #endif
--
2.43.0

[PATCH 4/4] staging: nvec: update TODO

[Marc Dietrich]

Remove isr delay from the bill.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/TODO | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/nvec/TODO b/drivers/staging/nvec/TODO
index 8afde3ccc960..33f9ebe6d59b 100644
--- a/drivers/staging/nvec/TODO
+++ b/drivers/staging/nvec/TODO
@@ -1,5 +1,4 @@
 ToDo list (incomplete, unordered)
 	- move the driver to the new i2c slave framework
 	- finish suspend/resume support
-	- fix udelay in the isr
 	- add atomic ops in order to fix shutoff/reboot problems
--
2.43.0

Re: [PATCH 3/4] staging: nvec: make i2c controller register writes robust

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3933 bytes --]

On Fri Apr 5, 2024 at 4:09 PM CEST, Marc Dietrich wrote:
> The i2c controller needs to read back the data written to its registers.
> This way we can avoid the long delay in the interrupt handler.

s/i2c/I2C/ perhaps? Also, looking at the preceding patches it looks to
me like the reason why we can drop the udelay() call is not just because
we read back, but also because we actually wait for completion. If so,
maybe that should also be mentioned.

>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 282a664c9176..9914c30b6933 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec)
>  		(uint)nvec->tx->size, nvec->tx->data[1]);
>  }
>
> +/**
> + * i2c_writel - savely write to an i2c client controller register

"safely", also "I2C"

> + @ val: value to be written
> + @ reg: register to write to

The formatting here looks odd. I think this is supposed to be:

 * @val:
 * @reg:

Thierry

> + */
> +
> +static void i2c_writel(u32 val, void *reg)
> +{
> +	writel_relaxed(val, reg);
> +
> +	/* read back register to make sure that register writes completed */
> +	readl_relaxed(reg);
> +}
> +
>  /**
>   * nvec_interrupt - Interrupt handler
>   * @irq: The IRQ
> @@ -599,7 +613,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>  	if ((status & RNW) == 0) {
>  		received = readl(nvec->base + I2C_SL_RCVD);
>  		if (status & RCVD)
> -			writel(0, nvec->base + I2C_SL_RCVD);
> +			i2c_writel(0, nvec->base + I2C_SL_RCVD);
>  	}
>
>  	if (status == (I2C_SL_IRQ | RCVD))
> @@ -691,7 +705,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>
>  	/* Send data if requested, but not on end of transmission */
>  	if ((status & (RNW | END_TRANS)) == RNW)
> -		writel(to_send, nvec->base + I2C_SL_RCVD);
> +		i2c_writel(to_send, nvec->base + I2C_SL_RCVD);
>
>  	/* If we have send the first byte */
>  	if (status == (I2C_SL_IRQ | RNW | RCVD))
> @@ -708,15 +722,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>  		status & RCVD ? " RCVD" : "",
>  		status & RNW ? " RNW" : "");
>
> -	/*
> -	 * TODO: replace the udelay with a read back after each writel above
> -	 * in order to work around a hardware issue, see i2c-tegra.c
> -	 *
> -	 * Unfortunately, this change causes an intialisation issue with the
> -	 * touchpad, which needs to be fixed first.
> -	 */
> -	udelay(100);
> -
>  	return IRQ_HANDLED;
>  }
>
> @@ -732,15 +737,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
>
>  	val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
>  	    (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
> -	writel(val, nvec->base + I2C_CNFG);
> +	i2c_writel(val, nvec->base + I2C_CNFG);
>
>  	clk_set_rate(nvec->i2c_clk, 8 * 80000);
>
> -	writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
> -	writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
> +	i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
> +	i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
>
> -	writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
> -	writel(0, nvec->base + I2C_SL_ADDR2);
> +	i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
> +	i2c_writel(0, nvec->base + I2C_SL_ADDR2);
>
>  	enable_irq(nvec->irq);
>  }
> @@ -749,7 +754,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
>  static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
>  {
>  	disable_irq(nvec->irq);
> -	writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
> +	i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
>  	clk_disable_unprepare(nvec->i2c_clk);
>  }
>  #endif
> --
> 2.43.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2067 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] staging: nvec: make touchpad init synchronous

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]

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_ps2.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec_ps2.c b/drivers/staging/nvec/nvec_ps2.c
> index cb6d71b8dc83..f34016c4a26b 100644
> --- a/drivers/staging/nvec/nvec_ps2.c
> +++ b/drivers/staging/nvec/nvec_ps2.c
> @@ -60,16 +60,6 @@ static void ps2_stopstreaming(struct serio *ser_dev)
>  	nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
>  }
>
> -static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
> -{
> -	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
> -
> -	buf[2] = cmd & 0xff;
> -
> -	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
> -	return nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
> -}
> -
>  static int nvec_ps2_notifier(struct notifier_block *nb,
>  			     unsigned long event_type, void *data)
>  {
> @@ -98,6 +88,27 @@ static int nvec_ps2_notifier(struct notifier_block *nb,
>  	return NOTIFY_DONE;
>  }
>
> +static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
> +{
> +	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
> +	struct nvec_msg *msg;
> +	int ret;
> +
> +	buf[2] = cmd & 0xff;
> +
> +	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
> +
> +	ret = nvec_write_sync(ps2_dev.nvec, buf, sizeof(buf), &msg);
> +	if (ret < 0)
> +		return ret;
> +
> +	nvec_ps2_notifier(NULL, NVEC_PS2, msg->data);
> +
> +	nvec_msg_free(ps2_dev.nvec, msg);
> +
> +	return 0;
> +}
> +

Is there a particular reason why you've moved the function around? It'd
probably make the patch a tiny bit smaller if you kept it in the right
spot.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/4] Improve robustnes during initialization

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 880 bytes --]

On Fri Apr 5, 2024 at 4:09 PM CEST, Marc Dietrich wrote:
> This series against 6.9-rc2 improves robustnes of the keyboard and
> touchpad initialization with the goal to eliminate the ugly delay
> it the i2c client controller ISR.
>
> Marc Dietrich (4):
>   staging: nvec: make keyboard init synchronous
>   staging: nvec: make touchpad init synchronous
>   staging: nvec: make i2c controller register writes robust
>   staging: nvec: update TODO
>
>  drivers/staging/nvec/TODO       |  1 -
>  drivers/staging/nvec/nvec.c     | 39 +++++++++++++++++++--------------
>  drivers/staging/nvec/nvec_kbd.c | 14 ++++++++----
>  drivers/staging/nvec/nvec_ps2.c | 31 +++++++++++++++++---------
>  4 files changed, 53 insertions(+), 32 deletions(-)

Overall very nice cleanup, thanks for tackling this. I've left a few
minor comments on the individual patches.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Dan Carpenter]

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

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

[Dan Carpenter]

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

Re: [PATCH 3/4] staging: nvec: make i2c controller register writes robust

[Dan Carpenter]

On Fri, Apr 05, 2024 at 04:09:05PM +0200, Marc Dietrich wrote:
> The i2c controller needs to read back the data written to its registers.
> This way we can avoid the long delay in the interrupt handler.
> 
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  drivers/staging/nvec/nvec.c | 39 +++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index 282a664c9176..9914c30b6933 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -565,6 +565,20 @@ static void nvec_tx_set(struct nvec_chip *nvec)
>  		(uint)nvec->tx->size, nvec->tx->data[1]);
>  }
> 
> +/**
> + * i2c_writel - savely write to an i2c client controller register
> + @ val: value to be written
> + @ reg: register to write to
> + */
> +
> +static void i2c_writel(u32 val, void *reg)

I'm not a expert at kernel doc or whatever, but this comment isn't in
the right format.  And delete the blank line between the comment and the
function.

/**
 * i2c_writel - savely write to an i2c client controller register
 * @ val: value to be written
 * @ reg: register to write to
 */
static void i2c_writel(u32 val, void *reg)

regards,
dan carpenter

Re: [PATCH 2/4] staging: nvec: make touchpad init synchronous

[Marc Dietrich]

Hello Thierry,

On Fri, 5 Apr 2024, Thierry Reding wrote:

> 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_ps2.c | 31 +++++++++++++++++++++----------
>>  1 file changed, 21 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/staging/nvec/nvec_ps2.c b/drivers/staging/nvec/nvec_ps2.c
>> index cb6d71b8dc83..f34016c4a26b 100644
>> --- a/drivers/staging/nvec/nvec_ps2.c
>> +++ b/drivers/staging/nvec/nvec_ps2.c
>> @@ -60,16 +60,6 @@ static void ps2_stopstreaming(struct serio *ser_dev)
>>  	nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
>>  }
>>
>> -static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
>> -{
>> -	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
>> -
>> -	buf[2] = cmd & 0xff;
>> -
>> -	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
>> -	return nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
>> -}
>> -
>>  static int nvec_ps2_notifier(struct notifier_block *nb,
>>  			     unsigned long event_type, void *data)
>>  {
>> @@ -98,6 +88,27 @@ static int nvec_ps2_notifier(struct notifier_block *nb,
>>  	return NOTIFY_DONE;
>>  }
>>
>> +static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
>> +{
>> +	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
>> +	struct nvec_msg *msg;
>> +	int ret;
>> +
>> +	buf[2] = cmd & 0xff;
>> +
>> +	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
>> +
>> +	ret = nvec_write_sync(ps2_dev.nvec, buf, sizeof(buf), &msg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	nvec_ps2_notifier(NULL, NVEC_PS2, msg->data);
>> +
>> +	nvec_msg_free(ps2_dev.nvec, msg);
>> +
>> +	return 0;
>> +}
>> +
>
> Is there a particular reason why you've moved the function around? It'd
> probably make the patch a tiny bit smaller if you kept it in the right
> spot.

because I'm calling nvec_ps2_notifier, I either need to to move the
function down or add a forward declaration. I prefered moving down because
that keeps the code a bit cleaner (with the cost of a slighly bigger
patch).

Marc

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

[Marc Dietrich]

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