Hi Linus,
On 25.10.24 18:36, Linus Walleij wrote:
...
On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@xxxxxxxxxxxxxx> 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
