| Message ID | 20180618132242.8673-3-tomi.valkeinen@ti.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Tomi, Thank you for the patch. On Monday, 18 June 2018 16:22:35 EEST Tomi Valkeinen wrote: > Add DT bindings for Texas Instruments K2G SoC Display Subsystem. The DSS > is quite simple, with a single plane and a single output. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: devicetree@vger.kernel.org > Reviewed-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/display/ti/ti,k2g-dss.txt | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt > > diff --git a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt > b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt new file mode > 100644 > index 000000000000..1af11425eda3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt > @@ -0,0 +1,15 @@ > +Texas Instruments K2G Display Subsystem > +======================================= > + > +Required properties: > +- compatible: "ti,k2g-dss" > +- reg: address and length of the register spaces for DSS submodules > +- reg-names: "cfg", "common", "vid1", "ovr1", "vp1" When seeing multiple register ranges for a DT node I always suspect that we describe multiple IP cores that could be better modeled as independent nodes. What prompted you not to model the DISPC as a separate DT node (possibly a child of the DSS DT node) ? Furthermore, "cfg" corresponds to the DSS registers, so I wonder whether it shouldn't be named "dss". Similarly, "common" really sounds like DSS common registers, while it relates to the DISPC. > +- clocks: phandle to fclk and vp1 clocks > +- clock-names: "fck", "vp1" > +- interrupts: phandle to the DISPC interrupt > + > +The DSS outputs are described using the device graphs as documented in > +Documentation/devicetree/bindings/graph.txt. K2G DSS has a single DPI > output as > +port 0. Both SPRUHY8H and SPRSP07D document a DPI output and a DBI output. Am I looking at the wrong document ? > + Extra blank line ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 24/07/18 17:29, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Monday, 18 June 2018 16:22:35 EEST Tomi Valkeinen wrote: >> Add DT bindings for Texas Instruments K2G SoC Display Subsystem. The DSS >> is quite simple, with a single plane and a single output. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> >> Cc: devicetree@vger.kernel.org >> Reviewed-by: Rob Herring <robh@kernel.org> >> --- >> .../devicetree/bindings/display/ti/ti,k2g-dss.txt | 15 +++++++++++++++ >> 1 file changed, 15 insertions(+) >> create mode 100644 >> Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt >> >> diff --git a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt >> b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt new file mode >> 100644 >> index 000000000000..1af11425eda3 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt >> @@ -0,0 +1,15 @@ >> +Texas Instruments K2G Display Subsystem >> +======================================= >> + >> +Required properties: >> +- compatible: "ti,k2g-dss" >> +- reg: address and length of the register spaces for DSS submodules >> +- reg-names: "cfg", "common", "vid1", "ovr1", "vp1" > > When seeing multiple register ranges for a DT node I always suspect that we > describe multiple IP cores that could be better modeled as independent nodes. > What prompted you not to model the DISPC as a separate DT node (possibly a > child of the DSS DT node) ? Yes, it's been difficult to figure out the IP boundaries at times. Well, the vid, ovr and vp regions are part of the DISPC IP, that's clear. The question is with DSS (core) and DISPC. On K2G, DSS and DISPC are indeed separate IPs, similar to OMAPs. DSS ("cfg" region here) is just a wrapper and has only a few registers, though. On AM6, that's not the case, and there's only a single IP. The reason to represent K2G DSS as a single DT node is just practicality: there's no benefit that I can see to consider DSS and DISPC as separate IPs, but it does cause problems in the SW side, especially now that we want to support AM6 DSS too, which does not have separate DSS and DISPC IPs. The main problem there was the question about DRM device: is it the DSS or the DISPC? From DRM point of view, the only sane answer is that DISPC is the DRM device, as that's really the display IP and has the interrupts etc. That then leaves the DSS device kind of hanging around. Considering DSS and DISCP as a single device removed all the problems I had. I think omapdrm driver would also be much simpler with this approach. > Furthermore, "cfg" corresponds to the DSS registers, so I wonder whether it > shouldn't be named "dss". Similarly, "common" really sounds like DSS common > registers, while it relates to the DISPC. "cfg" corresponds to the DSSUL_0_CFG in the TRM and "common" to the DISPC_COMMON region. If you consider there to be only one DSS/DISPC IP, then the naming makes more sense, as "cfg" is about the main level config, "common" is about common stuff (to all planes and videoports). >> +- clocks: phandle to fclk and vp1 clocks >> +- clock-names: "fck", "vp1" >> +- interrupts: phandle to the DISPC interrupt >> + >> +The DSS outputs are described using the device graphs as documented in >> +Documentation/devicetree/bindings/graph.txt. K2G DSS has a single DPI >> output as >> +port 0. > > Both SPRUHY8H and SPRSP07D document a DPI output and a DBI output. Am I > looking at the wrong document ? No, you're right, it has DBI. I'm quite sure it was descoped at some point, though, but it seems to be there in the final ones. We don't support DBI in the driver, so we've missed it. So I'll add the node to the DT. Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt new file mode 100644 index 000000000000..1af11425eda3 --- /dev/null +++ b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.txt @@ -0,0 +1,15 @@ +Texas Instruments K2G Display Subsystem +======================================= + +Required properties: +- compatible: "ti,k2g-dss" +- reg: address and length of the register spaces for DSS submodules +- reg-names: "cfg", "common", "vid1", "ovr1", "vp1" +- clocks: phandle to fclk and vp1 clocks +- clock-names: "fck", "vp1" +- interrupts: phandle to the DISPC interrupt + +The DSS outputs are described using the device graphs as documented in +Documentation/devicetree/bindings/graph.txt. K2G DSS has a single DPI output as +port 0. +