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