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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4744 bytes --]

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@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@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@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@foss.st.com>
> > > > > > Reviewed-by: Yannick Fertre <yannick.fertre@foss.st.com>
> > > > > > Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]