Hi Sam,
On Sun, Jan 5, 2020 at 10:13 AM Sam Ravnborg <sam@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx
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
