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

[Geert Uytterhoeven]

Hi Sam,

On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam at ravnborg.org> wrote:
> Good to see we add more functionality to the smallest driver in DRM.
> The patch triggered a few comments - see below.
> Some comments relates to the original driver - and not your changes.

Thanks for your comments!

> 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.
>
> As a general comment - it would have eased review if this was split
> in two patches.
> One patch to introduce the infrastructure to deal with another set of
> controller/display and one patch introducing the new combination.

I had thought about that, but didn't pursue as the new combination is
just 7 added lines.  If you prefer a split, I can do that.

> > --- a/drivers/gpu/drm/tiny/st7735r.c
> > +++ b/drivers/gpu/drm/tiny/st7735r.c
> > @@ -1,8 +1,9 @@
> >  // SPDX-License-Identifier: GPL-2.0+
> >  /*
> > - * DRM driver for Sitronix ST7735R panels
> > + * DRM driver for Sitronix ST7715R/ST7735R panels
>
> This comment could describe the situation a little better.
> This is a sitronix st7735r controller with a jianda jd-t18003-t01
> display.
> Or a sitronix st7715r controller with a okaya rh128128t display.

Indeed. It is currently limited to two controller/display combos.
But I expect more combos to be added over time.
Hence does it make sense to describe all of that in the top comments?

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

> struct st7735r_priv includes "rgb" which is copied from struct
> st7735r_cfg.
> Maybe just add a const pointer to struct st7735r_cfg,
> so when we later add more configuration items we do not need to have two
> copies. And then ofc drop st7735r_priv.rgb.

I thought about that, but didn't do so, as the rgb field is the only
parameter used outside the probe function.  Can be changed, of course.

> > @@ -136,13 +167,14 @@ static struct drm_driver st7735r_driver = {
> >  };
> >
> >  static const struct of_device_id st7735r_of_match[] = {
> > -     { .compatible = "jianda,jd-t18003-t01" },
> > +     { .compatible = "jianda,jd-t18003-t01", .data = &jd_t18003_t01_cfg },
> > +     { .compatible = "okaya,rh128128t", .data = &rh128128t_cfg },
> >       { },
> { /* sentinel },
>
> Also - which is not a new thing - this fails to check that we have the
> correct combination of two compatibles.
> From the binding:
>
>     Must be one of the following combinations:
>     - "jianda,jd-t18003-t01", "sitronix,st7735r"
>     - "okaya,rh128128t", "sitronix,st7715r"

That will be checked by "make dtbs_check", once I have converted the DT
bindings to yaml ;-)

In reality, there are lots of different ways to communicate with an
ST77[13]5R display controller (3/4-wire SPI, or 6800/8080 bus, ...), and
lots of different ways to wire a display to the controller.  Ideally,
this should be described in DT.  As I don't have schematics for the
display board, doing so is difficult, and will miss important details,
which may lead to DTB ABI compatibility issues later.
I understand using these compatible combinations was a pragmatic solution
to this problem.

> >  };
> >  MODULE_DEVICE_TABLE(of, st7735r_of_match);
> >
> >  static const struct spi_device_id st7735r_id[] = {
> > -     { "jd-t18003-t01", 0 },
> > +     { "jd-t18003-t01", (uintptr_t)&jd_t18003_t01_cfg },
> >       { },
> { /* sentinel */ },
>
> Do we need an entry for "okaya,rh128128t" here?
>
> Note: I have not fully understood how MODULE_DEVICE_TABLE()
> works - so forgive me my ignorance.

st7735r_id[] is used for matching based on platform device name.
Hence an entry is needed only to use the display on non-DT systems.
Can be added later, if ever needed.

Note that there is a separate MODULE_DEVICE_TABLE() for DT-based matching,
so the driver module will be auto-loaded on DT-based systems.

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