Re: [PATCH v2] drm/bridge: fsl-ldb: Parse register offsets from DT

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hi Marek,

On Tue Nov 4, 2025 at 12:08 AM CET, Marek Vasut wrote:
> On 11/3/25 4:55 PM, Luca Ceresoli wrote:
>
> Hello Luca,
>
>> On Sun Nov 2, 2025 at 6:02 PM CET, Marek Vasut wrote:
>>> The DT binding for this bridge describe register offsets for the LDB,
>>> parse the register offsets from DT instead of hard-coding them in the
>>> driver. No functional change.
>>>
>>> Signed-off-by: Marek Vasut <marek.vasut@mailbox.org>
>>
>> I was initially a bit skeptical because normally register offsets are
>> derived from the compatible string, not from device tree. But then I
>> realized this is about the LDB which has its two registers in the
>> MEDIA_BLK. This means all in all this looks somewhat like an integration
>> aspect (the LDB component uses two resources of the MEDIA_CLK component)
>> and your patch mekse sense.
>>
>> So my only remark is that the above may be in the commit message, to make
>> the "why" clear from the beginning. It took a bit of research for me to
>> find out.
>
> Actually, the LDB was always meant to parse the 'reg' offsets out of the
> DT, it then went somewhat ... wrong ... and we ended up with hard-coded
> reg<->compatible mapping. It was never intended to be that way. That is
> all there is to it, there isn't any deeper reason behind it.
>
> What would you add into the commit message ?

The above paragraph is a good draft of what I woudl add.

>> [0] https://lore.kernel.org/dri-devel/20251103-dcif-upstreaming-v6-3-76fcecfda919@oss.nxp.com/
>>
>>> @@ -309,6 +302,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
>>>   	fsl_ldb->dev = &pdev->dev;
>>>   	fsl_ldb->bridge.of_node = dev->of_node;
>>>
>>> +	/* No "reg-names" property likely means single-register LDB */
>>
>> Uh? If it is "likely" it means we are not sure this code is not introducing
>> regressions, and that would be bad.
>
> I can drop the 'likely' part.

If you are sure it's not "likely" but "sure", then OK. However it all
depends on the bindings, which leads to the below question.

>>> +	idx = of_property_match_string(dev->of_node, "reg-names", "ldb");
>>> +	if (idx < 0) {
>>> +		fsl_ldb->single_ctrl_reg = true;
>>> +		idx = 0;
>>> +	}
>>
>>  From the bindings I understand that having two 'reg' values and no
>> 'reg-names' at all is legal. Your patch implies differently. Who's right
>> here?
> I think if you have two two reg values, you should have reg-names , so
> the binding should be updated ?

If the bindings are unclear or ambiguous (or wrong) then they should be
fixed in the first place. With bad bindings we can either have a bad but
compliant implementation or a good but non-compliant implementation.

Best regards,
Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com