Re: [PATCH v3] drm/panel: novatek-nt36672a: Inline panel init sequences

[Chintan Patel <chintanlike@gmail.com>]

>> @@ -162,8 +141,8 @@ static int nt36672a_panel_prepare(struct drm_panel 
>> *panel)
>>       dsi_ctx.accum_err = nt36672a_panel_power_on(pinfo);
>>       /* send first part of init cmds */
>> -    nt36672a_send_cmds(&dsi_ctx, pinfo->desc->on_cmds_1,
>> -               pinfo->desc->num_on_cmds_1);
>> +    if (pinfo->desc->send_init_cmds_1)
>> +        pinfo->desc->send_init_cmds_1(&dsi_ctx);
>>       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>> @@ -173,8 +152,8 @@ static int nt36672a_panel_prepare(struct drm_panel 
>> *panel)
>>       mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
>>       /* Send rest of the init cmds */
>> -    nt36672a_send_cmds(&dsi_ctx, pinfo->desc->on_cmds_2,
>> -               pinfo->desc->num_on_cmds_2);
>> +    if (pinfo->desc->send_init_cmds_2)
>> +        pinfo->desc->send_init_cmds_2(&dsi_ctx);
>>       mipi_dsi_msleep(&dsi_ctx, 120);
> 
> Hi!
> 
> Here, the split of panel init sequence into 2 separate functions _1 / _2
> is completely artificial and completely unnecessary. For some unknown
> reason the initial version of driver had initialization procedure cut in
> 2 parts on the boundary of exit_sleep_mode + set_display_on commands.
> 
> I think the whole init sequence should be glued together into one big
> send_init_cmds() callback and have all these calls:
> 
>      mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
>      /* 0x46 = 70 ms delay */
>      mipi_dsi_msleep(&dsi_ctx, 70);
>      mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> 
> 
> included in the middle. This opens up a possibility for other panels
> based on nt36672a to be supported in this driver, which don't have the
> init sequence split this way. The location of exit_sleep_mode and
> set_display_on commands there is different. And, importantly, the delay
> is different too.
> 
> I once did a very similar change to this driver [1] to add support for
> more panels (ignore touchscreen changes there, look only at panel).
> What is also important, that change was also tested on Xiaomi Poco F1
> (xiaomi-beryllium) phone, which is the main user of this panel.
> Unfortunately I never got to send it, even though I wanted to..
> 
> What do you think?
> 
> [1] https://gitlab.com/sdm845-mainline/linux/-/merge_requests/131

Hi Alexey,

Thanks, that makes sense and I agree the fully panel-owned init sequence 
would likely be a cleaner abstraction long term, especially for 
supporting additional NT36672A panel variants with different sequencing 
requirements.

For this patch, though, I was trying to keep the scope limited to the 
original refactor requested during earlier review:

remove the command table abstraction,
inline the command sequences,
and preserve the existing init flow/behavior as closely as possible.

Since moving exit_sleep_mode, delays, and set_display_on into the panel 
callback changes the sequencing model itself, I was thinking it may be 
better handled as a follow-up cleanup/refactor patch to avoid mixing 
behavioral restructuring into this series.

If that sounds reasonable, I'd prefer to keep this patch focused and 
address the sequencing abstraction separately afterward.

Regards,
Chintan