Re: [PATCH v2] staging: sm750fb: replace magic number with jiffies timeout

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Jan 27, 2026 at 04:48:16PM +0000, Madhumitha Sundar wrote:
> The hardware wait loop in hw_sm750_de_wait used a hardcoded loop
> counter (0x10000000), which depends on CPU speed and is unreliable.
> 
> Replace the loop counter with a jiffies-based timeout (1 second)
> using time_before(). This ensures consistent delays across architectures.
> 
> Signed-off-by: Madhumitha Sundar <madhuananda18@gmail.com>
> ---
> Changes in v2:
>  - Switched from loop counter to jiffies + cpu_relax() as requested by Greg KH.
> ---
>  drivers/staging/sm750fb/sm750_hw.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/sm750fb/sm750_hw.c b/drivers/staging/sm750fb/sm750_hw.c
> index ce46f240c..b24d27a77 100644
> --- a/drivers/staging/sm750fb/sm750_hw.c
> +++ b/drivers/staging/sm750fb/sm750_hw.c
> @@ -523,19 +523,32 @@ int hw_sm750le_de_wait(void)
>  
>  int hw_sm750_de_wait(void)
>  {
> -	int i = 0x10000000;
> +	/* Wait for 1 second (HZ) */

No need for a comment, right?

> +	unsigned long timeout = jiffies + HZ;

Why is a variable needed?

And are you sure 1 second is ok?  How short was the original timeout?

>  	unsigned int mask = SYSTEM_CTRL_DE_STATUS_BUSY |
>  			    SYSTEM_CTRL_DE_FIFO_EMPTY |
>  			    SYSTEM_CTRL_DE_MEM_FIFO_EMPTY;
> +	unsigned int val;

Why move this?

> -	while (i--) {
> -		unsigned int val = peek32(SYSTEM_CTRL);
> +	/* Run while Current Time is BEFORE the Deadline */

Run what?

> +	while (time_before(jiffies, timeout)) {
> +		val = peek32(SYSTEM_CTRL);
>  
> +		/* If Not Busy (0) AND Buffers Empty (1), we are good */
>  		if ((val & mask) ==
>  		    (SYSTEM_CTRL_DE_FIFO_EMPTY | SYSTEM_CTRL_DE_MEM_FIFO_EMPTY))
>  			return 0;
> +
> +		/* Polite pause to save power */
> +		cpu_relax();

Are you sure you can sleep here?

>  	}
> -	/* timeout error */
> +
> +	/* Final check in case we succeeded at the last millisecond */

I do not understand this, why would this matter?

> +	val = peek32(SYSTEM_CTRL);
> +	if ((val & mask) ==
> +	    (SYSTEM_CTRL_DE_FIFO_EMPTY | SYSTEM_CTRL_DE_MEM_FIFO_EMPTY))
> +		return 0;
> +

Was this tested on real hardware?

thanks,

greg k-h