Re: [PATCH v1 2/4] staging: fbtft: ssd1351: send gamma table via fbtft_write_buf_dc()

[Dan Carpenter <dan.carpenter@linaro.org>]

On Sun, Jan 04, 2026 at 07:06:36PM +0800, Sun Jian wrote:
> Clang reports a large stack frame in set_gamma() (-Wframe-larger-than=1024)
> due to the large write_reg() call emitting 63 gamma bytes via varargs.
> 
> Send the command byte (0xB8) and the gamma payload using
> fbtft_write_buf_dc() to avoid the varargs/NUMARGS stack blow-up.
> 
> No functional change intended.
> 
> Signed-off-by: Sun Jian <sun.jian.kdev@gmail.com>
> ---
>  drivers/staging/fbtft/fb_ssd1351.c | 35 +++++++++++++-----------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/fbtft/fb_ssd1351.c b/drivers/staging/fbtft/fb_ssd1351.c
> index 6736b09b2f45..b4ab2c81e528 100644
> --- a/drivers/staging/fbtft/fb_ssd1351.c
> +++ b/drivers/staging/fbtft/fb_ssd1351.c
> @@ -119,43 +119,38 @@ static int set_var(struct fbtft_par *par)
>   */
>  static int set_gamma(struct fbtft_par *par, u32 *curves)
>  {
> -	unsigned long tmp[GAMMA_NUM * GAMMA_LEN];
> +	u8 data[GAMMA_LEN];

Ugh...  GAMMA_NUM is 1 so this is an annoying calculation.  So what
this does is it changes the type from unsigned long to u8 and renames
the variable.  I am fine with renaming the variable it's unrelated and
makes the review harder.

> +	u8 cmd = 0xB8;
>  	int i, acc = 0;
> +	int ret;
>  
> -	for (i = 0; i < 63; i++) {
> +	for (i = 0; i < GAMMA_LEN; i++) {

GAMMA_LEN is 63.  So this looks like a change, but it's an unrelated
cleanup.

>  		if (i > 0 && curves[i] < 2) {
>  			dev_err(par->info->device,
>  				"Illegal value in Grayscale Lookup Table at index %d : %d. Must be greater than 1\n",
>  				i, curves[i]);
>  			return -EINVAL;
>  		}
> +

This is an unrelated white space change.

>  		acc += curves[i];
> -		tmp[i] = acc;
> +
>  		if (acc > 180) {
>  			dev_err(par->info->device,
>  				"Illegal value(s) in Grayscale Lookup Table. At index=%d : %d, the accumulated value has exceeded 180\n",
>  				i, acc);
>  			return -EINVAL;
>  		}
> +
> +		data[i] = acc;

Here we move the acc assignment after the sanity check, but it's just
an unrelated cleanup.

>  	}
>  
> -	write_reg(par, 0xB8,
> -		  tmp[0],  tmp[1],  tmp[2],  tmp[3],
> -		  tmp[4],  tmp[5],  tmp[6],  tmp[7],
> -		  tmp[8],  tmp[9],  tmp[10], tmp[11],
> -		  tmp[12], tmp[13], tmp[14], tmp[15],
> -		  tmp[16], tmp[17], tmp[18], tmp[19],
> -		  tmp[20], tmp[21], tmp[22], tmp[23],
> -		  tmp[24], tmp[25], tmp[26], tmp[27],
> -		  tmp[28], tmp[29], tmp[30], tmp[31],
> -		  tmp[32], tmp[33], tmp[34], tmp[35],
> -		  tmp[36], tmp[37], tmp[38], tmp[39],
> -		  tmp[40], tmp[41], tmp[42], tmp[43],
> -		  tmp[44], tmp[45], tmp[46], tmp[47],
> -		  tmp[48], tmp[49], tmp[50], tmp[51],
> -		  tmp[52], tmp[53], tmp[54], tmp[55],
> -		  tmp[56], tmp[57], tmp[58], tmp[59],
> -		  tmp[60], tmp[61], tmp[62]);
> +	ret = fbtft_write_buf_dc(par, &cmd, 1, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = fbtft_write_buf_dc(par, data, sizeof(data), 1);
> +	if (ret < 0)
> +		return ret;

These are good changes.  Just change the type from unsigned long to u8
and use fbtft_write_buf_dc() instead of write_reg().  Then do the other
changes in a separate patch.

Same for the other patches.

regards,
dan carpenter

>  
>  	return 0;
>  }
> -- 
> 2.43.0
>