| 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 |
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 --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, }, };
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(+)