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
