Message-ID: <431e2e98-7c0b-47e6-8949-aa5e0a6ac8f1@rocketmail.com>
Date: Sat, 26 Oct 2024 10:51:35 +0200
From: Jakob Hauser <jahau@...ketmail.com>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>,
 Jessica Zhang <quic_jesszhan@...cinc.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Thierry Reding <thierry.reding@...il.com>,
 Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
 Andrzej Hajda <andrzej.hajda@...el.com>, dri-devel@...ts.freedesktop.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH v3 3/5] drm/panel: samsung-s6e88a0-ams427ap24: Add initial
 driver

Hi Linus,

On 25.10.24 18:36, Linus Walleij wrote:
...
> On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@...ketmail.com> wrote:
...
> 
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
> 
> Why do you need this include? .of_match_table is part of
> <linux/driver.h>

You're right, I'll remove it.

>> +static int s6e88a0_ams427ap24_on(struct s6e88a0_ams427ap24 *ctx)
>> +{
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>> +
>> +       dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a);
> 
> Can we provide #defines for at least some of this magic?
> See other drivers for a very good idea of what some of them mean.
> panel-samsung-s6d27a1.c:#define S6D27A1_PASSWD_L2       0xF0    /*
> Password Command for Level 2 Control */
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD1             0xf0
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a);
> 
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD3             0xfc
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30);
> (...)
>> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>> +       mipi_dsi_msleep(&dsi_ctx, 20);
>> +
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a);
> 
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD2             0xf1
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5);
> 
> panel-samsung-s6d7aa0.c:#define MCS_PASSWD2             0xf1
> Send in the reverse password: disable access.
> 
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
>> +                                    0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
>> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
>> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
>> +                                    0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
>> +                                    0x80, 0x80, 0x00, 0x00, 0x00);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
>> +                                    0x40, 0x08, 0x20, 0x00, 0x08);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5);
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5);
> 
> Send in the reverse password: disable access.
> 
> A bit of #defines and comments would make it much more clear what
> is going on.

The difficulty I'm in is that I don't have a datasheet.

 From the Android kernel downstream driver data ams427ap24 [1], from the similar 
but older panel ams427ap01 downstream driver [2] and the similar panel 
ams452ef01 upstream driver [3] I can guess what the commands do... except I 
couldn't find out what "0xf2, 0x03, 0x0d" does and for "0xf1, 0x5a, 0x5a" I was 
guessing level 3 key on because level 1 and 2 was already there.

[1] 
https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/drivers/video/msm/mdss/samsung/S6E88A0_AMS427AP24/dsi_panel_S6E88A0_AMS427AP24_qhd_octa_video.dtsi
[2] 
https://github.com/LineageOS/android_kernel_samsung_msm8930-common/blob/lineage-15.1/drivers/video/msm/mipi_samsung_oled_video_qhd_pt-8930.c
[3] 
https://github.com/torvalds/linux/blob/v6.11/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c

That said, I can offer to comment the single command lines like this:

mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0x5a, 0x5a); // level 1 key on
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0x5a, 0x5a); // level 2 key on
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x11); // src latch set global 1
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x11); // src latch set 1
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x13); // src latch set global 2
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfd, 0x18); // src latch set 2
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb0, 0x02); // avdd set 1
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb8, 0x30); // avdd set 2

mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
mipi_dsi_msleep(&dsi_ctx, 20);

mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0x5a, 0x5a); // level 3 key on
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xcc, 0x4c); // pixel clock divider pol.
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf2, 0x03, 0x0d); // unknown
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf1, 0xa5, 0xa5); // level 3 key off
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xca,
                              0x01, 0x00, 0x01, 0x00, 0x01, 0x00, 0x80,
                              0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
                              0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
                              0x80, 0x80, 0x80, 0x80, 0x80, 0x80, 0x80,
                              0x80, 0x80, 0x00, 0x00, 0x00); // set gamma
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb2,
                              0x40, 0x08, 0x20, 0x00, 0x08); // set aid
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xb6, 0x28, 0x0b); // set elvss
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf7, 0x03); // gamma update
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00); // acl off
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xf0, 0xa5, 0xa5); // level 1 key off
mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xfc, 0xa5, 0xa5); // level 2 key off

Some of those lines exceed the 80 characters limit. I would still prefer not to 
break those lines to keep readability.

Now commenting the single command lines is one thing. But giving names to the 
command registers is something different. E.g. in 0xb0 there is "src latch set 
global" but also one of the "avdd". I wouldn't know what name that command 
register should have. For 0xcc and 0xf2 in the downstream ams427ap24 they're 
both in a block labelled "PCD" and in upstream ams452ef01 the 0xcc line is 
commented as "set Pixel Clock Divider polarity" but the 0xf2 register seems to 
be more general because in downstream ams427ap24 it also shows up in a block 
called "AVC" and in downstream ams427ap01 in a command labelled "vporch". 
Guessing names for the command registers will become too arbitrary.

Additionally, I actually would like to avoid the #defines for the command 
registers because this will definitively break the 80 character lines limits 
strong enough, splintering the above command blocks into quite some mess.

Kind regards,
Jakob

Powered by blists - more mailing lists