Received-SPF: pass (google.com: domain of linux-kernel+bounces-147518-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 16 Apr 2024 22:22:34 +0200
From: Marijn Suijten <marijn.suijten@somainline.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: David Wronek <david@mainlining.org>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, 
	Conor Dooley <conor+dt@kernel.org>, Konrad Dybcio <konradybcio@kernel.org>, 
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] drm/panel: Add driver for EDO RM69380 OLED panel
Message-ID: <e2jkc4gs74pe365danyiq22f7s7wqxvj27jrjlzehgsgigf2dd@gai76zslxr7o>
Mail-Followup-To: Marijn Suijten <marijn.suijten@somainline.org>, 
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, David Wronek <david@mainlining.org>, 
	Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, David Airlie <airlied@gmail.com>, 
	Daniel Vetter <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, 
	Conor Dooley <conor+dt@kernel.org>, Konrad Dybcio <konradybcio@kernel.org>, 
	dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, 
	~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org
References: <20240415-raydium-rm69380-driver-v2-0-524216461306@mainlining.org>
 <20240415-raydium-rm69380-driver-v2-2-524216461306@mainlining.org>
 <yccnqb63gp5svx76rudfnphecpatkpwgp2wlfenojebulrigma@xfztf3ox6q7d>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <yccnqb63gp5svx76rudfnphecpatkpwgp2wlfenojebulrigma@xfztf3ox6q7d>

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