Hi, On Sun, Jun 30, 2019 at 1:03 PM Sam Ravnborg <sam@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxx> > > Cc: Eric Anholt <eric@xxxxxxxxxx> > > Cc: Heiko Stuebner <heiko@xxxxxxxxx> > > Cc: Jeffy Chen <jeffy.chen@xxxxxxxxxxxxxx> > > Cc: Rob Herring <robh+dt@xxxxxxxxxx> > > Cc: Stéphane Marchesin <marcheu@xxxxxxxxxxxx> > > Cc: Thierry Reding <thierry.reding@xxxxxxxxx> > > Cc: devicetree@xxxxxxxxxxxxxxx > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx > > Cc: linux-rockchip@xxxxxxxxxxxxxxxxxxx > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> > > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx> > > --- > > > > .../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/
