[PATCH v2 1/2] staging: sm750fb: Replace busy-wait loop with udelay()

[PATCH v2 1/2] staging: sm750fb: Replace busy-wait loop with udelay()

[Oarora Etimis]

From: OaroraEtimis <OaroraEtimis@gmail.com>

The empty for-loop in sw_i2c_wait() triggers a -Wunused-but-set-variable
warning and can be optimized away by modern compilers.

Fix this by replacing the unreliable loop with a standard udelay(2).
This also cleans up the unused 'tmp' variable and outdated comments.
Include <linux/delay.h> to resolve the implicit function declaration.

Signed-off-by: OaroraEtimis <OaroraEtimis@gmail.com>
---
Changes in v2:
- Rebased onto the latest staging-next branch to resolve merge conflicts.
- No logical code changes.

 drivers/staging/sm750fb/ddk750_swi2c.c | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
index e63f3b00ec4c..d579cb9da79e 100644
--- a/drivers/staging/sm750fb/ddk750_swi2c.c
+++ b/drivers/staging/sm750fb/ddk750_swi2c.c
@@ -11,6 +11,7 @@
 #include "ddk750_reg.h"
 #include "ddk750_swi2c.h"
 #include "ddk750_power.h"
+#include <linux/delay.h>
 
 /*
  * I2C Software Master Driver:
@@ -80,24 +81,7 @@ static unsigned long sw_i2c_data_gpio_data_dir_reg = GPIO_DATA_DIRECTION;
  */
 static void sw_i2c_wait(void)
 {
-	/* find a bug:
-	 * peekIO method works well before suspend/resume
-	 * but after suspend, peekIO(0x3ce,0x61) & 0x10
-	 * always be non-zero,which makes the while loop
-	 * never finish.
-	 * use non-ultimate for loop below is safe
-	 */
-
-    /* Change wait algorithm to use PCI bus clock,
-     * it's more reliable than counter loop ..
-     * write 0x61 to 0x3ce and read from 0x3cf
-     */
-	int i, tmp;
-
-	for (i = 0; i < 600; i++) {
-		tmp = i;
-		tmp += i;
-	}
+	udelay(2);
 }
 
 /*
-- 
2.47.3

Re: [PATCH v2 1/2] staging: sm750fb: Replace busy-wait loop with udelay()

[Greg KH]

On Mon, Mar 16, 2026 at 07:20:42AM +0800, Oarora Etimis wrote:
> From: OaroraEtimis <OaroraEtimis@gmail.com>
> 
> The empty for-loop in sw_i2c_wait() triggers a -Wunused-but-set-variable
> warning and can be optimized away by modern compilers.
> 
> Fix this by replacing the unreliable loop with a standard udelay(2).
> This also cleans up the unused 'tmp' variable and outdated comments.
> Include <linux/delay.h> to resolve the implicit function declaration.
> 
> Signed-off-by: OaroraEtimis <OaroraEtimis@gmail.com>
> ---
> Changes in v2:
> - Rebased onto the latest staging-next branch to resolve merge conflicts.
> - No logical code changes.
> 
>  drivers/staging/sm750fb/ddk750_swi2c.c | 20 ++------------------
>  1 file changed, 2 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/ddk750_swi2c.c b/drivers/staging/sm750fb/ddk750_swi2c.c
> index e63f3b00ec4c..d579cb9da79e 100644
> --- a/drivers/staging/sm750fb/ddk750_swi2c.c
> +++ b/drivers/staging/sm750fb/ddk750_swi2c.c
> @@ -11,6 +11,7 @@
>  #include "ddk750_reg.h"
>  #include "ddk750_swi2c.h"
>  #include "ddk750_power.h"
> +#include <linux/delay.h>
>  
>  /*
>   * I2C Software Master Driver:
> @@ -80,24 +81,7 @@ static unsigned long sw_i2c_data_gpio_data_dir_reg = GPIO_DATA_DIRECTION;
>   */
>  static void sw_i2c_wait(void)
>  {
> -	/* find a bug:
> -	 * peekIO method works well before suspend/resume
> -	 * but after suspend, peekIO(0x3ce,0x61) & 0x10
> -	 * always be non-zero,which makes the while loop
> -	 * never finish.
> -	 * use non-ultimate for loop below is safe
> -	 */
> -
> -    /* Change wait algorithm to use PCI bus clock,
> -     * it's more reliable than counter loop ..
> -     * write 0x61 to 0x3ce and read from 0x3cf
> -     */
> -	int i, tmp;
> -
> -	for (i = 0; i < 600; i++) {
> -		tmp = i;
> -		tmp += i;
> -	}
> +	udelay(2);

How is "2" the same as this busy loop?

And why not fix this properly, as the comment states?  You just removed
that information so no one knows how to do this in the future :(

thanks,

greg k-h

Re: [PATCH v2 1/2] staging: sm750fb: Replace busy-wait loop with udelay()

[OaroraEtimis]

Hi Greg,

Thanks for the review. Sorry for dropping the historical comment in v2. 
My only goal was to fix the -Wunused-but-set-variable warning and 
prevent the loop from being optimized away by the compiler.

I will definitely restore the comment.


On Mon, Mar 16, 2026 at 14:11, Greg KH wrote:

 > How is "2" the same as this busy loop?

It was a rough estimation. A 600-iteration empty loop on older CPUs 
(~500MHz) took about 2 to 3 microseconds.


 > And why not fix this properly, as the comment states?

The comment suggests writing to VGA ports (0x3ce/0x3cf) to force a 
delay. I didn't implement this because I don't have the specific 
hardware or datasheets to test it.

I was afraid that introducing direct VGA I/O just to fix a compiler 
warning might cause unexpected hardware regressions *or compatibility 
issues across different platforms.*

Given that I can't test hardware I/O, how would you prefer I handle this 
in v3?

1. Keep the original loop but add cpu_relax() inside to prevent compiler 
optimization. (Safest for the hardware)
2. Use udelay(2) (or ndelay) and restore the historical comment.
3. Migrate the driver to the standard i2c-algo-bit framework (a much 
heavier refactoring).

I'd appreciate your guidance on the best path forward for this staging 
driver.

Thanks,
Oarora

Re: [PATCH v2 1/2] staging: sm750fb: Replace busy-wait loop with udelay()

[Greg KH]

On Mon, Mar 16, 2026 at 03:42:04PM +0800, OaroraEtimis wrote:
> Hi Greg,
> 
> Thanks for the review. Sorry for dropping the historical comment in v2. My
> only goal was to fix the -Wunused-but-set-variable warning and prevent the
> loop from being optimized away by the compiler.
> 
> I will definitely restore the comment.
> 
> 
> On Mon, Mar 16, 2026 at 14:11, Greg KH wrote:
> 
> > How is "2" the same as this busy loop?
> 
> It was a rough estimation. A 600-iteration empty loop on older CPUs
> (~500MHz) took about 2 to 3 microseconds.
> 
> 
> > And why not fix this properly, as the comment states?
> 
> The comment suggests writing to VGA ports (0x3ce/0x3cf) to force a delay. I
> didn't implement this because I don't have the specific hardware or
> datasheets to test it.
> 
> I was afraid that introducing direct VGA I/O just to fix a compiler warning
> might cause unexpected hardware regressions *or compatibility issues across
> different platforms.*
> 
> Given that I can't test hardware I/O, how would you prefer I handle this in
> v3?

Don't do any logical change that testing without hardware can not be
verified (i.e. whitespace changes).

> 1. Keep the original loop but add cpu_relax() inside to prevent compiler
> optimization. (Safest for the hardware)

Are you sure?  Can you sleep at that point in time?  What does the
performance of the device then look like?

> 2. Use udelay(2) (or ndelay) and restore the historical comment.
> 3. Migrate the driver to the standard i2c-algo-bit framework (a much heavier
> refactoring).

Number 3 is the best way.

good luck!

greg k-h