Date: Mon, 24 Jun 2024 09:31:14 -0700
From: Doug Anderson <dianders@...gle.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Zhaoxiong Lv <lvzhaoxiong@...qin.corp-partner.google.com>, dmitry.torokhov@...il.com, 
	robh@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org, 
	jikos@...nel.org, benjamin.tissoires@...hat.co, hsinyi@...gle.com, 
	jagan@...eble.ai, neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/5] drm/panel: panel-jadard-jd9365da-h3: use wrapped
 MIPI DCS functions

Hi,

On Mon, Jun 24, 2024 at 8:27 AM Dmitry Baryshkov
<dmitry.baryshkov@...aro.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@...qin.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:

#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...

-Doug

Powered by blists - more mailing lists