linux-kernel - Re: [PATCH v4 3/4] drm/panel: jd9365da: Support for kd101ne3-40ti MIPI-DSI panel.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 21 Jun 2024 12:33:45 -0700
From: Doug Anderson <dianders@...gle.com>
To: Zhaoxiong Lv <lvzhaoxiong@...qin.corp-partner.google.com>
Cc: 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, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/4] drm/panel: jd9365da: Support for kd101ne3-40ti
 MIPI-DSI panel.

Hi,

On Thu, Jun 20, 2024 at 1:05 AM Zhaoxiong Lv
<lvzhaoxiong@...qin.corp-partner.google.com> 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().