[PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred over udelay warnings

[PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred over udelay warnings

[Moritz C. Weber]

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 *dev)
 		break;
 	case 2:		/* first byte after command */
 		if (status == (I2C_SL_IRQ | RNW | RCVD)) {
-			udelay(33);
+			usleep_range(32, 33);
 			if (nvec->rx->data[0] != 0x01) {
 				dev_err(nvec->dev,
 					"Read without prior read command\n");
@@ -714,7 +714,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 	 * We experience less incomplete messages with this delay than without
 	 * it, but we don't know why. Help is appreciated.
 	 */
-	udelay(100);
+	usleep_range(99, 100);
 
 	return IRQ_HANDLED;
 }
-- 
2.30.2

Re: [PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred over udelay warnings

[Dan Carpenter]

On Mon, Feb 12, 2024 at 02:36:45PM +0100, Moritz C. Weber wrote:
> Fixed a code style issue raised by checkpatch.

Needs Signed-off-by.  Please run your patches through 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 *dev)
>  		break;
>  	case 2:		/* first byte after command */
>  		if (status == (I2C_SL_IRQ | RNW | RCVD)) {
> -			udelay(33);
> +			usleep_range(32, 33);

We only accept these udelay() -> usleep_range() patches if they have
been tested on real hardware.  Sorry.

regards,
dan carpenter

Re: [PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred over udelay warnings

[Nam Cao]

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 *dev)
>  		break;
>  	case 2:		/* first byte after command */
>  		if (status == (I2C_SL_IRQ | RNW | RCVD)) {
> -			udelay(33);
> +			usleep_range(32, 33);
>  			if (nvec->rx->data[0] != 0x01) {
>  				dev_err(nvec->dev,
>  					"Read without prior read command\n");
> @@ -714,7 +714,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>  	 * We experience less incomplete messages with this delay than without
>  	 * 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 be
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. The
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.

Best regards,
Nam

Re: [PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred over udelay warnings

[Thierry Reding]

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

On Mon Feb 12, 2024 at 3:21 PM CET, 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 *dev)
> >  		break;
> >  	case 2:		/* first byte after command */
> >  		if (status == (I2C_SL_IRQ | RNW | RCVD)) {
> > -			udelay(33);
> > +			usleep_range(32, 33);
> >  			if (nvec->rx->data[0] != 0x01) {
> >  				dev_err(nvec->dev,
> >  					"Read without prior read command\n");
> > @@ -714,7 +714,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
> >  	 * We experience less incomplete messages with this delay than without
> >  	 * 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 be
> 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. The
> 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.

Also, pay attention to what else is being said in the timers-howto.rst
documentation. It specifically mentions that usleep_range() uses a range
in order to give the scheduler some leeway in coalescing with other
wakeups, so choosing a range of 32-33 us or 99-100 us isn't very useful.

Thierry

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

Re: [PATCH] Staging: nvec: nvec: fixed two usleep_range is preferred over udelay warnings

[Marc Dietrich]

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 *dev)
>>  		break;
>>  	case 2:		/* first byte after command */
>>  		if (status == (I2C_SL_IRQ | RNW | RCVD)) {
>> -			udelay(33);
>> +			usleep_range(32, 33);
>>  			if (nvec->rx->data[0] != 0x01) {
>>  				dev_err(nvec->dev,
>>  					"Read without prior read command\n");
>> @@ -714,7 +714,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
>>  	 * We experience less incomplete messages with this delay than without
>>  	 * 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 be
> 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. The
> 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