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

[Geert Uytterhoeven]

Hi Sam,

On Mon, Jan 6, 2020 at 6:08 PM 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.
> > >
> > > 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.
>
> The good thing with a split is that is shows how little is really
> required to support a new controller/panel pair.
> So it would be good if you did so.

OK.

> > > > --- 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?
>
> If we do describe it we should do so as kernel-doc and then wire up the
> driver somwhere under Documentation/gpu/
> But there is no good place to wire it up yet.

Documentation/devicetree/bindings/display/sitronix,st7735r.yaml? ;-)

> > > > @@ -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}?
> s/sh/st/

Oops, seen to much SuperH-derived hardware ;-)

> Yeah, or maybe just sitronix_pipe_{enable,funcs} as we have support
> for more than one Sitronix controller anyway.

Note that there are other drivers for Sitronix controllers, e.g.
drivers/gpu/drm/tiny/st7586.c, which is a different beast.
So st7735r_* sounds better to me ('15 and '35 are very similar).

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