[PATCH] drm/panel: novatek-nt35950: transition to mipi_dsi wrapped functions

Doug Anderson dianders at chromium.org
Tue Aug 27 19:26:26 UTC 2024


Hi,

On Sat, Aug 24, 2024 at 1:44 AM Tejas Vipin <tejasvipin76 at gmail.com> wrote:
>
> +static void nt35950_set_cmd2_page(struct mipi_dsi_multi_context *dsi_ctx,
> +                                 struct nt35950 *nt, u8 page)
>  {
>         const u8 mauc_cmd2_page[] = { MCS_CMD_MAUCCTR, 0x55, 0xaa, 0x52,
>                                       0x08, page };
> -       int ret;
>
> -       ret = mipi_dsi_dcs_write_buffer(nt->dsi[0], mauc_cmd2_page,
> +       mipi_dsi_dcs_write_buffer_multi(dsi_ctx, mauc_cmd2_page,
>                                         ARRAY_SIZE(mauc_cmd2_page));
> -       if (ret < 0)
> -               return ret;
> +       if (dsi_ctx->accum_err)
> +               return;
>
>         nt->last_page = page;
> -       return 0;

nit: It's a style choice, but I personally would have changed the above to just:

if (!dsi_ctx->accum_err)
  nt->last_page = page;


> @@ -284,109 +258,68 @@ static int nt35950_on(struct nt35950 *nt)
>  {
>         const struct nt35950_panel_mode *mode_data = nt->desc->mode_data;
>         struct mipi_dsi_device *dsi = nt->dsi[0];
> -       struct device *dev = &dsi->dev;
> -       int ret;
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
>         nt->cur_mode = nt35950_get_current_mode(nt);
>         nt->dsi[0]->mode_flags |= MIPI_DSI_MODE_LPM;
>         nt->dsi[1]->mode_flags |= MIPI_DSI_MODE_LPM;
>
> -       ret = nt35950_set_cmd2_page(nt, 0);
> -       if (ret < 0)
> -               return ret;
> -
> -       ret = nt35950_set_data_compression(nt, mode_data[nt->cur_mode].compression);
> -       if (ret < 0)
> -               return ret;
> -
> -       ret = nt35950_set_scale_mode(nt, mode_data[nt->cur_mode].scaler_mode);
> -       if (ret < 0)
> -               return ret;
> -
> -       ret = nt35950_set_scaler(nt, mode_data[nt->cur_mode].scaler_on);
> -       if (ret < 0)
> -               return ret;
> +       nt35950_set_cmd2_page(&dsi_ctx, nt, 0);
> +       nt35950_set_data_compression(&dsi_ctx, nt, mode_data[nt->cur_mode].compression);
> +       nt35950_set_scale_mode(&dsi_ctx, mode_data[nt->cur_mode].scaler_mode);
> +       nt35950_set_scaler(&dsi_ctx, mode_data[nt->cur_mode].scaler_on);
> +       nt35950_set_dispout(&dsi_ctx, nt);
>
> -       ret = nt35950_set_dispout(nt);
> -       if (ret < 0)
> -               return ret;
> -
> -       ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to set tear on: %d\n", ret);
> -               return ret;
> -       }
> -
> -       ret = mipi_dsi_dcs_set_tear_scanline(dsi, 0);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to set tear scanline: %d\n", ret);
> -               return ret;
> -       }
> +       mipi_dsi_dcs_set_tear_on_multi(&dsi_ctx, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> +       mipi_dsi_dcs_set_tear_scanline_multi(&dsi_ctx, 0);
>
>         /* CMD2 Page 1 */
> -       ret = nt35950_set_cmd2_page(nt, 1);
> -       if (ret < 0)
> -               return ret;
> +       nt35950_set_cmd2_page(&dsi_ctx, nt, 1);
>
>         /* Unknown command */
> -       mipi_dsi_dcs_write_seq(dsi, 0xd4, 0x88, 0x88);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xd4, 0x88, 0x88);
>
>         /* CMD2 Page 7 */
> -       ret = nt35950_set_cmd2_page(nt, 7);
> -       if (ret < 0)
> -               return ret;
> +       nt35950_set_cmd2_page(&dsi_ctx, nt, 7);
>
>         /* Enable SubPixel Rendering */
> -       mipi_dsi_dcs_write_seq(dsi, MCS_PARAM_SPR_EN, 0x01);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MCS_PARAM_SPR_EN, 0x01);
>
>         /* SPR Mode: YYG Rainbow-RGB */
> -       mipi_dsi_dcs_write_seq(dsi, MCS_PARAM_SPR_MODE, MCS_SPR_MODE_YYG_RAINBOW_RGB);
> +       mipi_dsi_dcs_write_seq_multi(&dsi_ctx, MCS_PARAM_SPR_MODE,
> +                                    MCS_SPR_MODE_YYG_RAINBOW_RGB);
>
>         /* CMD3 */
> -       ret = nt35950_inject_black_image(nt);
> -       if (ret < 0)
> -               return ret;
> +       nt35950_inject_black_image(&dsi_ctx);
> +       mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 120);
>
> -       ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> -       if (ret < 0)
> -               return ret;
> -       msleep(120);
> -
> -       ret = mipi_dsi_dcs_set_display_on(dsi);
> -       if (ret < 0)
> -               return ret;
> -       msleep(120);
> +       mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 120);
>
>         nt->dsi[0]->mode_flags &= ~MIPI_DSI_MODE_LPM;
>         nt->dsi[1]->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> -       return 0;
> +       return dsi_ctx.accum_err;

