1. Patchew
  2. linux
  3. View series

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

Chintan Patel posted 1 patch 2 months ago 
.../gpu/drm/panel/panel-novatek-nt36672a.c    | 36 +++++++++----------
1 file changed, 17 insertions(+), 19 deletions(-)
[PATCH] drm/panel: novatek-nt36672a: Inline command sequences
Posted by Chintan Patel 2 months ago 
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@gmail.com>
---
 .../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));
+	}
 
 	/* Reset error to continue with display off even if send_cmds failed */
 	dsi_ctx.accum_err = 0;
@@ -158,12 +149,16 @@ static int nt36672a_panel_prepare(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;
 
 	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);
+	for (i = 0; i < pinfo->desc->num_on_cmds_1; i++) {
+		const struct nt36672a_panel_cmd *cmd = &pinfo->desc->on_cmds_1[i];
+
+		mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, (const u8 *)cmd->data, sizeof(cmd->data));
+	}
 
 	mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
 
@@ -173,8 +168,11 @@ 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);
+	for (i = 0; i < pinfo->desc->num_on_cmds_2; i++) {
+		const struct nt36672a_panel_cmd *cmd = &pinfo->desc->on_cmds_2[i];
+
+		mipi_dsi_dcs_write_buffer_multi(&dsi_ctx, (const u8 *)cmd->data, sizeof(cmd->data));
+	}
 
 	mipi_dsi_msleep(&dsi_ctx, 120);
 
-- 
2.43.0
Re: [PATCH] drm/panel: novatek-nt36672a: Inline command sequences
Posted by Doug Anderson 1 month, 4 weeks ago 
Hi,

On Wed, Apr 15, 2026 at 4:56 PM Chintan Patel <chintanlike@gmail.com> 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@gmail.com>
> ---
>  .../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@mail.gmail.com/
Re: [PATCH] drm/panel: novatek-nt36672a: Inline command sequences
Posted by Chintan Patel 1 month, 4 weeks ago 
Hi Doug,

> 
> 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@mail.gmail.com/

Thank you for the clarification and patience. I apologize for 
misunderstanding your initial feedback—I appreciate you taking the time 
to explain that the goal was to inline the command sequences as 
functions rather than simply removing the helper.

I will send v2.

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.