LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Linus Walleij <linus.walleij@linaro.org>
To: Alexey Minnekhanov <alexeymin@postmarketos.org>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Sam Ravnborg <sam@ravnborg.org>, 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 10:04:28 +0200	[thread overview]
Message-ID: <CACRpkdbpxQdyTVKkKGCtjLGn5G9L2=w-dhyMpgJP42_tpLU6Pg@mail.gmail.com> (raw)
In-Reply-To: <20210725140339.2465677-2-alexeymin@postmarketos.org>

Hi Alexey,

I had some gmail problems and replied to the very old driver
by Iskren, sorry for the mess.

I overall like this driver a lot. Some of Sam's comments could
be addressed especially for backlight.

I think the driver should indeed handle both the physical
displays like you do here.

On Sun, Jul 25, 2021 at 4:05 PM Alexey Minnekhanov
<alexeymin@postmarketos.org> wrote:

> Samsung S6E3FA2 panel is amoled 1080x1920 command mode DSI
> panel used in Samsung Galaxy S5 phone. There are 2 known
> variations of panel that were shipped in this phone, and
> this driver handles both of them.
>
> Panel has built-in backlight (like all other AMOLED panels),
> controlled over DSI by some vendor specific commands, some
> of them include sending long byte sequences of what seems
> to be called "smart dimming".
>
> Signed-off-by: Alexey Minnekhanov <alexeymin@postmarketos.org>

(...)

> +#define dsi_generic_write_seq(dsi, seq...) do {                                \
> +               static const u8 d[] = { seq };                          \
> +               int ret;                                                \
> +               ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));    \
> +               if (ret < 0)                                            \
> +                       return ret;                                     \
> +       } while (0)
> +
> +#define dsi_dcs_write_seq(dsi, seq...) do {                            \
> +               static const u8 d[] = { seq };                          \
> +               int ret;                                                \
> +               ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \
> +               if (ret < 0)                                            \
> +                       return ret;                                     \
> +       } while (0)

These look generic as pointed out in other mail.

> +static int s6e3fa2_dsi_dcs_read1(struct mipi_dsi_device *dsi, const u8 cmd,
> +                               u8 *data)
> +{
> +       int ret;
> +
> +       ret = mipi_dsi_dcs_read(dsi, cmd, data, 1);
> +       if (ret < 0) {
> +               dev_err(&dsi->dev, "could not read DCS CMD %02x\n", cmd);
> +               return ret;
> +       }
> +       return 0;
> +}

I don't think this needs a wrapper, just call mipi_dsi_dcs_read() directly.

> +/* Panel variants */
> +#define LCD_ID_S6E3FA2         0x602813
> +#define LCD_ID_EA8064G         0x622872

Interesting use of the "vendor" byte by Samsung here. It seems they are
repurposing the non-standard MTP bytes as they seem fit.

> +/*
> + * Which AID sequence to use for each candela level.
> + * This lookup table is same for both panels.
> + */
> +static const u8 map_candela_to_aid[S6E3FA2_NUM_GAMMA_LEVELS] = {
> +        0,  2,  3,  4,  6,  7,  8, 10, 11, 13, 14, 15,
> +       16, 17, 18, 20, 21, 22, 23, 24, 25, 26, 27, 28,
> +       29, 30, 31, 32, 33, 34, 35, 36, 36, 36, 36, 36,
> +       36, 36, 36, 36, 36, 36, 36, 36, 36, 36, 37, 38,
> +       39, 40, 41, 42, 43, 44, 44, 44, 44, 44, 44, 44
> +};

This and other things hints that we are dealing with the same display
controller.

> +/* Other panel drivers call these commands test_key_enable/disable */
> +static const u8 seq_s6e3fa2_test_key_en[6] = {
> +       0xf0, 0x5a, 0x5a,
> +       0xfc, 0x5a, 0x5a
> +};

0xf0 and 0xfc is obviously some "level 2 unlock" commands.
Maybe #define them as pointed out in other comments.

> +static const u8 seq_s6e3fa2_test_key_dis[6] = {
> +       0xf0, 0xa5, 0xa5,
> +       0xfc, 0xa5, 0xa5
> +};
> +static const u8 seq_ea8064g_test_key_en[6] = {
> +       0xf0, 0x5a, 0x5a,
> +       0xf1, 0x5a, 0x5a
> +};
> +static const u8 seq_ea8064g_test_key_dis[6] = {
> +       0xf1, 0xa5, 0xa5,
> +       0xf0, 0xa5, 0xa5
> +};

The use of two different registers for locking is suspicious, that
may point to different display controllers. :/

This is an icky panel, but it seems they are close enough to
be handled by the same driver IMO.

Yours,
Linus Walleij

  parent reply	other threads:[~2021-07-26  8:04 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
2021-07-26  8:04   ` Linus Walleij [this message]
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='CACRpkdbpxQdyTVKkKGCtjLGn5G9L2=w-dhyMpgJP42_tpLU6Pg@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).