[Uboot-stm32] [PATCH v3 1/7] ofnode: support panel-timings in ofnode_decode_display_timing

[Simon Glass]

Hi Raphale,

On Tue, 4 Nov 2025 at 17:55, Tom Rini <trini at konsulko.com> 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 <trini at konsulko.com> 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 <rgallaispou at gmail.com>
> > > > 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
> > > > > > <raphael.gallais-pou at foss.st.com> 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 <patrice.chotard at foss.st.com>
> > > > > > > Reviewed-by: Yannick Fertre <yannick.fertre at foss.st.com>
> > > > > > > Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou at foss.st.com>
> > > > > > > ---
> > > > > > >  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