Hi, On Tue, May 26, 2026 at 9:17 PM Chintan Patel <[email protected]> wrote: > > >> @@ -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.
Yeah, I agree with Chintan here. I'd prefer to apply his patch as-is and then I'd be happy to review and apply a followup patch if someone posts it. This patch already has my Reviewed-by and the changes from v2 to v3 address the nits I had on v2. Thanks! I'll plan to apply this patch in a few days unless there is any additional activity. -Doug
