linux-kernel - Re: [PATCH v2 2/2] drm/panel: Add driver for EDO RM69380 OLED 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]

Date: Tue, 16 Apr 2024 22:22:34 +0200
From: Marijn Suijten <marijn.suijten@...ainline.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: David Wronek <david@...nlining.org>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Jessica Zhang <quic_jesszhan@...cinc.com>, 
	Sam Ravnborg <sam@...nborg.org>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	~postmarketos/upstreaming@...ts.sr.ht, phone-devel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] drm/panel: Add driver for EDO RM69380 OLED panel

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