The LD070WX3 is a Color Active Matrix Liquid Crystal Display with an
integral Light Emitting Diode (LED) backlight system. The matrix employs
a-Si Thin Film Transistor as the active element. It is a transmissive type
display operating in the normally Black mode. This TFT-LCD has 7.0 inches
diagonally measured active display area with WXGA resolution (800 by 1280
pixel array).

LG LD070WX3-SL01 MIPI DSI panel was treated as simple DSI panel when it is
actually not and requires proper setup for correct work. Simple panel work
relied on preliminary configuration done by bootloader.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 drivers/gpu/drm/panel/Kconfig             |  13 ++
 drivers/gpu/drm/panel/Makefile            |   1 +
 drivers/gpu/drm/panel/panel-lg-ld070wx3.c | 182 ++++++++++++++++++++++
 drivers/gpu/drm/panel/panel-simple.c      |  31 ----
 4 files changed, 196 insertions(+), 31 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-lg-ld070wx3.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index aad4e0da8f75..ca5c5e60cfa1 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -398,6 +398,19 @@ config DRM_PANEL_LG_LB035Q02
 	  (found on the Gumstix Overo Palo35 board). To compile this driver as
 	  a module, choose M here.
 
+config DRM_PANEL_LG_LD070WX3
+	tristate "LG LD070WX3 MIPI DSI panel"
+	depends on OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select VIDEOMODE_HELPERS
+	help
+	  Say Y here if you want to enable support for the LD070WX3 MIPI DSI
+	  panel found in the NVIDIA Tegra Note 7 tablet.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called panel-lg-ld070wx3.
+
 config DRM_PANEL_LG_LG4573
 	tristate "LG4573 RGB/SPI panel"
 	depends on OF && SPI
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index a6a100e4c4e6..a673a74cd371 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o
 obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o
 obj-$(CONFIG_DRM_PANEL_LINCOLNTECH_LCD197) += panel-lincolntech-lcd197.o
 obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o
+obj-$(CONFIG_DRM_PANEL_LG_LD070WX3) += panel-lg-ld070wx3.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_LG_SW43408) += panel-lg-sw43408.o
 obj-$(CONFIG_DRM_PANEL_MAGNACHIP_D53E6EA8966) += panel-magnachip-d53e6ea8966.o
