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

[Marek Vasut <marek.vasut@mailbox.org>]

On 11/5/25 7:03 PM, Luca Ceresoli wrote:

Hello Luca,

sorry for the late reply.

>>> 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.

Fixed in V3

>>>> +	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.
Binding update sent:

https://lore.kernel.org/dri-devel/20260104213457.128734-1-marek.vasut@mailbox.org/