[PATCH 1/6] drivers: core: ofnode: Add panel timing decode.

[Simon Glass]

Hi Nikhl,

On Tue, 24 Jan 2023 at 22:14, Nikhl M Jain <n-jain1 at ti.com> wrote:
>
> Hi Simon,
>
> On 24/01/23 00:12, Simon Glass wrote:
> > Hi Nikhil,
> >
> > On Mon, 23 Jan 2023 at 01:07, Nikhil M Jain <n-jain1 at ti.com> wrote:
> >>
> >> ofnode_decode_display_timing supports reading timing parameters from
> >> subnode of display-timings node, for displays supporting multiple
> >> resolution, in case if a display supports single resolution, it fails
> >> reading directly from display-timings node, to support it
> >> ofnode_decode_panel_timing is added.
> >>
> >> Signed-off-by: Nikhil M Jain <n-jain1 at ti.com>
> >> ---
> >>  drivers/core/ofnode.c | 53 +++++++++++++++++++++++++++++++++++++++++++
> >>  include/dm/ofnode.h   | 12 ++++++++++
> >>  2 files changed, 65 insertions(+)
> >>
> >> diff --git a/drivers/core/ofnode.c b/drivers/core/ofnode.c
> >> index 4d56b1a767..d06b947516 100644
> >> --- a/drivers/core/ofnode.c
> >> +++ b/drivers/core/ofnode.c
> >> @@ -991,6 +991,59 @@ int ofnode_decode_display_timing(ofnode parent, int index,
> >>         return ret;
> >>  }
> >>
> >> +int ofnode_decode_panel_timing(ofnode parent,
> >> +                              struct display_timing *dt)
> >> +{
> >> +       ofnode timings;
> >> +       u32 val = 0;
> >> +       int ret = 0;
> >> +
> >> +       timings = ofnode_find_subnode(parent, "panel-timings");
> >> +
> >> +       if (!ofnode_valid(timings))
> >> +               return -EINVAL;
> >> +       memset(dt, 0, sizeof(*dt));
> >> +       ret |= decode_timing_property(timings, "hback-porch", &dt->hback_porch);
> >> +       ret |= decode_timing_property(timings, "hfront-porch", &dt->hfront_porch);
> >> +       ret |= decode_timing_property(timings, "hactive", &dt->hactive);
> >> +       ret |= decode_timing_property(timings, "hsync-len", &dt->hsync_len);
> >> +       ret |= decode_timing_property(timings, "vback-porch", &dt->vback_porch);
> >> +       ret |= decode_timing_property(timings, "vfront-porch", &dt->vfront_porch);
> >> +       ret |= decode_timing_property(timings, "vactive", &dt->vactive);
> >> +       ret |= decode_timing_property(timings, "vsync-len", &dt->vsync_len);
> >> +       ret |= decode_timing_property(timings, "clock-frequency", &dt->pixelclock);
> >> +       dt->flags = 0;
> >> +       val = ofnode_read_u32_default(timings, "vsync-active", -1);
> >> +       if (val != -1) {
> >
> > if (!ofnode_read_u32(timings, "vsync-active", &val)) {
> >
> > Please fix below also
> >
> >> +               dt->flags |= val ? DISPLAY_FLAGS_VSYNC_HIGH :
> >> +                               DISPLAY_FLAGS_VSYNC_LOW;
> >> +       }
> >> +       val = ofnode_read_u32_default(timings, "hsync-active", -1);
> >> +       if (val != -1) {
> >> +               dt->flags |= val ? DISPLAY_FLAGS_HSYNC_HIGH :
> >> +                               DISPLAY_FLAGS_HSYNC_LOW;
> >> +       }
> >> +       val = ofnode_read_u32_default(timings, "de-active", -1);
> >> +       if (val != -1) {
> >> +               dt->flags |= val ? DISPLAY_FLAGS_DE_HIGH :
> >> +                               DISPLAY_FLAGS_DE_LOW;
> >> +       }
> >> +       val = ofnode_read_u32_default(timings, "pixelclk-active", -1);
> >> +       if (val != -1) {
> >> +               dt->flags |= val ? DISPLAY_FLAGS_PIXDATA_POSEDGE :
> >> +                               DISPLAY_FLAGS_PIXDATA_NEGEDGE;
> >> +       }
> >> +
> >> +       if (ofnode_read_bool(timings, "interlaced"))
> >> +               dt->flags |= DISPLAY_FLAGS_INTERLACED;
> >> +       if (ofnode_read_bool(timings, "doublescan"))
> >> +               dt->flags |= DISPLAY_FLAGS_DOUBLESCAN;
> >> +       if (ofnode_read_bool(timings, "doubleclk"))
> >> +               dt->flags |= DISPLAY_FLAGS_DOUBLECLK;
> >> +
> >> +       return ret;
> >> +}
> >> +
> >>  const void *ofnode_get_property(ofnode node, const char *propname, int *lenp)
> >>  {
> >>         if (ofnode_is_np(node))
> >> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h
> >> index fa9865602d..3f6b0843c5 100644
> >> --- a/include/dm/ofnode.h
> >> +++ b/include/dm/ofnode.h
> >> @@ -974,6 +974,18 @@ struct display_timing;
> >>  int ofnode_decode_display_timing(ofnode node, int index,
> >>                                  struct display_timing *config);
> >>
> >> +/**
> >> + * ofnode_decode_panel_timing() - decode display timings
> >> + *
> >> + * Decode panel timings from the supplied 'panel-timings' node.
> >> + *
> >> + * @node:      'display-timing' node containing the timing subnodes
> >> + * @config:    Place to put timings
> >> + * Return: 0 if OK, -FDT_ERR_NOTFOUND if not found
> >> + */
> >> +int ofnode_decode_panel_timing(ofnode node,
> >> +                              struct display_timing *config);
> >> +
> >>  /**
> >>   * ofnode_get_property() - get a pointer to the value of a node property
> >>   *
> >> --
> >> 2.17.1
> >>
> >
> > Please add a test to test/dm/ofnode.c
> >
> Wanted to confirm whether I need to add a test for ofnode_get_property
> or for the ofnode_decode_panel_timings.

I mean for ofnode_decode_panel_timing().

You could certainly add a separate test for ofnode_get_property() too.
It is somewhat implicitly tested by things like dm_test_read_int(),
but more direct tests are always useful.

Regards,
Simon