linux-kernel - Re: [PATCH v2 1/2] drm/panel: jd9365da: Move the location of "exit sleep mode" and "set display on" commands

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] [day] [month] [year] [list]

Message-ID: <CAD=FV=WrMxyxkuCYEbd=aYFaTJKNqGqXr6Re+V=B_h9jnjHPvg@mail.gmail.com>
Date: Tue, 6 Aug 2024 10:31:25 -0700
From: Doug Anderson <dianders@...omium.org>
To: Zhaoxiong Lv <lvzhaoxiong@...qin.corp-partner.google.com>
Cc: neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, 
	hsinyi@...gle.com, airlied@...il.com, daniel@...ll.ch, jagan@...eble.ai, 
	dmitry.baryshkov@...aro.org, jani.nikula@...ux.intel.com, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] drm/panel: jd9365da: Move the location of "exit
 sleep mode" and "set display on" commands

Hi,

On Mon, Aug 5, 2024 at 8:40 PM Zhaoxiong Lv
<lvzhaoxiong@...qin.corp-partner.google.com> wrote:
>
> Move the "exit sleep mode" and "set display on" command from
> enable() to init() function.
>
> As mentioned in the patch:
> https://lore.kernel.org/all/20240624141926.5250-2-lvzhaoxiong@huaqin.corp-partner.google.com/
>
> Our DSI host has different modes in prepare() and enable()

nit: it's not obvious to the reader of this patch which DSI host is
"our"s. Maybe spell out which SoC you're using? I assume this is a
Mediatek SoC?


> functions. prepare() is in LP mode and enable() is in HS mode.
> Since the "exit sleep mode" and "set display on" command must
> also be sent in LP mode, so we also move "exit sleep mode" and
> "set display on" command to the init() function.
>
> We have no other actions in the enable() function after moves
> "exit sleep mode" and "set display on", and we checked the call
> of the enable() function during the "startup" process. It seems
> that only one judgment was made in drm_panel_enabel(). If the
> panel does not define enable(), the judgment will skip the
> enable() and continue execution. This does not seem to have
> any other effects,and we found that some drivers also seem

s/effects,and/effect, and/


> to have no enable() function added, for example:
> panel-asus-z00t-tm5p5-n35596 / panel-boe-himax8279d ...
> In addition, we briefly tested the kingdisplay_kd101ne3 panel and
> melfas_lmfbx101117480 panel, and it seems that there is no garbage
> on the panel, so we delete enable() function.
>
> After moving the "exit sleep mode" and "set display on" command
> to the init() function, we no longer need additional delay
> judgment, so we deletevariables "exit_sleep_to_display_on_delay_ms"

nit: s/deletevariables/delete variables/


> and "display_on_delay_ms".
>
> Signed-off-by: Zhaoxiong Lv <lvzhaoxiong@...qin.corp-partner.google.com>
> ---
> Changes between V2 and V1:
> -  1. The code has not changed, Modify the commit information.
> v1: https://lore.kernel.org/all/20240725083245.12253-2-lvzhaoxiong@huaqin.corp-partner.google.com/
> ---
>  .../gpu/drm/panel/panel-jadard-jd9365da-h3.c  | 59 ++++++++++---------
>  1 file changed, 32 insertions(+), 27 deletions(-)

nit: ${SUBJECT} is a bit long. In general it's worth abbreviating a
bit more so that the subject doesn't go to crazy.

drm/panel: jd9365da: Move "exit sleep mode" and "set display on" cmds


Aside from the above nits, this looks OK to me. I wouldn't object to
fixing some of my own nits when applying or you could send a v3 if
there is no other feedback. In any case:

Reviewed-by: Douglas Anderson <dianders@...omium.org>


I'd prefer someone with more MIPI panel experience give a review,
though, so I'll expect that Jessica or Neil or someone else gives a
review.

-Doug