Re: [PATCH v4 1/3] media: dt-bindings: rockchip,vdec: Add alternative reg-names order for RK35{76,88}

From: Conor Dooley <conor@kernel.org>
To: Nicolas Dufresne <nicolas.dufresne@collabora.com>
Cc: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
	Rob Herring <robh@kernel.org>,
	Krzysztof Kozlowski <krzk+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Detlev Casanova <detlev.casanova@collabora.com>,
	Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil@kernel.org>,
	kernel@collabora.com, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
	Conor Dooley <conor.dooley@microchip.com>,
	linux-media@vger.kernel.org
Subject: Re: [PATCH v4 1/3] media: dt-bindings: rockchip,vdec: Add alternative reg-names order for RK35{76,88}
Date: Thu, 26 Feb 2026 22:15:49 +0000	[thread overview]
Message-ID: <20260226-ferocious-saturday-0e1f9bb28ec5@spud> (raw)
In-Reply-To: <3d28c699e47f606bad46bb6447785badace37793.camel@collabora.com>

[-- Attachment #1: Type: text/plain, Size: 1883 bytes --]

On Thu, Feb 26, 2026 at 04:56:30PM -0500, Nicolas Dufresne wrote:
> Le jeudi 26 février 2026 à 20:59 +0000, Conor Dooley a écrit :
> > On Thu, Feb 26, 2026 at 02:45:11PM -0500, Nicolas Dufresne wrote:
> > > Le jeudi 26 février 2026 à 18:43 +0000, Conor Dooley a écrit :
> > In this case, the driver is currently buggy, because, as I mentioned, it
> > uses reg-names without reg-names being required on the platforms with
> > more than 1 reg property. Probably the binding should make reg-names
> > mandatory for these platforms even without this patch, but it *has* to
> > IMO for this proposed change to be applicable.
> 
> That forced me to check the driver. So for RK33xx and older, there is only one
> range, and the driver will just pick the one entry expected:
> 
> 
> 	if (rkvdec->variant->has_single_reg_region) {
> 		rkvdec->regs = devm_platform_ioremap_resource(pdev, 0);
> 		if (IS_ERR(rkvdec->regs))
> 			return PTR_ERR(rkvdec->regs);
> 	} else {
> 		rkvdec->regs = devm_platform_ioremap_resource_byname(pdev, "function");
> 		if (IS_ERR(rkvdec->regs))
> 			return PTR_ERR(rkvdec->regs);
> 
> 		rkvdec->link = devm_platform_ioremap_resource_byname(pdev, "link");
> 		if (IS_ERR(rkvdec->link))
> 			return PTR_ERR(rkvdec->link);
> 	}
> 
> 
> Where for RK35xx variants, it only pick the resources by name. I don't see the
> bug that you see, but I believe this was just a supposition, that you didn't
> check the code.

Busy reading path of exile patch notes, so sniping this comment only...

This is a bug, not a supposition, and it's that snippet from the
driver that prompted my comment.. That code requires that if
->has_single_reg_region is set that the dts provides reg-names, but the
binding does not mandate reg-names for rk3576-vdec and rk3588-vdec, so
the driver will fail to probe on a dts that the binding says is valid.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]