diff mbox series

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

Message ID 20240409180900.31347-2-purofle@gmail.com (mailing list archive)
State Handled Elsewhere
Headers show
Series [v2] staging: fbtft: fb_st7789v: support setting offset | expand

Commit Message

Yuguo Pei April 9, 2024, 6:09 p.m. UTC
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.

Signed-off-by: Yuguo Pei <purofle@gmail.com>
---
v2: modify Signed-off-by, fix explanation of changes

 drivers/staging/fbtft/fb_st7789v.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Nam Cao April 9, 2024, 7:21 p.m. UTC | #1
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@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
diff mbox series

Patch

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,
+		  (y >> 8) & 0xFF, xe & 0xFF);
+
+	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);
+
+	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,
 	},
 };