Hi,
On Thu, Jun 20, 2024 at 1:05 AM Zhaoxiong Lv
<lvzhaoxiong@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> @@ -31,6 +31,15 @@ struct jadard_panel_desc {
> enum mipi_dsi_pixel_format format;
> const struct jadard_init_cmd *init_cmds;
> u32 num_init_cmds;
> + bool lp11_before_reset;
> + bool reset_before_power_off_vcioo;
> + unsigned int vcioo_to_lp11_delay;
> + unsigned int lp11_to_reset_delay;
> + unsigned int exit_sleep_to_display_on_delay;
> + unsigned int display_on_delay;
> + unsigned int backlight_off_to_display_off_delay;
> + unsigned int display_off_to_enter_sleep_delay;
> + unsigned int enter_sleep_to_reset_down_delay;
If the above delays are in milliseconds please add a "_ms" suffix to
the variables.
It's also surprising to me that you need all these extra delays /
boolean options if this is really the same underlying chipset. Any
idea why they didn't need it?
> @@ -53,6 +62,7 @@ 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;
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi };
> int err;
>
> msleep(120);
> @@ -61,10 +71,16 @@ static int jadard_enable(struct drm_panel *panel)
> if (err < 0)
> DRM_DEV_ERROR(dev, "failed to exit sleep mode ret = %d\n", err);
>
> + if (jadard->desc->exit_sleep_to_display_on_delay)
> + mipi_dsi_msleep(dsi_ctx, jadard->desc->exit_sleep_to_display_on_delay);
mipi_dsi_msleep() is really only useful if you're using it between a
whole bunch of other "_multi" functions. If you just have a simple
"msleep()" then just call "msleep()".
...but really you should be transitioning the function to just use
more "_multi" functions since they exist for the other functions
called here. In other words, this function should look something like:
mipi_dsi_dcs_exit_sleep_mode_multi(&dsi_ctx);
if (...)
mipi_dsi_msleep(...)
mipi_dsi_dcs_set_display_on_multi
if (...)
mipi_dsi_msleep(...)
return dsi_ctx.accum_err;
I would also note that you seem to be missing commit 66055636a146
("drm/mipi-dsi: fix handling of ctx in mipi_dsi_msleep"), which
changes the first argument of mipi_dsi_msleep() to be a pointer.
> @@ -72,16 +88,26 @@ static int jadard_disable(struct drm_panel *panel)
> {
> struct device *dev = panel->dev;
> struct jadard *jadard = panel_to_jadard(panel);
> + struct mipi_dsi_multi_context dsi_ctx = { .dsi = jadard->dsi };
> int ret;
>
> + if (jadard->desc->backlight_off_to_display_off_delay)
> + mipi_dsi_msleep(dsi_ctx, jadard->desc->backlight_off_to_display_off_delay);
> +
> ret = mipi_dsi_dcs_set_display_off(jadard->dsi);
Similar comments here to the enable function. Use more _multi which
then makes mipi_dsi_msleep() make sense.
> @@ -100,6 +127,20 @@ static int jadard_prepare(struct drm_panel *panel)
> if (ret)
> return ret;
>
> + if (jadard->desc->vcioo_to_lp11_delay)
> + mipi_dsi_msleep(dsi_ctx, jadard->desc->vcioo_to_lp11_delay);
> +
> + if (jadard->desc->lp11_before_reset) {
> + ret = mipi_dsi_dcs_nop(jadard->dsi);
> + if (ret)
> + return ret;
> +
> + usleep_range(1000, 2000);
Why do you need this extra sleep. For any panel that needs it can't
the "lp11_to_reset_delay" just be increased by 1ms?
> @@ -582,6 +628,233 @@ static const struct jadard_panel_desc cz101b4001_desc = {
> .num_init_cmds = ARRAY_SIZE(cz101b4001_init_cmds),
> };
>
> +static const struct jadard_init_cmd kingdisplay_kd101ne3_init_cmds[] = {
> + { .data = { 0xe0, 0x00 } },
> + { .data = { 0xe1, 0x93 } },
> + { .data = { 0xe2, 0x65 } },
> + { .data = { 0xe3, 0xf8 } },
As mentioned in my reply to patch #1, please don't add new panels that
use the table-based approach. Before adding your new panel to this
driver have a patch that transitions it to a per-panel init() function
that uses mipi_dsi_dcs_write_seq_multi().
