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 <[email protected]> >> >> 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/[email protected]/ >> >>> @@ -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
