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

[Conor Dooley]

On Fri, Feb 27, 2026 at 07:49:33PM +0200, Cristian Ciocaltea wrote:
> On 2/27/26 7:18 PM, Conor Dooley wrote:
> > 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 :
> > 
> >>>>> Deprecating the order also makes little sense to me, given that some of
> >>>>> these devices only have one reg entry, which as far as I can tell from
> >>>>> looking at the driver *is* the "function" region, so it can never be
> >>>>> entirely deprecated.
> >>>>
> >>>> What I'd like to see, is a binding expression that behave like a set, not a
> >>>> list, and leave the ordering open. As people keep repeating, there is nothing in
> >>>> a binding that assist to define the right ordering (its not address or base
> >>>> addres aware). That basically means, we can't as reviewer see that ordering is
> >>>> going to imposing using a base address in the unit name (which is a convenience,
> >>>> not a rule I suppose) that differ from the vendor documented base address.
> >>>>
> >>>> By explicitly removing the ordering in the binding, we create a strict rule that
> >>>> driver should retrieve this by name, and never assume the ordering, which I
> >>>> personally like.
> >>>>
> >>>> thoughts ?
> >>>
> >>> Yeah, you can do this, but to avoid potential breaks you have to do it
> >>> from the start, not after the fact. Probably there's bindings that get
> >>> acked every day that do do this. Even the retcon is okay to do when
> >>> reg-names is mandated by the binding and the users use reg-names in my
> >>> opinion.
> >>
> >> I think from the above analyses, since the usage only starts in rc1, we have
> >> room for improving it knowing we aren't creating problem for anyone. Note that I
> >> have no idea what the syntax is to "do this", and I doubt either Detlev or
> >> Cristian have a clue.
> > 
> > I think this is the only bit that really still needs a reply, this can
> > be solved by adding reg-names as "required" to the existing conditional
> > portion of the binding. There's probably hundreds of examples if one
> > does a search for "then:\n.*required:" to use a basis for the change
> > here. Probably should be an independent change, since it is needed even
> > without the re-order given the bug I brought up.
> 
> As mentioned in my previous reply, the actual problem is that the binding has
> been already released, and I'm not sure we can change this without breaking the
> ABI.

I feel like I am losing my mind here lol. Forget about this patch for a
moment and consider the driver right now. The code currently looks like
this:
	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);
	}
This means you will fail to probe on any platform that does not have
has_single_reg_region set if the reg-names property is not present.
rk3588-vdec uses:
	static const struct rkvdec_variant vdpu381_variant = {
		.coded_fmts = vdpu381_coded_fmts,
		.num_coded_fmts = ARRAY_SIZE(vdpu381_coded_fmts),
		.rcb_sizes = vdpu381_rcb_sizes,
		.num_rcb_sizes = ARRAY_SIZE(vdpu381_rcb_sizes),
		.ops = &vdpu381_variant_ops,
	};
The binding does not currently require rk3588-vdec use reg-names. This
means that the driver will fail to probe if someone provides what the
binding considers to be a valid devicetree.

There are two ways to resolve this. The first is to do the following:
| diff --git a/drivers/media/platform/rockchip/rkvdec/rkvdec.c b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
| index 967c452ab61f7..f508fc4746a87 100644
| --- a/drivers/media/platform/rockchip/rkvdec/rkvdec.c
| +++ b/drivers/media/platform/rockchip/rkvdec/rkvdec.c
| @@ -1800,11 +1800,11 @@ static int rkvdec_probe(struct platform_device *pdev)
|  		if (IS_ERR(rkvdec->regs))
|  			return PTR_ERR(rkvdec->regs);
|  	} else {
| -		rkvdec->regs = devm_platform_ioremap_resource_byname(pdev, "function");
| +		rkvdec->regs = devm_platform_ioremap_resource(pdev, 0);
|  		if (IS_ERR(rkvdec->regs))
|  			return PTR_ERR(rkvdec->regs);
|  
| -		rkvdec->link = devm_platform_ioremap_resource_byname(pdev, "link");
| +		rkvdec->link = devm_platform_ioremap_resource(pdev, 1);
|  		if (IS_ERR(rkvdec->link))
|  			return PTR_ERR(rkvdec->link);
|  	}

The second is to make the reg-names property mandatory.

The first way does not constitute an ABI break. The second way is an ABI
break, as you rightly point out.

Now thinking about your patch here. Without it, the following is a valid
vdec node on rk3588.
	video-codec@fdc38000 {
		compatible = "rockchip,rk3588-vdec";
		reg = <0x0 0xfdc38100 0x0 0x500>,
		      <0x0 0xfdc38000 0x0 0x100>,
		      <0x0 0xfdc38600 0x0 0x100>;
		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
		assigned-clock-rates = <800000000>, <600000000>,
				       <600000000>, <1000000000>;
		iommus = <&vdec0_mmu>;
		power-domains = <&power RK3588_PD_RKVDEC0>;
		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
		reset-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
		sram = <&vdec0_sram>;
	};
Ignore the fact that this will not currently probe for a moment, assuming
we applied my diff above to the driver, and look at what will be a valid
node after your patch:
	video-codec@fdc38000 {
		compatible = "rockchip,rk3588-vdec";
		reg = <0x0 0xfdc38000 0x0 0x100>,
		      <0x0 0xfdc38100 0x0 0x500>,
		      <0x0 0xfdc38600 0x0 0x100>;
		interrupts = <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH 0>;
		clocks = <&cru ACLK_RKVDEC0>, <&cru HCLK_RKVDEC0>, <&cru CLK_RKVDEC0_CA>,
			 <&cru CLK_RKVDEC0_CORE>, <&cru CLK_RKVDEC0_HEVC_CA>;
		clock-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
		assigned-clocks = <&cru ACLK_RKVDEC0>, <&cru CLK_RKVDEC0_CORE>,
				  <&cru CLK_RKVDEC0_CA>, <&cru CLK_RKVDEC0_HEVC_CA>;
		assigned-clock-rates = <800000000>, <600000000>,
				       <600000000>, <1000000000>;
		iommus = <&vdec0_mmu>;
		power-domains = <&power RK3588_PD_RKVDEC0>;
		resets = <&cru SRST_A_RKVDEC0>, <&cru SRST_H_RKVDEC0>, <&cru SRST_RKVDEC0_CA>,
			 <&cru SRST_RKVDEC0_CORE>, <&cru SRST_RKVDEC0_HEVC_CA>;
		reset-names = "axi", "ahb", "cabac", "core", "hevc_cabac";
		sram = <&vdec0_sram>;
	};
Driver is going to break here, cos it will pick up the link region and
set rkvdec->regs to it!

So, the only way to accommodate the deprecated scheme and the new scheme
is to use the reg-names property (as the driver currently does).

So yes, while what I propose is an ABI break, the driver currently
expects reg-names to be mandatory for the rk3588-vdec. Additionally, new
required properties are only really a meaningful ABI break if the driver
is changed to required them, since that would render old devicetrees
non-functional. The driver in question already requires them, so that's
pretty moot! Were it not for your patch here, I would say that my diff
should be applied to the driver instead of making the property required.

reg-names is a dependency for your ABI-annihilation, as my example above
demonstrates, so I find it really bemusing that you're worried about its
impact!

Hopefully I've made my point about reg-names being mandatory this time
around?

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature