Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Sam Ravnborg <sam@ravnborg.org>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: "Geert Uytterhoeven" <geert+renesas@glider.be>,
	"Noralf Trønnes" <noralf@tronnes.org>,
	"David Lechner" <david@lechnology.com>,
	"David Airlie" <airlied@linux.ie>,
	"Daniel Vetter" <daniel@ffwll.ch>,
	"Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	"Chris Brandt" <chris.brandt@renesas.com>,
	"DRI Development" <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] drm: tiny: st7735r: Add support for Okaya RH128128T
Date: Mon, 6 Jan 2020 18:08:08 +0100
Message-ID: <20200106170808.GA21745@ravnborg.org> (raw)
In-Reply-To: <CAMuHMdUL3tCZzCDyJkmqYT5n+-t+Z-Ubo4=+NJpHpZU1w5C07g@mail.gmail.com>

Hi Geert.

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

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.

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

> > > @@ -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/
Yeah, or maybe just sitronix_pipe_{enable,funcs} as we have support
for more than one Sitronix controller anyway.

> If needed, the display-specific parts (e.g. gamma parameters) could be
> factored out in st7735r_cfg later, if neeeded.
> 
> 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.
Agreed, let's bite that when we have a display we care enough about
and maybe then we also can write a proper driver for it.
I have a few displays here using 8080 and I hope someone steps up
to do proper support for such displays.

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

Thanks for the explanation.

	Sam

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-02 14:12 [PATCH 0/3] drm: " Geert Uytterhoeven
2020-01-02 14:12 ` [PATCH 1/3] dt-bindings: display: sitronix,st7735r: Add Okaya rh128128t Geert Uytterhoeven
2020-01-02 14:46   ` [PATCH 1/3] dt-bindings: display: sitronix, st7735r: " Sam Ravnborg
2020-01-06 16:47     ` David Lechner
2020-01-02 14:12 ` [PATCH 2/3] drm/mipi_dbi: Add support for display offsets Geert Uytterhoeven
2020-01-05  8:46   ` Sam Ravnborg
2020-01-02 14:12 ` [PATCH 3/3] drm: tiny: st7735r: Add support for Okaya RH128128T Geert Uytterhoeven
2020-01-05  9:13   ` Sam Ravnborg
2020-01-06  9:28     ` Geert Uytterhoeven
2020-01-06 17:08       ` Sam Ravnborg [this message]
2020-01-07 12:00         ` Geert Uytterhoeven
2020-01-06 17:12       ` David Lechner
2020-01-07 12:46         ` Geert Uytterhoeven

Reply instructions:

You may reply publically 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=20200106170808.GA21745@ravnborg.org \
    --to=sam@ravnborg.org \
    --cc=airlied@linux.ie \
    --cc=chris.brandt@renesas.com \
    --cc=daniel@ffwll.ch \
    --cc=david@lechnology.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=geert+renesas@glider.be \
    --cc=geert@linux-m68k.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=maarten.lankhorst@linux.intel.com \
    --cc=mark.rutland@arm.com \
    --cc=mripard@kernel.org \
    --cc=noralf@tronnes.org \
    --cc=robh+dt@kernel.org \
    /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

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org
	public-inbox-index linux-renesas-soc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git