Re: [PATCH v2 2/2] drm/panel: Add driver for EDO RM69380 OLED panel

[Marijn Suijten]

On 2024-04-15 19:50:49, Dmitry Baryshkov wrote:
> On Mon, Apr 15, 20
[...]
> > +static int rm69380_on(struct rm69380_panel *ctx)
[...]
> ret = mipi_dsi_dcs_set_display_brightness_large(dsi, 0x7ff);

Downstream may send this here, but why?  As far as I've observed, update_status
also sets &backlight_properties.brightness which you configure below.

Try removing this line and maybe also set the initial brightness lower to the
benefit of users' eyes and OLED lifetime?

[...]
> > +
> > +	if (dsi_sec) {
> > +		dev_dbg(dev, "Using Dual-DSI\n");
> > +
> > +		const struct mipi_dsi_device_info info = { "RM69380", 0,

Personally I'm never really sure what to put in the name here, maybe something
that signifies the second DSI interface of the panel?

> > +							   dsi_sec };
> > +
> > +		dev_dbg(dev, "Found second DSI `%s`\n", dsi_sec->name);
> > +
> > +		dsi_sec_host = of_find_mipi_dsi_host_by_node(dsi_sec);
> > +		of_node_put(dsi_sec);
> > +		if (!dsi_sec_host) {
> > +			return dev_err_probe(dev, -EPROBE_DEFER,
> > +					     "Cannot get secondary DSI host\n");
> > +		}
> > +
> > +		ctx->dsi[1] =
> > +			mipi_dsi_device_register_full(dsi_sec_host, &info);
> 
> Either you should be using devm_mipi_dsi_device_register_full() here or
> you should call mipi_dsi_device_unregister() in the error and remove
> paths. I'd suggest the former.

There is also devm_mipi_dsi_attach() which may solve inadequate cleanup handling
in the error paths below, as pointed out by Christophe.

> 
> > +		if (IS_ERR(ctx->dsi[1])) {
> > +			return dev_err_probe(dev, PTR_ERR(ctx->dsi[1]),
> > +					     "Cannot get secondary DSI node\n");
> > +		}
> > +
> > +		dev_dbg(dev, "Second DSI name `%s`\n", ctx->dsi[1]->name);

It looks like you inerited /all/ my debug logging when copy-pasting this setup
from my in-progress dual-DSI dual-DSC Samsung ANA6707 panel driver.  Since it's
all working now, I suggest to remove mostly-useless debug lines like this.

I'll continue the review on v3, as I mainly wanted to extend the initial devm_
suggestion from Dmitry done on v2.

- Marijn