[PATCH v5 3/5] drm/panel: panel-jadard-jd9365da-h3: use wrapped MIPI DCS functions

[Dmitry Baryshkov]

On Mon, 24 Jun 2024 at 19:31, Doug Anderson <dianders at google.com> wrote:
>
> Hi,
>
> On Mon, Jun 24, 2024 at 8:27 AM Dmitry Baryshkov
> <dmitry.baryshkov at linaro.org> wrote:
> >
> > On Mon, Jun 24, 2024 at 10:19:24PM GMT, Zhaoxiong Lv wrote:
> > > Remove conditional code and always use mipi_dsi_dcs_*multi() wrappers to
> > > simplify driver's init/enable/exit code.
> > >
> > > Signed-off-by: Zhaoxiong Lv <lvzhaoxiong at huaqin.corp-partner.google.com>
> > > ---
> > >  .../gpu/drm/panel/panel-jadard-jd9365da-h3.c  | 793 +++++++++---------
> > >  1 file changed, 390 insertions(+), 403 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> > > index a9c483a7b3fa..e836260338bf 100644
> > > --- a/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> > > +++ b/drivers/gpu/drm/panel/panel-jadard-jd9365da-h3.c
> > > @@ -19,17 +19,13 @@
> > >  #include <linux/of.h>
> > >  #include <linux/regulator/consumer.h>
> > >
> > > -#define JD9365DA_INIT_CMD_LEN                2
> > > -
> > > -struct jadard_init_cmd {
> > > -     u8 data[JD9365DA_INIT_CMD_LEN];
> > > -};
> > > +struct jadard;
> > >
> > >  struct jadard_panel_desc {
> > >       const struct drm_display_mode mode;
> > >       unsigned int lanes;
> > >       enum mipi_dsi_pixel_format format;
> > > -     const struct jadard_init_cmd *init_cmds;
> > > +     int (*init)(struct jadard *jadard);
> > >       u32 num_init_cmds;
> > >  };
> > >
> > > @@ -50,46 +46,33 @@ static inline struct jadard *panel_to_jadard(struct drm_panel *panel)
> > >
> > >  static int jadard_enable(struct drm_panel *panel)
> > >  {
> > > -     struct device *dev = panel->dev;
> > >       struct jadard *jadard = panel_to_jadard(panel);
> > > -     struct mipi_dsi_device *dsi = jadard->dsi;
> > > -     int err;
> > > +     struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi };
> > >
> > >       msleep(120);
> > >
> > > -     err = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > > -     if (err < 0)
> > > -             DRM_DEV_ERROR(dev, "failed to exit sleep mode ret = %d\n", err);
> > > +     mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
> > >
> > > -     err =  mipi_dsi_dcs_set_display_on(dsi);
> > > -     if (err < 0)
> > > -             DRM_DEV_ERROR(dev, "failed to set display on ret = %d\n", err);
> > > +     mipi_dsi_dcs_set_display_on_multi(&dsi_ctx);
> > >
> > > -     return 0;
> > > +     return dsi_ctx.accum_err;
> > >  }
> > >
> > >  static int jadard_disable(struct drm_panel *panel)
> > >  {
> > > -     struct device *dev = panel->dev;
> > >       struct jadard *jadard = panel_to_jadard(panel);
> > > -     int ret;
> > > +     struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi };
> > >
> > > -     ret = mipi_dsi_dcs_set_display_off(jadard->dsi);
> > > -     if (ret < 0)
> > > -             DRM_DEV_ERROR(dev, "failed to set display off: %d\n", ret);
> > > +     mipi_dsi_dcs_set_display_off_multi(&dsi_ctx);
> > >
> > > -     ret = mipi_dsi_dcs_enter_sleep_mode(jadard->dsi);
> > > -     if (ret < 0)
> > > -             DRM_DEV_ERROR(dev, "failed to enter sleep mode: %d\n", ret);
> > > +     mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx);
> > >
> > > -     return 0;
> > > +     return dsi_ctx.accum_err;
> > >  }
> > >
> > >  static int jadard_prepare(struct drm_panel *panel)
> > >  {
> > >       struct jadard *jadard = panel_to_jadard(panel);
> > > -     const struct jadard_panel_desc *desc = jadard->desc;
> > > -     unsigned int i;
> > >       int ret;
> > >
> > >       ret = regulator_enable(jadard->vccio);
> > > @@ -109,13 +92,9 @@ static int jadard_prepare(struct drm_panel *panel)
> > >       gpiod_set_value(jadard->reset, 1);
> > >       msleep(130);
> > >
> > > -     for (i = 0; i < desc->num_init_cmds; i++) {
> > > -             const struct jadard_init_cmd *cmd = &desc->init_cmds[i];
> > > -
> > > -             ret = mipi_dsi_dcs_write_buffer(dsi, cmd->data, JD9365DA_INIT_CMD_LEN);
> >
> > This function usesd mipi_dsi_dcs_write_buffer()...
> >
> > > -             if (ret < 0)
> > > -                     return ret;
> > > -     }
> > > +     ret = jadard->desc->init(jadard);
> > > +     if (ret)
> > > +             return ret;
> > >
> > >       return 0;
> >
> > [...]
> >
> > > +static int radxa_display_8hd_ad002_init_cmds(struct jadard *jadard)
> > > +{
> > > +     struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi };
> > > +
> > > +     mipi_dsi_dcs_write_seq_multi(&dsi_ctx, 0xE0, 0x00);
> >
> > ... while your code uses mipi_dsi_dcs_write_seq_multi(), which
> > internally calls mipi_dsi_generic_write_multi(). These two function use
> > different packet types to send the payload. To be conservatite, please
> > use mipi_dsi_dcs_write_buffer_multi().
>
> Are you certain about this? I see that mipi_dsi_dcs_write_seq_multi()
> is just a wrapper on mipi_dsi_dcs_write_buffer_multi(). Specifically,
> I see:

I see, I was looking at mipi_dsi_generic_write_seq_multi() instead.
Please excuse me.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>

>
> #define mipi_dsi_dcs_write_seq_multi(ctx, cmd, seq...)                  \
>   do {                                                            \
>     static const u8 d[] = { cmd, seq };                     \
>     mipi_dsi_dcs_write_buffer_multi(ctx, d, ARRAY_SIZE(d)); \
>   } while (0)
>
> Certainly I could be confused...
>



-- 
With best wishes
Dmitry