* [PATCH v2] staging: fbtft: fb_st7789v: support setting offset
@ 2024-04-09 18:09 Yuguo Pei
2024-04-09 19:21 ` Nam Cao
0 siblings, 1 reply; 2+ messages in thread
From: Yuguo Pei @ 2024-04-09 18:09 UTC (permalink / raw
To: gregkh; +Cc: dri-devel, linux-fbdev, linux-staging, purofle
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(+)
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,
},
};
--
2.44.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] staging: fbtft: fb_st7789v: support setting offset
2024-04-09 18:09 [PATCH v2] staging: fbtft: fb_st7789v: support setting offset Yuguo Pei
@ 2024-04-09 19:21 ` Nam Cao
0 siblings, 0 replies; 2+ messages in thread
From: Nam Cao @ 2024-04-09 19:21 UTC (permalink / raw
To: Yuguo Pei; +Cc: gregkh, dri-devel, linux-fbdev, linux-staging
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-04-09 19:21 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-09 18:09 [PATCH v2] staging: fbtft: fb_st7789v: support setting offset Yuguo Pei
2024-04-09 19:21 ` Nam Cao
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).