[PATCH 3/3] drm: tiny: st7735r: Add support for Okaya RH128128T

[Geert Uytterhoeven]

Hi David,

On Mon, Jan 6, 2020 at 6:12 PM David Lechner <david at lechnology.com> wrote:
> On 1/6/20 3:28 AM, Geert Uytterhoeven wrote:
> > On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam at ravnborg.org> wrote:
> >> On Thu, Jan 02, 2020 at 03:12:46PM +0100, Geert Uytterhoeven wrote:
> >>> Add support for the Okaya RH128128T display to the st7735r driver.
> >>>
> >>> The RH128128T is a 128x128 1.44" TFT display driven by a Sitronix
> >>> ST7715R TFT Controller/Driver.  The latter is very similar to the
> >>> ST7735R, and can be handled by the existing st7735r driver.

> >>> --- a/drivers/gpu/drm/tiny/st7735r.c
> >>> +++ b/drivers/gpu/drm/tiny/st7735r.c

> >>> @@ -37,12 +39,28 @@
> >>>   #define ST7735R_MY   BIT(7)
> >>>   #define ST7735R_MX   BIT(6)
> >>>   #define ST7735R_MV   BIT(5)
> >>> +#define ST7735R_RGB  BIT(3)
> >>> +
> >>> +struct st7735r_cfg {
> >>> +     const struct drm_display_mode mode;
> >>> +     unsigned int left_offset;
> >>> +     unsigned int top_offset;
> >>> +     unsigned int write_only:1;
> >>> +     unsigned int rgb:1;             /* RGB (vs. BGR) */
> >>> +};
> >>> +
> >>> +struct st7735r_priv {
> >>> +     struct mipi_dbi_dev dbidev;     /* Must be first for .release() */
> >>> +     unsigned int rgb:1;
> >>> +};
> >>
> >> The structs here uses "st7735r" as the generic prefix.
> >> But the rest of this file uses "jd_t18003_t01" as the generic prefix.
> >>
> >> It would help readability if the same prefix is used for the common
> >> stuff everywhere.
> >
> > Agreed.
> > So I think it makes most sense to rename jd_t18003_t01_pipe_{enable,funcs}
> > to sh7735r_pipe_{enable,funcs}?
> > If needed, the display-specific parts (e.g. gamma parameters) could be
> > factored out in st7735r_cfg later, if neeeded.
>
> IIRC, the original intention here is that functions/structs with the
> jd_t18003_t01_ prefix are specific to the panel, not the controller.

Makes sense.

> E.g. things like power settings and gamma curves.
>
> The idea is that it is much easier to write and understand the init sequence
> as a function rather than trying to make a generic function that can parse
> a any possible init sequence from a data structure.

I believe the init sequence is the same.  The init parameters may not be.
What happened to the separation of code and data? ;-)

> This new panel really has all of the same settings as the existing one?

I went through all the ST77[13]5R-specific register settings in the pipe
enable function. All these registers exist on both ST7715R and ST7735R.
Unfortunately the Okaya display documentation doesn't give any clues
w.r.t. the expected values to program.
However, my display seems to work fine.  Even the grayscale bands from
fbtest test006 look good, so the gamma parameters must be correct ;-)

> Having a separate pipe enable function for the new panel would also eliminate
> the need for the extra private rgb data.

At the expense of duplicating the whole function, for one single bit of
difference...

P.S. Note that I'm using this on an RSK+RZA1 board with 32 MiB of SDRAM.
Enabling support for this display increases kernel size by 316 KiB.
And apparently most real world users of the RZ/A1 SoC are not even using
SDRAM, but doing XIP with the builtin 10 MiB of SRAM...

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds