Hi Raphale, On Tue, 4 Nov 2025 at 17:55, Tom Rini <[email protected]> wrote: > > On Tue, Nov 04, 2025 at 05:31:21PM +0100, Simon Glass wrote: > > Hi Tom, > > > > On Mon, 3 Nov 2025 at 15:17, Tom Rini <[email protected]> wrote: > > > > > > On Sun, Nov 02, 2025 at 08:53:43PM +0100, Simon Glass wrote: > > > > Hi Raphael, > > > > > > > > On Sun, 2 Nov 2025 at 02:10, Raphaël Gallais-Pou <[email protected]> > > > > wrote: > > > > > > > > > > Le Sat, Nov 01, 2025 at 10:03:59AM +0100, Simon Glass a écrit : > > > > > > Hi Raphael, > > > > > > > > > > > > On Thu, 4 Sept 2025 at 14:53, Raphael Gallais-Pou > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > The "Display Timings" in panel-common.yaml can be provided by 2 > > > > properties > > > > > > > - panel-timing: when display panels are restricted to a single > > > > resolution > > > > > > > the "panel-timing" node expresses the required > > > > timings. > > > > > > > - display-timings: several resolutions with different timings are > > > > supported > > > > > > > with several timing subnode of > > > > > > > "display-timings" > > > > node > > > > > > > > > > > > > > This patch update the parsing function to handle this 2 > > > > > > > possibility > > > > > > > when index = 0. > > > > > > > > > > > > > > Reviewed-by: Patrice Chotard <[email protected]> > > > > > > > Reviewed-by: Yannick Fertre <[email protected]> > > > > > > > Signed-off-by: Raphael Gallais-Pou > > > > > > > <[email protected]> > > > > > > > --- > > > > > > > drivers/core/ofnode.c | 17 ++++++++++------- > > > > > > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c > > > > > > > index > > > > e040e3f2806ffe74c58dcd82f36307351acd5a99..5a721b46e5a3214e7bd437739776362c2d22a3c9 > > > > 100644 > > > > > > > --- a/drivers/core/ofnode.c > > > > > > > +++ b/drivers/core/ofnode.c > > > > > > > @@ -1221,13 +1221,16 @@ int ofnode_decode_display_timing(ofnode > > > > parent, int index, > > > > > > > int ret = 0; > > > > > > > > > > > > > > timings = ofnode_find_subnode(parent, "display-timings"); > > > > > > > - if (!ofnode_valid(timings)) > > > > > > > - return -EINVAL; > > > > > > > - > > > > > > > - i = 0; > > > > > > > - ofnode_for_each_subnode(node, timings) { > > > > > > > - if (i++ == index) > > > > > > > - break; > > > > > > > + if (ofnode_valid(timings)) { > > > > > > > + i = 0; > > > > > > > + ofnode_for_each_subnode(node, timings) { > > > > > > > + if (i++ == index) > > > > > > > + break; > > > > > > > + } > > > > > > > + } else { > > > > > > > + if (index != 0) > > > > > > > + return -EINVAL; > > > > > > > + node = ofnode_find_subnode(parent, > > > > > > > "panel-timing"); > > > > > > > } > > > > > > > > > > > > > > if (!ofnode_valid(node)) > > > > > > > > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > Please add a test for this in test/dm/ofnode.c > > > > > > > > > > Hi Simon, > > > > > > > > > > I'll gladly do that, but I haven't write and use any test in U-Boot. > > > > > So > > > > > it is a bit foggy how to implement it. > > > > > > > > There is some info here: > > > > > > > > https://docs.u-boot.org/en/latest/develop/testing.html > > > > > > > > > > > > > > Do we want to create a fake device-tree and test each configuration or > > > > > do we want to test in the _current_ device-tree if timings are > > > > > correctly > > > > > set according to the index value ? > > > > > > > > It looks like there is a 'display-timings' node in test.dts, with three > > > > subnodes, so you should just be able to get an ofnode for that and then > > > > read out one of them and check it. > > > > > > OK, but what is the utility in doing that? We don't, and aren't, going > > > to have tests for every valid possible DT node, and this isn't > > > introducing new library parsing functionality (the most recent patch to > > > test/dm/ofnode.c was for ofnode_graph and that is important to test). We > > > don't have display-timing tests to start with, so we're fine not adding > > > something more here. > > > > The utility is that code is tested, so it works now and doesn't break > > later. For ofnode we do have tests - see test/dm/ofnode.c > > That is a circular and unsatisfying answer to what I said. I did read > test/dm/ofnode.c and then re-read the patch and don't see any value in > adding nodes and then reading nodes, but gave an example of what kind of > changes do make sense to add tests for because they add value. And in > the interest of not having yet another seemingly infinite thread with > you, this is all I'm going to further add here.
The nodes exist in test.dts so it should just be a case of calling ofnode_decode_display_timing() with a given index and checking what is returned. That would be enough to test your code, if you'd like to. Regards, SImon
