Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: David Lechner <david@lechnology.com>
Cc: "Sam Ravnborg" <sam@ravnborg.org>,
	"Noralf Trønnes" <noralf@tronnes.org>,
	"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: Tue, 7 Jan 2020 13:46:47 +0100
Message-ID: <CAMuHMdXxwSR6D_tzg9iqwwLYEJZg6DEonvvvC_TigtG0PAvn+w@mail.gmail.com> (raw)
In-Reply-To: <893b059c-fd7d-3c21-384c-da33af595ffb@lechnology.com>

Hi David,

On Mon, Jan 6, 2020 at 6:12 PM David Lechner <david@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@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@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

      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
2020-01-07 12:00         ` Geert Uytterhoeven
2020-01-06 17:12       ` David Lechner
2020-01-07 12:46         ` Geert Uytterhoeven [this message]

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=CAMuHMdXxwSR6D_tzg9iqwwLYEJZg6DEonvvvC_TigtG0PAvn+w@mail.gmail.com \
    --to=geert@linux-m68k.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=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 \
    --cc=sam@ravnborg.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