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@xxxxxxxxx>
> ---
> 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
