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

[Doug Anderson]

Hi,

On Wed, Apr 15, 2026 at 4:56 PM Chintan Patel <chintanlike@xxxxxxxxx> wrote:
>
> Inline the command sequences and remove the nt36672a_send_cmds()
> helper.
>
> Small wrapper helpers around mipi_dsi_dcs_write_buffer_multi()
> are discouraged as they add indirection without improving
> readability and can increase code size.
>
> Signed-off-by: Chintan Patel <chintanlike@xxxxxxxxx>
> ---
>  .../gpu/drm/panel/panel-novatek-nt36672a.c    | 36 +++++++++----------
>  1 file changed, 17 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> index 7e8b5e059575..2f75200806dd 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
> @@ -79,19 +79,6 @@ static inline struct nt36672a_panel *to_nt36672a_panel(struct drm_panel *panel)
>         return container_of(panel, struct nt36672a_panel, base);
>  }
>
> -static void nt36672a_send_cmds(struct mipi_dsi_multi_context *dsi_ctx,
> -                              const struct nt36672a_panel_cmd *cmds, int num)
> -{
> -       unsigned int i;
> -
> -       for (i = 0; i < num; i++) {
> -               const struct nt36672a_panel_cmd *cmd = &cmds[i];
> -
> -               /* cmd->data[0] is the DCS command, cmd->data[1] is the parameter */
> -               mipi_dsi_dcs_write_buffer_multi(dsi_ctx, cmd->data, sizeof(cmd->data));
> -       }
> -}
> -
>  static void nt36672a_panel_power_off(struct drm_panel *panel)
>  {
>         struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
> @@ -108,10 +95,14 @@ static int nt36672a_panel_unprepare(struct drm_panel *panel)
>  {
>         struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
>         struct mipi_dsi_multi_context dsi_ctx = { .dsi = pinfo->link };
> +       unsigned int i;
>
>         /* send off cmds */
> -       nt36672a_send_cmds(&dsi_ctx, pinfo->desc->off_cmds,
> -                          pinfo->desc->num_off_cmds);
> +       for (i = 0; i < pinfo->desc->num_off_cmds; i++) {
> +               const struct nt36672a_panel_cmd *cmd = &pinfo->desc->off_cmds[i];
> +
> +               mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, (const u8 *)cmd->data, sizeof(cmd->data));
> +       }

This isn't quite what I meant earlier [1]. Instead, the idea would be
to fully get rid of the table "tianma_fhd_video_on_cmds_1" and replace
it with a function. The function might look something like this:

static void tianma_fhd_video_on_init_1(struct mipi_dsi_multi_context* dsi_ctx)
{
  /* skin enhancement mode */
  mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0xff, 0x22);
  mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0x00, 0x40);
  mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0x01, 0xc0);
  mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0x02, 0x40);
  ...

  /* dimming enable */
  mipi_dsi_dcs_write_seq_multi(dsi_ctx, 0x01, 0x84);

  ...
}

While that may seem a lot less efficient, the design of
mipi_dsi_dcs_write_seq_multi() limits the inefficiency. Writing things
like the above also allows you to share code better. For instance, if
a second panel is added to the driver that uses the same sequence of
commands for "dimming enable", or setting the resolution, or turning
on "UI" mode, you could create a helper function to share the code.
That is impossible / awkward with the tables-based approach.

Eyeballing the table, you could also potentially optimize things a
little bit if it matters. For instance, I see a big sequence of 0x80
going to lots of registers (without seeing the panel datasheet, I
don't know why). Potentially, you could do things like:

  for (uint8_t reg = 0x8b, i < 0xa0, i++)
    mipi_dsi_dcs_write_var_seq_multi(dsi_ctx, reg, 0x80);

I dunno if it's worth it. You could try running bloat-o-meter to see
if the code is smaller, I guess...

[1] https://lore.kernel.org/r/CAD=FV=WtjW5WWmjeb2zwF2PjiJeZv1jZS_UKZ0bT1658=CkwVA@xxxxxxxxxxxxxx/