LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Sam Ravnborg <sam@ravnborg.org>
Cc: Alexey Minnekhanov <alexeymin@postmarketos.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	open list <linux-kernel@vger.kernel.org>,
	"open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
	<devicetree@vger.kernel.org>, Hans de Goede <hdegoede@redhat.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>," 
	<~postmarketos/upstreaming@lists.sr.ht>,
	phone-devel@vger.kernel.org
Subject: Re: [PATCH 2/2] drm/panel: Add Samsung S6E3FA2 DSI panel driver
Date: Mon, 26 Jul 2021 09:44:33 +0200	[thread overview]
Message-ID: <CACRpkdYbL54XeWWipgn8yiQRuQOL+VkdHumkLZv6DrkUgdCjUg@mail.gmail.com> (raw)
In-Reply-To: <YP184rqayPLbWLx4@ravnborg.org>

 On Sun, Jul 25, 2021 at 5:01 PM Sam Ravnborg <sam@ravnborg.org> wrote:

> This driver supports two different panels:
>
>         S6E3FA2
>         EA8064G
>
> They differ on a lot of the tables and requires different init.
> In other words there is only a little boiler plate code that is in
> common.
>
> I think it would be much cleaner with individual drivers for each panel.

Sometimes Samsung have different *physical* panels connected
to the same display controller, but I don't know what is the case
here. This looks like it could actually be two different display
controllers. (I don't like these opaque binary drops from Samsung,
datasheets would be nice...)

What I think is most intuitive is to have one driver per display controller.
If the two drivers are writing some very similar registers with very
similar values they are probably the same display controller.

If they are not then they are not...

If they are obviously the same display controller I think parameterizing
a display controller driver along the line of panel-novatek-nt35510.c
is the best. If different display controllers, we need different drivers.

> Which brings me to next topic - this is two different panels and the DT
> are supports to describe the HW - so the DT tree should have different
> entries depending on the actual panel. As it is now you try to hide the
> fact that one compatible describes two different panels.

(...)
> > +     ret = s6e3fa2_dsi_dcs_read1(dsi, MCS_READ_ID1, &id1);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = s6e3fa2_dsi_dcs_read1(dsi, MCS_READ_ID2, &id2);
> > +     if (ret < 0)
> > +             return ret;
> > +     ret = s6e3fa2_dsi_dcs_read1(dsi, MCS_READ_ID3, &id3);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     lcd_id = id1 << 16 | id2 << 8 | id3;
> > +
> > +     switch (lcd_id) {
> > +     case LCD_ID_S6E3FA2:
> > +             dev_info(&dsi->dev, "detected S6E3FA2 panel (ID: 0x%x)\n",
> > +                      lcd_id);
> > +             ctx->subtype = PANEL_S6E3FA2;
> > +             ctx->seq_data = &seqdata_s6e3fa2;
> > +             break;
> > +     case LCD_ID_EA8064G:
> > +             dev_info(&dsi->dev, "detected EA8064G panel (ID: 0x%x)\n",
> > +                      lcd_id);
> > +             ctx->subtype = PANEL_EA8064G;
> > +             ctx->seq_data = &seqdata_ea8064g;
> > +             break;
> > +     default:
> > +             dev_warn(&dsi->dev, "unsupported panel ID: 0x%x\n", lcd_id);
> > +             ctx->subtype = PANEL_UNKNOWN;

This does look like two different panels, I'd like to know the MTP
IDs printed (also wrote in different mail). The MTP print I think
should be kept.

Yours,
Linus Walleij

  parent reply	other threads:[~2021-07-26  7:44 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-25 14:03 [PATCH 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Alexey Minnekhanov
2021-07-25 14:03 ` [PATCH 2/2] drm/panel: Add Samsung S6E3FA2 DSI panel driver Alexey Minnekhanov
     [not found]   ` <YP184rqayPLbWLx4@ravnborg.org>
2021-07-26  7:44     ` Linus Walleij [this message]
2021-07-26  8:04   ` Linus Walleij
2021-07-26  7:51 ` [PATCH 1/2] dt-bindings: panel: Add Samsung S6E3FA2 panel Linus Walleij
2021-07-29 22:30   ` Rob Herring
2021-07-29 22:31 ` Rob Herring

Reply instructions:

You may reply publicly 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=CACRpkdYbL54XeWWipgn8yiQRuQOL+VkdHumkLZv6DrkUgdCjUg@mail.gmail.com \
    --to=linus.walleij@linaro.org \
    --cc=airlied@linux.ie \
    --cc=alexeymin@postmarketos.org \
    --cc=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=phone-devel@vger.kernel.org \
    --cc=sam@ravnborg.org \
    --cc=thierry.reding@gmail.com \
    --cc=~postmarketos/upstreaming@lists.sr.ht \
    --subject='Re: [PATCH 2/2] drm/panel: Add Samsung S6E3FA2 DSI panel driver' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).