| Message ID | 20190401171724.215780-2-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panel: simple: Add mode support to devicetree | expand |
On Mon, 1 Apr 2019 10:17:18 -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(+) > Reviewed-by: Rob Herring <robh@kernel.org>
On Mon, 1 Apr 2019 10:17:18 -0700 Douglas Anderson <dianders@chromium.org> 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> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > > .../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. > + > +Format information on the panel-timing subnode can be found in > +display-timing.txt. > + > + > Example: > > panel: panel { > @@ -25,4 +35,16 @@ Example: > enable-gpios = <&gpio 90 0>; > > backlight = <&backlight>; > + > + panel-timing { > + clock-frequency = <266604720>; > + hactive = <2400>; > + hfront-porch = <48>; > + hback-porch = <84>; > + hsync-len = <32>; > + vactive = <1600>; > + vfront-porch = <3>; > + vback-porch = <120>; > + vsync-len = <10>; > + }; > };
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 Is there any reason why we need the panel- prefix? This is already part of a panel definition, so it's completely redundant. Why not just name the subnode "timing"? Thierry
Thierry, On Mon, Apr 8, 2019 at 3:32 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > 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 > > Is there any reason why we need the panel- prefix? This is already part > of a panel definition, so it's completely redundant. Why not just name > the subnode "timing"? It was a really long time ago since this patch series was idle for a while, but you previous had similar feedback in v3 but ended up OK with it. See: https://patchwork.kernel.org/patch/10207583/ I believe the original node name came out of some back and forth between Rob and Sean. As far as I can tell, the context is back in <https://patchwork.kernel.org/patch/10203483/>. I think Rob wanted it to follow other similar node names. That all being said, if you feel strongly about it being called "timing" and Rob's OK w/ that too then I'll re-spin the series. -Doug
Thierry, On Mon, Apr 8, 2019 at 7:39 AM Doug Anderson <dianders@chromium.org> wrote: > > Thierry, > > On Mon, Apr 8, 2019 at 3:32 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > 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 > > > > Is there any reason why we need the panel- prefix? This is already part > > of a panel definition, so it's completely redundant. Why not just name > > the subnode "timing"? > > It was a really long time ago since this patch series was idle for a > while, but you previous had similar feedback in v3 but ended up OK > with it. See: > > https://patchwork.kernel.org/patch/10207583/ > > I believe the original node name came out of some back and forth > between Rob and Sean. As far as I can tell, the context is back in > <https://patchwork.kernel.org/patch/10203483/>. I think Rob wanted it > to follow other similar node names. > > > That all being said, if you feel strongly about it being called > "timing" and Rob's OK w/ that too then I'll re-spin the series. With 5.2-rc1 out, maybe this series is ready to land? If you'd like me to change things as per above I can. ...but it feels like keeping the already-agreed-upon name might be easiest / best? Presumably you'd land patches 1, 2, 4, and 5 and then Heiko could land the dts patches? Thanks! -Doug
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(+) Sorry for taking so long to get back to this, sounds good to me: Acked-by: Thierry Reding <thierry.reding@gmail.com>
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. > + > +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. > + > + > Example: > > panel: panel { > @@ -25,4 +35,16 @@ Example: > enable-gpios = <&gpio 90 0>; > > backlight = <&backlight>; > + > + panel-timing { > + clock-frequency = <266604720>; > + hactive = <2400>; > + hfront-porch = <48>; > + hback-porch = <84>; > + hsync-len = <32>; > + vactive = <1600>; > + vfront-porch = <3>; > + vback-porch = <120>; > + vsync-len = <10>; > + }; > }; > -- > 2.21.0.392.gf8f6787159e-goog > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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/
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. + +Format information on the panel-timing subnode can be found in +display-timing.txt. + + Example: panel: panel { @@ -25,4 +35,16 @@ Example: enable-gpios = <&gpio 90 0>; backlight = <&backlight>; + + panel-timing { + clock-frequency = <266604720>; + hactive = <2400>; + hfront-porch = <48>; + hback-porch = <84>; + hsync-len = <32>; + vactive = <1600>; + vfront-porch = <3>; + vback-porch = <120>; + vsync-len = <10>; + }; };