[PATCH v2] staging: fbtft: fb_st7789v: support setting offset

[Nam Cao]

On 10/Apr/2024 Yuguo Pei wrote:
> Some screen sizes using st7789v chips are different from 240x320,
> and offsets need to be set to display all images properly.
> 
> For those who use screens with offset, they only need to modify the values
> of size and offset, and do not need to a new set_addr_win function.

If I understand the patch correctly, you are adding a new feature so that
people can change the screen offset? And from the patch, I think users
are supposed change the values of macros LEFT_OFFSET and TOP_OFFSET?

I hope I don't misunderstand anything, because I would be against this
approach. Asking users to modify the source code doesn't sound like a
good idea. If this is really needed, I suggest adding new device tree
properties instead.

> Signed-off-by: Yuguo Pei <purofle at gmail.com>
> ---
> v2: modify Signed-off-by, fix explanation of changes
> 
>  drivers/staging/fbtft/fb_st7789v.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/staging/fbtft/fb_st7789v.c b/drivers/staging/fbtft/fb_st7789v.c
> index 861a154144e6..d47ab4262374 100644
> --- a/drivers/staging/fbtft/fb_st7789v.c
> +++ b/drivers/staging/fbtft/fb_st7789v.c
> @@ -30,6 +30,12 @@
>  
>  #define HSD20_IPS 1
>  
> +#define WIDTH 240
> +#define HEIGHT 320
> +
> +#define LEFT_OFFSET 0
> +#define TOP_OFFSET 0
> +
>  /**
>   * enum st7789v_command - ST7789V display controller commands
>   *
> @@ -349,6 +355,21 @@ static int set_gamma(struct fbtft_par *par, u32 *curves)
>  	return 0;
>  }
>  
> +static void set_addr_win(struct fbtft_par *par, int xs, int ys, int xe, int ye)
> +{
> +	unsigned int x = xs + TOP_OFFSET, y = xe + TOP_OFFSET;
> +
> +	write_reg(par, MIPI_DCS_SET_COLUMN_ADDRESS, (x >> 8) & 0xFF, xs & 0xFF,
                                                                     ^ should be x?
> +		  (y >> 8) & 0xFF, xe & 0xFF);
                                   ^ should be y?

As noted above, I don't think this is correct. The spec says this register should
be written with:
	- upper 8 bit of SC
	- lower 8 bit of SC
	- upper 8 bit of EC
	- lower 8 bit of EC
...and I don't think the code does that correctly.

> +	x = ys + LEFT_OFFSET, y = ye + LEFT_OFFSET;
> +
> +	write_reg(par, MIPI_DCS_SET_PAGE_ADDRESS, (x >> 8) & 0xFF, ys & 0xFF,
> +		  (y >> 8) & 0xFF, ye & 0xFF);

Same problem as above?

> +	write_reg(par, MIPI_DCS_WRITE_MEMORY_START);
> +}
> +
>  /**
>   * blank() - blank the display
>   *
> @@ -379,6 +400,7 @@ static struct fbtft_display display = {
>  		.set_var = set_var,
>  		.set_gamma = set_gamma,
>  		.blank = blank,
> +		.set_addr_win = set_addr_win,
>  	},
>  };
>

Because I don't think the implementation is correct, as pointed out above,
I have to ask: has this patch been tested with hardware? Is there
really a use case here? I wouldn't like to add code that is not really
used..

Best regards,
Nam