diff --git a/drivers/gpu/drm/panel/panel-lg-ld070wx3.c b/drivers/gpu/drm/panel/panel-lg-ld070wx3.c
new file mode 100644
index 000000000000..da46a317a749
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-lg-ld070wx3.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/array_size.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+
+struct lg_ld070wx3 {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+
+	struct regulator_bulk_data supplies[2];
+};
+
+static inline struct lg_ld070wx3 *to_lg_ld070wx3(struct drm_panel *panel)
+{
+	return container_of(panel, struct lg_ld070wx3, panel);
+}
+
+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;
+}
+
+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);
+
+	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);
+
+	return 0;
+}
+
+static const struct drm_display_mode lg_ld070wx3_mode = {
+	.clock = (800 + 32 + 48 + 8) * (1280 + 5 + 3 + 1) * 60 / 1000,
+	.hdisplay = 800,
+	.hsync_start = 800 + 32,
+	.hsync_end = 800 + 32 + 48,
+	.htotal = 800 + 32 + 48 + 8,
+	.vdisplay = 1280,
+	.vsync_start = 1280 + 5,
+	.vsync_end = 1280 + 5 + 3,
+	.vtotal = 1280 + 5 + 3 + 1,
+	.width_mm = 94,
+	.height_mm = 151,
+	.type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static int lg_ld070wx3_get_modes(struct drm_panel *panel,
+				 struct drm_connector *connector)
+{
+	return drm_connector_helper_get_modes_fixed(connector, &lg_ld070wx3_mode);
+}
+
+static const struct drm_panel_funcs lg_ld070wx3_panel_funcs = {
+	.prepare = lg_ld070wx3_prepare,
+	.unprepare = lg_ld070wx3_unprepare,
+	.get_modes = lg_ld070wx3_get_modes,
+};
+
+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");
+
+	priv->dsi = dsi;
+	mipi_dsi_set_drvdata(dsi, priv);
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_LPM;
+
+	ret = drm_panel_of_backlight(&priv->panel);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to get backlight\n");
+
+	drm_panel_add(&priv->panel);
+
+	ret = devm_mipi_dsi_attach(dev, dsi);
+	if (ret < 0) {
+		drm_panel_remove(&priv->panel);
+		return dev_err_probe(dev, ret, "failed to attach to DSI host\n");
+	}
+
+	return 0;
+}
+
+static void lg_ld070wx3_remove(struct mipi_dsi_device *dsi)
+{
+	struct lg_ld070wx3 *priv = mipi_dsi_get_drvdata(dsi);
+
+	drm_panel_remove(&priv->panel);
+}
+
+static const struct of_device_id lg_ld070wx3_of_match[] = {
+	{ .compatible = "lg,ld070wx3-sl01" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, lg_ld070wx3_of_match);
+
+static struct mipi_dsi_driver lg_ld070wx3_driver = {
+	.driver = {
+		.name = "panel-lg-ld070wx3",
+		.of_match_table = lg_ld070wx3_of_match,
+	},
+	.probe = lg_ld070wx3_probe,
+	.remove = lg_ld070wx3_remove,
+};
+module_mipi_dsi_driver(lg_ld070wx3_driver);
+
+MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@gmail.com>");
+MODULE_DESCRIPTION("LG LD070WX3-SL01 DSI panel driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 9f81fa960b46..ea2cdddb9b8f 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -5512,34 +5512,6 @@ static const struct panel_desc_dsi boe_tv080wum_nl0 = {
 	.lanes = 4,
 };
 
-static const struct drm_display_mode lg_ld070wx3_sl01_mode = {
-	.clock = 71000,
-	.hdisplay = 800,
-	.hsync_start = 800 + 32,
-	.hsync_end = 800 + 32 + 1,
-	.htotal = 800 + 32 + 1 + 57,
-	.vdisplay = 1280,
-	.vsync_start = 1280 + 28,
-	.vsync_end = 1280 + 28 + 1,
-	.vtotal = 1280 + 28 + 1 + 14,
-};
-
-static const struct panel_desc_dsi lg_ld070wx3_sl01 = {
-	.desc = {
-		.modes = &lg_ld070wx3_sl01_mode,
-		.num_modes = 1,
-		.bpc = 8,
-		.size = {
-			.width = 94,
-			.height = 151,
-		},
-		.connector_type = DRM_MODE_CONNECTOR_DSI,
-	},
-	.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_CLOCK_NON_CONTINUOUS,
-	.format = MIPI_DSI_FMT_RGB888,
-	.lanes = 4,
-};
-
 static const struct drm_display_mode lg_lh500wx1_sd03_mode = {
 	.clock = 67000,
 	.hdisplay = 720,
@@ -5663,9 +5635,6 @@ static const struct of_device_id dsi_of_match[] = {
 	}, {
 		.compatible = "boe,tv080wum-nl0",
 		.data = &boe_tv080wum_nl0
-	}, {
-		.compatible = "lg,ld070wx3-sl01",
-		.data = &lg_ld070wx3_sl01
 	}, {
 		.compatible = "lg,lh500wx1-sd03",
 		.data = &lg_lh500wx1_sd03
-- 
2.48.1

Hi,

On Mon, Sep 29, 2025 at 7:25 AM Svyatoslav Ryhel <clamor95@gmail.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?


> +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?


> +       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.


> +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?

вт, 30 вер. 2025 р. о 06:20 Doug Anderson <dianders@chromium.org> пише:
>
> Hi,
>
> On Mon, Sep 29, 2025 at 7:25 AM Svyatoslav Ryhel <clamor95@gmail.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.

Hi,

On Mon, Sep 29, 2025 at 10:13 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> > > +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.

I won't insist, though IMO any time an error return is purposely
ignored a comment about why can be justified.


> > > +       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.

You don't really need a mutex since the DRM core will make sure that
prepare and unprepare can't be called at the same time. panel-edp
implements this. See `unprepared_time` I believe.

NOTE: if you want to get really deep into this, it's actually a bit of
a complicated topic and I would also encourage you to add an
"off-on-delay-us" to the regulator in your device tree (which only
works on some regulators but really should be universal). This is
important because:

1. The regulator could be shared by other consumers and they could
cause violations of this.

2. The regulator could potentially be in either state when Linux starts.

3. The regulator framework could adjust the state of the regulator at
regulator probe time.

The "off-on-delay-us" handles at least some more of those cases,
though I seem to remember that at least a few of them still have rough
edges...


> > > +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.

Maybe look more closely at devm_regulator_bulk_get_const(). Really it
should just save you the lines of code:

  priv->supplies[0].supply = "vcc";
  priv->supplies[1].supply = "vdd";

...and it lets you put those names in a "static const" table in your
driver. All the timings of when regulators are initted should be the
same.

-Doug

вт, 30 вер. 2025 р. о 17:34 Doug Anderson <dianders@chromium.org> пише:
>
> Hi,
>
> On Mon, Sep 29, 2025 at 10:13 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> >
> > > > +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.
>
> I won't insist, though IMO any time an error return is purposely
> ignored a comment about why can be justified.
>
>
> > > > +       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.
>
> You don't really need a mutex since the DRM core will make sure that
> prepare and unprepare can't be called at the same time. panel-edp
> implements this. See `unprepared_time` I believe.
>
> NOTE: if you want to get really deep into this, it's actually a bit of
> a complicated topic and I would also encourage you to add an

Please spare me of this, I have enough stuff to work with and have no
capacity to delve into depth of drm any deeper. In case this panel had
a reset I would not care about regulators too much, but it already
gave me too much pain and caused partially reversible damage to itself
that I am not willing to risk.

> "off-on-delay-us" to the regulator in your device tree (which only
> works on some regulators but really should be universal). This is
> important because:
>
> 1. The regulator could be shared by other consumers and they could
> cause violations of this.
>
> 2. The regulator could potentially be in either state when Linux starts.
>
> 3. The regulator framework could adjust the state of the regulator at
> regulator probe time.
>
> The "off-on-delay-us" handles at least some more of those cases,
> though I seem to remember that at least a few of them still have rough
> edges...

regulator may be not fixes and not handled fully by framework, I am
not wiling to risk.

>
>
> > > > +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.
>
> Maybe look more closely at devm_regulator_bulk_get_const(). Really it
> should just save you the lines of code:
>
>   priv->supplies[0].supply = "vcc";
>   priv->supplies[1].supply = "vdd";
>
> ...and it lets you put those names in a "static const" table in your
> driver. All the timings of when regulators are initted should be the
> same.
>

Here it is my bad, devm_regulator_bulk_get_const indeed should be
preferred. I thought you meant devm_regulator_bulk_get_enable which is
not the case.

> -Doug

Hi,

On Tue, Sep 30, 2025 at 7:48 AM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
>
> вт, 30 вер. 2025 р. о 17:34 Doug Anderson <dianders@chromium.org> пише:
> >
> > Hi,
> >
> > On Mon, Sep 29, 2025 at 10:13 PM Svyatoslav Ryhel <clamor95@gmail.com> wrote:
> > >
> > > > > +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.
> >
> > I won't insist, though IMO any time an error return is purposely
> > ignored a comment about why can be justified.
> >
> >
> > > > > +       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.
> >
> > You don't really need a mutex since the DRM core will make sure that
> > prepare and unprepare can't be called at the same time. panel-edp
> > implements this. See `unprepared_time` I believe.
> >
> > NOTE: if you want to get really deep into this, it's actually a bit of
> > a complicated topic and I would also encourage you to add an
>
> Please spare me of this, I have enough stuff to work with and have no
> capacity to delve into depth of drm any deeper. In case this panel had
> a reset I would not care about regulators too much, but it already
> gave me too much pain and caused partially reversible damage to itself
> that I am not willing to risk.

I won't insist. It's not much code, but it could always be done later.

-Doug