вт, 30 вер. 2025 р. о 06:20 Doug Anderson <[email protected]> пише:
>
> Hi,
>
> On Mon, Sep 29, 2025 at 7:25 AM Svyatoslav Ryhel <[email protected]> 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.
