Received-SPF: pass (google.com: domain of linux-kernel+bounces-70463-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 18 Feb 2024 18:42:57 +0100 (CET)
From: Marc Dietrich <marvin24@gmx.de>
To: Nam Cao <namcao@linutronix.de>
cc: "Moritz C. Weber" <mo.c.weber@gmail.com>, marvin24@gmx.de, 
    ac100@lists.launchpad.net, linux-tegra@vger.kernel.org, 
    linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred
 over udelay warnings
In-Reply-To: <20240212152110.4f8fe0e6@namcao>
Message-ID: <569e7346-1063-3ef9-8bd0-3094fe200074@gmx.de>
References: <20240212133645.1836-1-mo.c.weber@gmail.com> <20240212152110.4f8fe0e6@namcao>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII; format=flowed
UI-OutboundReport: notjunk:1;M01:P0:PHBZs4daLeY=;/Kbot9neqGcH+Ta8Hr5K1f7jrAU
 JSs6cxjXRJGuxuB639lcklW+Uw/mfk8Ot5j7BlOTXVwHQKl9F/a8VltVyUEOE6W6glhkV0tw+
 wr+PZh4uIXIxZv+Fwkw067Aw5c/0yw+9IgZ+gvZeoxHdsoevog89aLQq8bl1PklPPaGf98QLF
 1nW49ArhvbCd7/fIO+xArnw2BsogBYhR3Vpgv52smz4AV6wuOWDFLDuBqc1OYO/mgYgs0p0+o
 mZ/p1pAW4qWw1L3d7etlwpcBWx3p1P5W7l6NV62dG963BiVOhjjlK1gz20EW6RZsLQ9TUMhwH
 +GJJSYkQxQOixRiGeAs/GC0nTpxL6M19Yd/TmeVRJe8XAOAkDl07UdrivxZrC5VUqOrCKuyib
 xyCUiyVUdeOqH8oXJQ4Zi8fUwDqd5pfHn6PsFNafrwwbqRoUftfNQAnq6sL8O+JCxVaktiiqK
 mL8DULWYPRa2787t/BcfXMIVHkTdhov/2jNKNgYSZQhZVj2LjS+QH2u9Ch2K+ddoDO0krEysw
 ah5SwiIKzw2SYFZjL6bWqXz4hA/2XUvstklzyji21zd9FnBSpXVNVujLwaixz+yail1nzDapB
 cMoIbzvEXmjEJqyAPxtoJs3GLfM9mZ4lJUXaNB9jtRDzt6F1busQElUfOlUTRNaDMEpWPsptF
 RwZiUq4Rqq/xNofIio33tzs4OmllLJh/HIinOYK4H9jYRm3rLVbDVXisxy1WDBf5MsYbcAbdB
 JEgifm+C3tC1jyAWpwLEpQH+mWKTmID/BHfr7lEGj8Vb/9ASMuK80dWfS3WOiQaPxnS5Us791
 lpstGJmmHPamqMTV2v7jewjcnaABtEEzq0HbgmDrqGss8=
Content-Transfer-Encoding: quoted-printable

Hi,

On Mon, 12 Feb 2024, Nam Cao wrote:

> On 12/Feb/2024 Moritz C. Weber wrote:
>> Fixed a code style issue raised by checkpatch.
>> ---
>>  drivers/staging/nvec/nvec.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
>> index 2823cacde..18c5471d5 100644
>> --- a/drivers/staging/nvec/nvec.c
>> +++ b/drivers/staging/nvec/nvec.c
>> @@ -627,7 +627,7 @@ static irqreturn_t nvec_interrupt(int irq, void *de=
v)
>>  		break;
>>  	case 2:		/* first byte after command */
>>  		if (status =3D=3D (I2C_SL_IRQ | RNW | RCVD)) {
>> -			udelay(33);
>> +			usleep_range(32, 33);
>>  			if (nvec->rx->data[0] !=3D 0x01) {
>>  				dev_err(nvec->dev,
>>  					"Read without prior read command\n");
>> @@ -714,7 +714,7 @@ static irqreturn_t nvec_interrupt(int irq, void *de=
v)
>>  	 * We experience less incomplete messages with this delay than withou=
t
>>  	 * it, but we don't know why. Help is appreciated.
>>  	 */
>> -	udelay(100);
>> +	usleep_range(99, 100);
>>
>>  	return IRQ_HANDLED;
>>  }
>
> I have zero knowledge about this driver, but nvec_interrupt() seems to b=
e
> a hard interrupt handler, and sleeping in an interrupt handler is a big =
no
> no. So I think this change breaks the driver.
>
> Delaying like the driver is currently doing doesn't break things, but it=
 is
> not very nice because this is interrupt handler and the processor cannot
> switch to other tasks, so delaying is wasting processor's cycles here. T=
he
> better fix would be to figure out how to remove the delay entirely, or
> switch to threaded interrupt handler and then we can use usleep_range() =
in
> there, but you need actual hardware to test such changes.

the real fix to read back the value we wrote to the controller, similar to
what is done for the tegra i2c host, which is a trival fix.
Unfortunately, this breaks the touch pad initialisation, which needs to be
fixed first. As this topic comes up every year, I'm going to post a patch
in order to updated the comment, so this kind of patches should
(hopefully) stop in the future.

Best regrards,

Marc