[PATCH 1/2] staging/nvec: update comment regarding udelay in the isr

[PATCH 1/2] staging/nvec: update comment regarding udelay in the isr

[Marc Dietrich]

Update the comment before the udelay on how to replace it.

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

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index b3f114cb00dc..ec081d81a308 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -709,10 +709,11 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 		status & RNW ? " RNW" : "");

 	/*
-	 * TODO: A correct fix needs to be found for this.
+	 * TODO: replace the udelay with a read back after each writel above
+	 * in order to work around a hardware issue, see i2c-tegra.c
 	 *
-	 * We experience less incomplete messages with this delay than without
-	 * it, but we don't know why. Help is appreciated.
+	 * Unfortunately, this change causes an intialisation issue with the
+	 * touchpad, which needs to be fixed first.
 	 */
 	udelay(100);

--
2.40.1

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

[Marc Dietrich]

replace old/implemented items with new todo list

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

diff --git a/drivers/staging/nvec/TODO b/drivers/staging/nvec/TODO
index e4d85d9b4681..8afde3ccc960 100644
--- a/drivers/staging/nvec/TODO
+++ b/drivers/staging/nvec/TODO
@@ -1,6 +1,5 @@
 ToDo list (incomplete, unordered)
-	- add compile as module support
-	- move half of the nvec init stuff to i2c-tegra.c
-	- move event handling to nvec_events
+	- move the driver to the new i2c slave framework
 	- finish suspend/resume support
-	- add support for more device implementations
+	- fix udelay in the isr
+	- add atomic ops in order to fix shutoff/reboot problems
--
2.40.1

Re: [PATCH 1/2] staging/nvec: update comment regarding udelay in the isr

[Thierry Reding]

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

On Sun Feb 18, 2024 at 10:25 PM CET, Marc Dietrich wrote:
> Update the comment before the udelay on how to replace it.
>
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  drivers/staging/nvec/nvec.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> index b3f114cb00dc..ec081d81a308 100644
> --- a/drivers/staging/nvec/nvec.c
> +++ b/drivers/staging/nvec/nvec.c
> @@ -709,10 +709,11 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>  		status & RNW ? " RNW" : "");
>
>  	/*
> -	 * TODO: A correct fix needs to be found for this.
> +	 * TODO: replace the udelay with a read back after each writel above
> +	 * in order to work around a hardware issue, see i2c-tegra.c

i2c-tegra.c is almost 2000 lines, so it'd be good to be a little more
specific. Are you referring to the work-around in i2c_writel()? If so,
perhaps mention that function so that people can find it.

>  	 *
> -	 * We experience less incomplete messages with this delay than without
> -	 * it, but we don't know why. Help is appreciated.
> +	 * Unfortunately, this change causes an intialisation issue with the
> +	 * touchpad, which needs to be fixed first.

Perhaps something like: "... this change would cause an initialisation
issue... " to make it a little more clear what you are referencing. Is
there any information about the touchpad issue? Any idea what's going
wrong during the initialization?

Seems like this issue has been around a very long time already, and the
further we kick this can down the road the less likely it will be that
we'll ever fix it.

Thierry

>  	 */
>  	udelay(100);
>
> --
> 2.40.1


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

Re: [PATCH 1/2] staging/nvec: update comment regarding udelay in the isr

[Marc Dietrich]

Hi Thierry,

sorry to reply late, but I somehow missed your mail.

On Mon, 19 Feb 2024, Thierry Reding wrote:

> On Sun Feb 18, 2024 at 10:25 PM CET, Marc Dietrich wrote:
>> Update the comment before the udelay on how to replace it.
>>
>> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
>> ---
>>  drivers/staging/nvec/nvec.c | 7 ++++---
>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
>> index b3f114cb00dc..ec081d81a308 100644
>> --- a/drivers/staging/nvec/nvec.c
>> +++ b/drivers/staging/nvec/nvec.c
>> @@ -709,10 +709,11 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>>  		status & RNW ? " RNW" : "");
>>
>>  	/*
>> -	 * TODO: A correct fix needs to be found for this.
>> +	 * TODO: replace the udelay with a read back after each writel above
>> +	 * in order to work around a hardware issue, see i2c-tegra.c
>
> i2c-tegra.c is almost 2000 lines, so it'd be good to be a little more
> specific. Are you referring to the work-around in i2c_writel()? If so,
> perhaps mention that function so that people can find it.

yes, I thought this should have been clear as there is (nearly) no write
without a readback in that driver.

>>  	 *
>> -	 * We experience less incomplete messages with this delay than without
>> -	 * it, but we don't know why. Help is appreciated.
>> +	 * Unfortunately, this change causes an intialisation issue with the
>> +	 * touchpad, which needs to be fixed first.
>
> Perhaps something like: "... this change would cause an initialisation
> issue... " to make it a little more clear what you are referencing. Is
> there any information about the touchpad issue? Any idea what's going
> wrong during the initialization?
>
> Seems like this issue has been around a very long time already, and the
> further we kick this can down the road the less likely it will be that
> we'll ever fix it.

I must confess that I wasn't motivated to fix it, because it works most
of the time. The driver needs more love than just fixing this issue, e.g.
implementing a reliable sync write which may be one of the underlying
problems.

On the other hand, there is a downstream i2c client implementation [1],
which uses the kernel i2c infrastructure and may lay a better foundation
for the current driver.

I'm also not happy with the current situation, but the only alternative
is to remove the driver completly. I'll try to find some time to improve
the keyboard / touchpad initialisation in the next weeks.

Marc

[1]
https://nv-tegra.nvidia.com/r/plugins/gitiles/linux-4.9/+/refs/heads/l4t/l4t-r31.0.1/drivers/i2c/busses/i2c-tegra-slave.c