From: Nam Cao <namcao@linutronix.de>
To: Yuguo Pei <purofle@gmail.com>
Cc: gregkh@linuxfoundation.org, dri-devel@lists.freedesktop.org,
linux-fbdev@vger.kernel.org, linux-staging@lists.linux.dev
Subject: Re: [PATCH v2] staging: fbtft: fb_st7789v: support setting offset
Date: Tue, 9 Apr 2024 21:21:35 +0200 [thread overview]
Message-ID: <20240409212135.72cbac3a@namcao> (raw)
In-Reply-To: <20240409180900.31347-2-purofle@gmail.com>
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
prev parent reply other threads:[~2024-04-09 19:21 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-09 18:09 [PATCH v2] staging: fbtft: fb_st7789v: support setting offset Yuguo Pei
2024-04-09 19:21 ` Nam Cao [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240409212135.72cbac3a@namcao \
--to=namcao@linutronix.de \
--cc=dri-devel@lists.freedesktop.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=purofle@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).