linux-kernel - Re: [PATCH v1 2/8] gpu/drm: panel: add support for LG LD070WX3-SL01 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] [day] [month] [year] [list]

Message-ID: <CAPVz0n2Prw0ZoQhrodobmSpAu7XV6aX=NV=2ee0RwL3H5hWARg@mail.gmail.com>
Date: Tue, 30 Sep 2025 08:13:23 +0300
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Doug Anderson <dianders@...omium.org>
Cc: Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Thierry Reding <thierry.reding@...il.com>, Jonathan Hunter <jonathanh@...dia.com>, 
	Sam Ravnborg <sam@...nborg.org>, dri-devel@...ts.freedesktop.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-tegra@...r.kernel.org
Subject: Re: [PATCH v1 2/8] gpu/drm: panel: add support for LG LD070WX3-SL01
 MIPI DSI panel

вт, 30 вер. 2025 р. о 06:20 Doug Anderson <dianders@...omium.org> пише:
>
> Hi,
>
> On Mon, Sep 29, 2025 at 7:25 AM Svyatoslav Ryhel <clamor95@...il.com> wrote:
> >
> > +static int lg_ld070wx3_prepare(struct drm_panel *panel)
> > +{
> > +       struct lg_ld070wx3 *priv = to_lg_ld070wx3(panel);
> > +       struct mipi_dsi_multi_context ctx = { .dsi = priv->dsi };
> > +       struct device *dev = panel->dev;
> > +       int ret;
> > +
> > +       ret = regulator_bulk_enable(ARRAY_SIZE(priv->supplies), priv->supplies);
> > +       if (ret < 0) {
> > +               dev_err(dev, "failed to enable power supplies: %d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       /*
> > +        * According to spec delay between enabling supply is 0,
> > +        * for regulators to reach required voltage ~5ms needed.
> > +        * MIPI interface signal for setup requires additional
> > +        * 110ms which in total results in 115ms.
> > +        */
> > +       mdelay(115);
> > +
> > +       mipi_dsi_dcs_soft_reset_multi(&ctx);
> > +       mipi_dsi_msleep(&ctx, 20);
> > +
> > +       /* Differential input impedance selection */
> > +       mipi_dsi_dcs_write_seq_multi(&ctx, 0xae, 0x0b);
> > +
> > +       /* Enter test mode 1 and 2*/
> > +       mipi_dsi_dcs_write_seq_multi(&ctx, 0xee, 0xea);
> > +       mipi_dsi_dcs_write_seq_multi(&ctx, 0xef, 0x5f);
> > +
> > +       /* Increased MIPI CLK driving ability */
> > +       mipi_dsi_dcs_write_seq_multi(&ctx, 0xf2, 0x68);
> > +
> > +       /* Exit test mode 1 and 2 */
> > +       mipi_dsi_dcs_write_seq_multi(&ctx, 0xee, 0x00);
> > +       mipi_dsi_dcs_write_seq_multi(&ctx, 0xef, 0x00);
> > +
> > +       return 0;
>
> Shouldn't this return the accumulated error?
>

Downstream does not, and I am not, though I agree that this may be a
decent idea. Thank you.

>
> > +static int lg_ld070wx3_unprepare(struct drm_panel *panel)
> > +{
> > +       struct lg_ld070wx3 *priv = to_lg_ld070wx3(panel);
> > +       struct mipi_dsi_multi_context ctx = { .dsi = priv->dsi };
> > +
> > +       mipi_dsi_dcs_enter_sleep_mode_multi(&ctx);
> > +
>
> Maybe add some comment about ignoring the accumulated error in the
> context and still doing the sleeps?
>

Isn't that obvious? Regardless of what command returns power supply
should be turned off, preferably with a set amount of delays (delays
are taken from datasheet) to avoid leaving panel in uncertain state
with power on.

>
> > +       msleep(50);
> > +
> > +       regulator_bulk_disable(ARRAY_SIZE(priv->supplies), priv->supplies);
> > +
> > +       /* power supply must be off for at least 1s after panel disable */
> > +       msleep(1000);
>
> Presumably it would be better to keep track of the time you turned it
> off and then make sure you don't turn it on again before that time?
> Otherwise you've got a pretty wasteful delay here.
>

And how do you propose to implement that? Should I use mutex?
Datasheet is clear regarding this, after supply is turned off there
MUST be AT LEAST 1 second of delay before supplies can be turned back
on.

>
> > +static int lg_ld070wx3_probe(struct mipi_dsi_device *dsi)
> > +{
> > +       struct device *dev = &dsi->dev;
> > +       struct lg_ld070wx3 *priv;
> > +       int ret;
> > +
> > +       priv = devm_drm_panel_alloc(dev, struct lg_ld070wx3, panel,
> > +                                   &lg_ld070wx3_panel_funcs,
> > +                                   DRM_MODE_CONNECTOR_DSI);
> > +       if (IS_ERR(priv))
> > +               return PTR_ERR(priv);
> > +
> > +       priv->supplies[0].supply = "vcc";
> > +       priv->supplies[1].supply = "vdd";
> > +
> > +       ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(priv->supplies), priv->supplies);
> > +       if (ret < 0)
> > +               return dev_err_probe(dev, ret, "failed to get regulators\n");
>
> Better to use devm_regulator_bulk_get_const() so you don't need to
> manually init the supplies?

So you propose to init supplies in the probe? Are you aware that
between probe and panel prepare may be 8-10 seconds, sometimes even
more. Having power supplies enabled without panel configuration may
result in permanent panel damage during that time especially since
panel has no hardware reset mechanism.