On Mon, 24 Jun 2024 at 19:31, Doug Anderson <dianders@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Mon, Jun 24, 2024 at 8:27 AM Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > .../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@xxxxxxxxxx>
>
> #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