I think you only want to clear "MIPI_DSI_MODE_LPM" if there was no
error, right? That matches the old code. So you'd want:

if (dsi_ctx.accum_err)
  return dsi_ctx.accum_err;

nt->dsi[0]->mode_flags &= ~MIPI_DSI_MODE_LPM;
nt->dsi[1]->mode_flags &= ~MIPI_DSI_MODE_LPM;

return 0;


>  static int nt35950_off(struct nt35950 *nt)
>  {
> -       struct device *dev = &nt->dsi[0]->dev;
> -       int ret;
> +       struct mipi_dsi_device *dsi = nt->dsi[0];
> +       struct mipi_dsi_multi_context dsi_ctx = { .dsi = dsi };
>
> -       ret = mipi_dsi_dcs_set_display_off(nt->dsi[0]);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to set display off: %d\n", ret);
> -               goto set_lpm;
> -       }
> -       usleep_range(10000, 11000);
> +       mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> +       mipi_dsi_usleep_range(&dsi_ctx, 10000, 11000);
>
> -       ret = mipi_dsi_dcs_enter_sleep_mode(nt->dsi[0]);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
> -               goto set_lpm;
> -       }
> -       msleep(150);
> +       mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> +       mipi_dsi_msleep(&dsi_ctx, 150);
>
> -set_lpm:
> -       nt->dsi[0]->mode_flags |= MIPI_DSI_MODE_LPM;
> -       nt->dsi[1]->mode_flags |= MIPI_DSI_MODE_LPM;
> +       if (dsi_ctx.accum_err) {
> +               nt->dsi[0]->mode_flags |= MIPI_DSI_MODE_LPM;
> +               nt->dsi[1]->mode_flags |= MIPI_DSI_MODE_LPM;
> +       }

I'm pretty sure you want to set MIPI_DSI_MODE_LPM _unconditionally,
right? The old code would set it in both error and non-error cases I
think.


> @@ -452,10 +384,8 @@ static int nt35950_prepare(struct drm_panel *panel)
>         nt35950_reset(nt);
>
>         ret = nt35950_on(nt);
> -       if (ret < 0) {
> -               dev_err(dev, "Failed to initialize panel: %d\n", ret);
> +       if (ret < 0)
>                 goto end;
> -       }

I'd just get rid of the "if" test since "end" is next anyway. Given
that there's no error message it seems silly to have the "if" test
now.


> @@ -469,12 +399,8 @@ static int nt35950_prepare(struct drm_panel *panel)
>  static int nt35950_unprepare(struct drm_panel *panel)
>  {
>         struct nt35950 *nt = to_nt35950(panel);
> -       struct device *dev = &nt->dsi[0]->dev;
> -       int ret;
>
> -       ret = nt35950_off(nt);
> -       if (ret < 0)
> -               dev_err(dev, "Failed to deinitialize panel: %d\n", ret);
> +       nt35950_off(nt);

This is the only caller of nt35950_off() and you no longer check the
return code, so you can just make nt35950_off() void, right?


More information about the dri-devel mailing list