Re: [PATCH v3 4/5] drm/panel: samsung-s6e88a0-ams427ap24: Add brightness control

[Jakob Hauser <jahau@rocketmail.com>]

Hi Linus,

On 25.10.24 21:27, Linus Walleij wrote:
...
> On Thu, Oct 24, 2024 at 5:18 AM Jakob Hauser <jahau@rocketmail.com> wrote:
>> +static const int s6e88a0_ams427ap24_br_to_cd[NUM_STEPS_CANDELA] = {
> 
> (...)
>> +       /* brightness till, candela */
> 
> Brightness to candela conversion table? Edit comment?

In the downstream driver there is a table with four columns:
<idx> <from> <till> <candella>.

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#L341-L397

The first column is a counter, the second and third is the 
from-till-range of brightness steps that correspond to the forth column 
of candela identifier.

In the patch here I only adopted the third and forth column, because the 
others were not necessary. The comment "brightness till, candela" was 
intended to label those two columns.

To make it more clear, I could add the keyword "columns" and the column 
"brightness from".

         /* columns: brightness from, brightness till, candela */
         /* 0 */    10,  /* 10CD */
         /* 11 */   11,  /* 11CD */
         /* 12 */   12,  /* 12CD */
         /* 13 */   13,  /* 13CD */
         /* 14 */   14,  /* 14CD */

         ...

         /* 30 */   30,  /* 39CD */
         /* 31 */   32,  /* 41CD */
         /* 33 */   34,  /* 44CD */
         /* 35 */   36,  /* 47CD */

         ...

         /* 92 */   97,  /* 126CD */
         /* 98 */   104, /* 134CD */
         /* 105 */  110, /* 143CD */
         /* 111 */  118, /* 152CD */

         ...

         /* 182 */  205, /* 249CD */
         /* 206 */  234, /* 265CD */
         /* 235 */  254, /* 282CD */
         /* 255 */  255, /* 300CD */

>> +static const u8 s6e88a0_ams427ap24_aid[NUM_STEPS_AID][SEQ_LENGTH_AID] = {
> 
> If you know that the sequence 0xb2, 0x40, 0x08, 0x20 means "set AID"
> (or is it AOR??) you can #define
> 
> #define S6E88A0_SET_AID 0xb2

Thanks to Alexey Min, who looked this up, I can say:

"The PWM mechanism used on Samsung displays is called AOR (AMOLED off 
ratio), the related function on the kernel driver is called AID (AMOLED 
impulsive driving)."

source: xdaforums

The downstream driver of ams427ap24 uses the "aid" labeling, therefore I 
stick to that. (Interestingly the older downstream driver ams427ap01 
uses the "aor" labeling.)

> Then make a small buffer:
> 
> u8 set_aid[5] = { S6E88A0_SET_AID, 0x40, 0x08, 0x20, 0x00, 0x00 };
> 
> then you can strip the first three bytes from the entire table,
> just copy in the two relevant bytes into set_aor[]
> and send that.

Ok, I'll try to implement that. The size of the second array dimension 
of that table will then become [SEQ_LENGTH_AID - 3].

...

>> +static int s6e88a0_ams427ap24_set_brightness(struct backlight_device *bd)
>> +{
>> +       struct s6e88a0_ams427ap24 *ctx = bl_get_data(bd);
>> +       struct mipi_dsi_device *dsi = ctx->dsi;
>> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>> +       struct device *dev = &dsi->dev;
>> +       int brightness = bd->props.brightness;
>> +       int candela_enum;
>> +       u8 b2[SEQ_LENGTH_AID + 1];
>> +       u8 b6[SEQ_LENGTH_ELVSS + 1];
>> +       u8 ca[SEQ_LENGTH_GAMMA + 1];
> 
> Rename them to something like my suggestions so we understand what it is
> all about. It seems the infrastructure for what I suggested is mostly already
> there.

These defines are intended to be the sequence length of the payload for 
commands aid, elvss and gamma. The naming makes sense to me.

The "+ 1" became necessary because when changing the DCS commands to 
multi type I ran into the issue that there is one for 
"mipi_dsi_dcs_write_seq" and one for "mipi_dsi_dcs_write_buffer"... but 
none for "mipi_dsi_dcs_write" :( So I had to convert those into 
"mipi_dsi_dcs_write_buffer"+multi, thus including the command register 
value into the payload string.

...

>> +       if (candela_enum <= CANDELA_111CD) {
>> +               memcpy(&b6[1], s6e88a0_ams427ap24_elvss[0], SEQ_LENGTH_ELVSS);
>> +       } else {
>> +               memcpy(&b6[1], s6e88a0_ams427ap24_elvss[candela_enum - CANDELA_111CD],
>> +                      SEQ_LENGTH_ELVSS);
>> +       }
>> +
>> +       /* get gamma */
>> +       ca[0] = 0xca;
> 
> #define S6E88A0_SET_GAMMA 0xca

As stated in my reply on patch 3, I would like to avoid those defines 
because firstly the naming becomes arbitrary and secondly it spoils the 
readability of the larger DCS command blocks due to necessary line breaks.

In this specific case here a define would make sense. But I can hardly 
implement it here without doing it elsewhere. Therefore I would like to 
keep that as it is.

...

>> +       mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, b2, ARRAY_SIZE(b2));
>> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0x55, 0x00);
> 
> 0x55 is MIPI_DCS_WRITE_POWER_SAVE in <video/mipi_display.h>

It's the only one that could be used from <video/mipi_display.h>.

Though "MIPI_DCS_WRITE_POWER_SAVE, 0x00" doesn't say much. In the 
downstream driver there are four levels of ACL:
         0x55, 0x00 -> ACL off
         0x55, 0x01 -> default ACL 15 %
         0x55, 0x02 -> ACL 30 %, also corresponds to the "ACL on" command
         0x55, 0x03 -> doesn't seem to be used

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#L275-L281

I would prefer to stay at 0x55 and add comment "acl off". Embedded in a 
block of other DCS commands with plain command register values and 
single line comments appended, as proposed in my reply on patch 3, it 
looks more readable and descriptive in the context of the other commands.

Kind regards,
Jakob