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

[Chintan Patel]

> > @@ -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