Re: [PATCH v5 1/7] dt-bindings: Add panel-timing subnode to simple-panel

[Doug Anderson <dianders@chromium.org>]

Hi,

On Sun, Jun 30, 2019 at 1:03 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Douglas.
>
> Some long overdue review feedback.
>
> On Mon, Apr 01, 2019 at 10:17:18AM -0700, Douglas Anderson wrote:
> > From: Sean Paul <seanpaul@chromium.org>
> >
> > This patch adds a new subnode to simple-panel allowing us to override
> > the typical timing expressed in the panel's display_timing.
> >
> > Changes in v2:
> >  - Split out the binding into a new patch (Rob)
> >  - display-timings is a new section (Rob)
> >  - Use the full display-timings subnode instead of picking the timing
> >    out (Rob/Thierry)
> > Changes in v3:
> >  - Go back to using the timing subnode directly, but rename to
> >    panel-timing (Rob)
> > Changes in v4:
> >  - Simplify desc. for when override should be used (Thierry/Laurent)
> >  - Removed Rob H review since it's been a year and wording changed
> > Changes in v5:
> >  - Removed bit about OS may ignore (Rob/Ezequiel)
> >
> > Cc: Doug Anderson <dianders@chromium.org>
> > Cc: Eric Anholt <eric@anholt.net>
> > Cc: Heiko Stuebner <heiko@sntech.de>
> > Cc: Jeffy Chen <jeffy.chen@rock-chips.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Stéphane Marchesin <marcheu@chromium.org>
> > Cc: Thierry Reding <thierry.reding@gmail.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-rockchip@lists.infradead.org
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> >  .../bindings/display/panel/simple-panel.txt   | 22 +++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/panel/simple-panel.txt b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > index b2b872c710f2..93882268c0b9 100644
> > --- a/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > +++ b/Documentation/devicetree/bindings/display/panel/simple-panel.txt
> > @@ -15,6 +15,16 @@ Optional properties:
> >    (hot plug detect) signal, but the signal isn't hooked up so we should
> >    hardcode the max delay from the panel spec when powering up the panel.
> >
> > +panel-timing subnode
> > +--------------------
> > +
> > +This optional subnode is for devices which require a mode differing
> > +from the panel's "typical" display timing.
> Meybe add here that it is expected that the panel has included timing
> in the driver itself, and not as part of DT.
> So what is specified here is a more precise variant, within the limits
> of what is specified for the panel.

See discussions previous versions of this patch.  Specifically you can
see v4 at <https://patchwork.kernel.org/patch/10875505/> and v3
(posted by Sean) at <https://patchwork.kernel.org/patch/10207591/>.

Specifically: According to Rob H it is generally not required to
validate what's in device tree--it can be just blindly applied.  Thus
the bindings shouldn't really say anything about trying to reconcile
with the driver (especially since that's heavily relying on the
current driver implementation).

At the moment the driver still does validate things and we could
discuss removing that in a future patchset if it was deemed important
/ desirable.


> > +Format information on the panel-timing subnode can be found in
> > +display-timing.txt.
> display-timing defines otional properties:
> hsync-active, pixelclk-active, doublescan etc.
> It is not from the above obvious which properties from display-timings
> that can be specified for a panel-timing sub-node.
> Maybe because they can all be specified?
>
> Display-timing allows timings to be specified as a range.
> If it is also OK to specify a range for panle-timing then everythign is
> fine. But if the panel-timign subnode do not allow ranges this needs to
> be specified.

One thing to think about here is that the bindings are a bit divorced
from the real world.  Specifically the bindings should describe
hardware / what's possible and it's OK for bindings to describe things
that aren't yet supported in code.  You've gotta be really careful
here, of course, because it's easy to write ridiculous bindings if
there is no implementation backing them up, but in general that's
supposed to be the idea.

Here it seems like it should be possible to specify timings as a range
and that would be a sensible thing to do.  ...and we're already using
existing code to parse this node, specifically
of_get_display_timing().  If simple-panel can't (yet) handle
reconciling ranges specified in DT then presumably we shouldn't rely
on that yet.  ...but if it becomes useful then we can add it later.
...but it's OK to already have it in the bindings.

Did that make sense?  If I'm misunderstanding something about the
situation then please yell!  :-)

I will also note that perhaps we shouldn't nit-pick too much as per
Rob's comment in the cover letter [1] of v5 of the series.
Specifically he said this binding is going away anyway.

Summary: I think I have no actions here and this could go to drm-misc
with Theirry's Ack plus other tags.


[1] https://lore.kernel.org/patchwork/cover/1057038/