| Message ID | 1479526093-7014-2-git-send-email-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > Document properties common to several display panels in a central > location that can be referenced by the panel device tree bindings. > Looks good. Just one comment... [...] > +Connectivity > +------------ > + > +- ports: Panels receive video data through one or multiple connections. While > + the nature of those connections is specific to the panel type, the > + connectivity is expressed in a standard fashion using ports as specified in > + the device graph bindings defined in > + Documentation/devicetree/bindings/graph.txt. We allow panels to either use graph binding or be a child of the display controller. Using the graph is preferred, but in the simple cases just a child node is sufficient. This should be described here or somewhere in this doc. Rob
Hi Rob, On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: > On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > > Document properties common to several display panels in a central > > location that can be referenced by the panel device tree bindings. > > Looks good. Just one comment... > > [...] > > > +Connectivity > > +------------ > > + > > +- ports: Panels receive video data through one or multiple connections. > > While > > + the nature of those connections is specific to the panel type, the > > + connectivity is expressed in a standard fashion using ports as > > specified in > > + the device graph bindings defined in > > + Documentation/devicetree/bindings/graph.txt. > > We allow panels to either use graph binding or be a child of the display > controller. I knew that some display controllers use a phandle to the panel (see the fsl,panel and nvidia,panel properties), but I didn't know we had panels as children of display controller nodes. I don't think we should allow that for anything but DSI panels, as the DT hierarchy is based on control buses. Are you sure we have other panels instantiated through that mechanism ? > Using the graph is preferred, but in the simple cases just a child node is > sufficient. This should be described here or somewhere in this doc.
On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > Document properties common to several display panels in a central > location that can be referenced by the panel device tree bindings. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > .../bindings/display/panel/panel-common.txt | 91 ++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/panel/panel-common.txt > > diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.txt b/Documentation/devicetree/bindings/display/panel/panel-common.txt > new file mode 100644 > index 000000000000..ec52c472c845 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.txt > @@ -0,0 +1,91 @@ > +Common Properties for Display Panel > +=================================== > + > +This document defines device tree properties common to several classes of > +display panels. It doesn't constitue a device tree binding specification by > +itself but is meant to be referenced by device tree bindings. > + > +When referenced from panel device tree bindings the properties defined in this > +document are defined as follows. The panel device tree bindings are > +responsible for defining whether each property is required or optional. > + > + > +Descriptive Properties > +---------------------- > + > +- width-mm, > +- height-mm: The width-mm and height-mm specify the width and height of the > + physical area where images are displayed. These properties are expressed in > + millimeters and rounded to the closest unit. Erm... this is already implied by the compatible string. Having this in device tree is completely redundant. > +- label: The label property specifies a symbolic name for the panel as a > + string suitable for use by humans. It typically contains a name inscribed on > + the system (e.g. as an affixed label) or specified in the system's > + documentation (e.g. in the user's manual). > + > + If no such name exists, and unless the property is mandatory according to > + device tree bindings, it shall rather be omitted than constructed of > + non-descriptive information. For instance an LCD panel in a system that > + contains a single panel shall not be labelled "LCD" if that name is not > + inscribed on the system or used in a descriptive fashion in system > + documentation. > + > + > +Display Timings > +--------------- > + > +- panel-timing: Most display panels are restricted to a single resolution and > + require specific display timings. The panel-timing subnode expresses those > + timings as specified in the timing subnode section of the display timing > + bindings defined in > + Documentation/devicetree/bindings/display/display-timing.txt. Why? That's also implied by the compatible string. Honestly, I thought by now we had been over this often enough... Thierry
Hi Thierry, On Tuesday 22 Nov 2016 12:05:48 Thierry Reding wrote: > On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > > Document properties common to several display panels in a central > > location that can be referenced by the panel device tree bindings. > > > > Signed-off-by: Laurent Pinchart > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > .../bindings/display/panel/panel-common.txt | 91 +++++++++++++++++ > > 1 file changed, 91 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/display/panel/panel-common.txt> > > diff --git > > a/Documentation/devicetree/bindings/display/panel/panel-common.txt > > b/Documentation/devicetree/bindings/display/panel/panel-common.txt > > new file mode 100644 > > index 000000000000..ec52c472c845 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/panel/panel-common.txt > > @@ -0,0 +1,91 @@ > > +Common Properties for Display Panel > > +=================================== > > + > > +This document defines device tree properties common to several classes of > > +display panels. It doesn't constitue a device tree binding specification > > by > > +itself but is meant to be referenced by device tree bindings. > > + > > +When referenced from panel device tree bindings the properties defined in > > this > > +document are defined as follows. The panel device tree bindings are > > +responsible for defining whether each property is required or optional. > > + > > + > > +Descriptive Properties > > +---------------------- > > + > > +- width-mm, > > +- height-mm: The width-mm and height-mm specify the width and height of > > the > > + physical area where images are displayed. These properties are > > expressed in > > + millimeters and rounded to the closest unit. > > Erm... this is already implied by the compatible string. Having this in > device tree is completely redundant. Nothing new under the sun here, we already have plenty of properties that could be implied by compatible strings. For instance for SoC IP cores many vendors use both an SoC-specific compatible string and a generic compatible string (e.g. "renesas,gpio-r8a7795" and "renesas,gpio-rcar"). The SoC- compatible string implies register addresses, clocks and interrupts, but we still describe them in DT. At the end of the day information about devices and their integration in the system needs to be available, either from DT or from C code. DT bindings should be designed to strike a good balance there, avoiding redundant information in DT (and thus keeping the bindings simple) while still providing enough information to allow for a reasonable level of genericity in OS implementations. > > +- label: The label property specifies a symbolic name for the panel as a > > + string suitable for use by humans. It typically contains a name > > inscribed on > > + the system (e.g. as an affixed label) or specified in the system's > > + documentation (e.g. in the user's manual). > > + > > + If no such name exists, and unless the property is mandatory according > > to > > + device tree bindings, it shall rather be omitted than constructed of > > + non-descriptive information. For instance an LCD panel in a system that > > + contains a single panel shall not be labelled "LCD" if that name is not > > + inscribed on the system or used in a descriptive fashion in system > > + documentation. > > + > > + > > +Display Timings > > +--------------- > > + > > +- panel-timing: Most display panels are restricted to a single resolution > > and > > + require specific display timings. The panel-timing subnode expresses > > those > > + timings as specified in the timing subnode section of the display > > timing > > + bindings defined in > > + Documentation/devicetree/bindings/display/display-timing.txt. > > Why? That's also implied by the compatible string. Honestly, I thought > by now we had been over this often enough... Same argument as above. I won't try to change your mind and fix the simple panel driver, but I still stand firm on my belief that expressing the size and timings in DT is the right solution in a wide variety of cases (and yes I've read http://sietch-tagr.blogspot.fi/2016/04/display-panels-are-not-special.html, and while I agree with the title, I still believe size and timings in DT are not wrong).
On Tue, Nov 22, 2016 at 5:05 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: >> Document properties common to several display panels in a central >> location that can be referenced by the panel device tree bindings. >> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> >> --- >> .../bindings/display/panel/panel-common.txt | 91 ++++++++++++++++++++++ >> 1 file changed, 91 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/display/panel/panel-common.txt >> >> diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.txt b/Documentation/devicetree/bindings/display/panel/panel-common.txt >> new file mode 100644 >> index 000000000000..ec52c472c845 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/panel/panel-common.txt >> @@ -0,0 +1,91 @@ >> +Common Properties for Display Panel >> +=================================== >> + >> +This document defines device tree properties common to several classes of >> +display panels. It doesn't constitue a device tree binding specification by >> +itself but is meant to be referenced by device tree bindings. >> + >> +When referenced from panel device tree bindings the properties defined in this >> +document are defined as follows. The panel device tree bindings are >> +responsible for defining whether each property is required or optional. >> + >> + >> +Descriptive Properties >> +---------------------- >> + >> +- width-mm, >> +- height-mm: The width-mm and height-mm specify the width and height of the >> + physical area where images are displayed. These properties are expressed in >> + millimeters and rounded to the closest unit. > > Erm... this is already implied by the compatible string. Having this in > device tree is completely redundant. > >> +- label: The label property specifies a symbolic name for the panel as a >> + string suitable for use by humans. It typically contains a name inscribed on >> + the system (e.g. as an affixed label) or specified in the system's >> + documentation (e.g. in the user's manual). >> + >> + If no such name exists, and unless the property is mandatory according to >> + device tree bindings, it shall rather be omitted than constructed of >> + non-descriptive information. For instance an LCD panel in a system that >> + contains a single panel shall not be labelled "LCD" if that name is not >> + inscribed on the system or used in a descriptive fashion in system >> + documentation. >> + >> + >> +Display Timings >> +--------------- >> + >> +- panel-timing: Most display panels are restricted to a single resolution and >> + require specific display timings. The panel-timing subnode expresses those >> + timings as specified in the timing subnode section of the display timing >> + bindings defined in >> + Documentation/devicetree/bindings/display/display-timing.txt. > > Why? That's also implied by the compatible string. Honestly, I thought > by now we had been over this often enough... While I completely agree we don't want *only* generic compatibles nor generic gpio and power control, I think timing values in DT are fine. They are just data copied out of datasheets and aren't tweaked per platform. If the same data would make sense to put into a display EDID, I think it also makes sense to put that data in DT. Rob
Hi Rob, On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: > On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: > > On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > >> Document properties common to several display panels in a central > >> location that can be referenced by the panel device tree bindings. > > > > Looks good. Just one comment... > > > > [...] > > > >> +Connectivity > >> +------------ > >> + > >> +- ports: Panels receive video data through one or multiple connections. > >> While > >> + the nature of those connections is specific to the panel type, the > >> + connectivity is expressed in a standard fashion using ports as > >> specified in > >> + the device graph bindings defined in > >> + Documentation/devicetree/bindings/graph.txt. > > > > We allow panels to either use graph binding or be a child of the display > > controller. > > I knew that some display controllers use a phandle to the panel (see the > fsl,panel and nvidia,panel properties), but I didn't know we had panels as > children of display controller nodes. I don't think we should allow that for > anything but DSI panels, as the DT hierarchy is based on control buses. Are > you sure we have other panels instantiated through that mechanism ? Ping ? Please note that this file documents properties common to multiple panel DT bindings, but in no way makes it mandatory to use the OF graph bindings for panels. The decision is left to individual bindings. > > Using the graph is preferred, but in the simple cases just a child node is > > sufficient. This should be described here or somewhere in this doc.
On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Rob, > > On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: >> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: >> > On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: >> >> Document properties common to several display panels in a central >> >> location that can be referenced by the panel device tree bindings. >> > >> > Looks good. Just one comment... >> > >> > [...] >> > >> >> +Connectivity >> >> +------------ >> >> + >> >> +- ports: Panels receive video data through one or multiple connections. >> >> While >> >> + the nature of those connections is specific to the panel type, the >> >> + connectivity is expressed in a standard fashion using ports as >> >> specified in >> >> + the device graph bindings defined in >> >> + Documentation/devicetree/bindings/graph.txt. >> > >> > We allow panels to either use graph binding or be a child of the display >> > controller. >> >> I knew that some display controllers use a phandle to the panel (see the >> fsl,panel and nvidia,panel properties), but I didn't know we had panels as >> children of display controller nodes. I don't think we should allow that for >> anything but DSI panels, as the DT hierarchy is based on control buses. Are >> you sure we have other panels instantiated through that mechanism ? Some panels have no control bus, so were do we place them? I would say the hierarchy is based on buses with a preference for the control bus when there are multiple buses. I'm not a fan of just sticking things are the top level. > Ping ? > > Please note that this file documents properties common to multiple panel DT > bindings, but in no way makes it mandatory to use the OF graph bindings for > panels. The decision is left to individual bindings. It is mandatory in the sense that we don't want more cases of "fsl,panel". Rob
Hi Rob, On Tuesday 29 Nov 2016 09:14:09 Rob Herring wrote: > On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart wrote: > > On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: > >> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: > >>> On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > >>>> Document properties common to several display panels in a central > >>>> location that can be referenced by the panel device tree bindings. > >>> > >>> Looks good. Just one comment... > >>> > >>> [...] > >>> > >>>> +Connectivity > >>>> +------------ > >>>> + > >>>> +- ports: Panels receive video data through one or multiple > >>>> connections. While > >>>> + the nature of those connections is specific to the panel type, the > >>>> + connectivity is expressed in a standard fashion using ports as > >>>> specified in > >>>> + the device graph bindings defined in > >>>> + Documentation/devicetree/bindings/graph.txt. > >>> > >>> We allow panels to either use graph binding or be a child of the > >>> display controller. > >> > >> I knew that some display controllers use a phandle to the panel (see the > >> fsl,panel and nvidia,panel properties), but I didn't know we had panels > >> as children of display controller nodes. I don't think we should allow > >> that for anything but DSI panels, as the DT hierarchy is based on control > >> buses. Are you sure we have other panels instantiated through that > >> mechanism ? > > Some panels have no control bus, so were do we place them? I'd say under the root node, like all similar control-less devices. > I would say the hierarchy is based on buses with a preference for the > control bus when there are multiple buses. I'm not a fan of just sticking > things are the top level. OK, so much for my comment a few lines up :-) The problem with placing non-DSI panels as children of the display controller and not using OF graph is that the panel bindings become dependent of the display controller being used. A display controller using OF graph would require the panel to do the same, while a display controller expecting a panel child node (with a specific name) would require DT properties for the panel node. I'm also not sure the complexity of OF graph is really that prohibitive if you compare it to panels as child nodes. To get the panel driver to bind to the panel DT node the display controller driver would need to create a platform device for the panel and register it. That's not very difficult, but parsing a single port and endpoint isn't either (and we could even provide a helper function for that, a version of of_drm_find_panel() that would take as an argument the display controller device node instead of the panel device node). > > Ping ? > > > > Please note that this file documents properties common to multiple panel > > DT bindings, but in no way makes it mandatory to use the OF graph bindings > > for panels. The decision is left to individual bindings. > > It is mandatory in the sense that we don't want more cases of "fsl,panel". That I agree with :-)
Hi Rob, On Tuesday 29 Nov 2016 20:23:41 Laurent Pinchart wrote: > On Tuesday 29 Nov 2016 09:14:09 Rob Herring wrote: > > On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart wrote: > >> On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: > >>> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: > >>>> On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > >>>>> Document properties common to several display panels in a central > >>>>> location that can be referenced by the panel device tree bindings. > >>>> > >>>> Looks good. Just one comment... > >>>> > >>>> [...] > >>>> > >>>>> +Connectivity > >>>>> +------------ > >>>>> + > >>>>> +- ports: Panels receive video data through one or multiple > >>>>> connections. While > >>>>> + the nature of those connections is specific to the panel type, the > >>>>> + connectivity is expressed in a standard fashion using ports as > >>>>> specified in > >>>>> + the device graph bindings defined in > >>>>> + Documentation/devicetree/bindings/graph.txt. > >>>> > >>>> We allow panels to either use graph binding or be a child of the > >>>> display controller. > >>> > >>> I knew that some display controllers use a phandle to the panel (see > >>> the fsl,panel and nvidia,panel properties), but I didn't know we had > >>> panels as children of display controller nodes. I don't think we should > >>> allow that for anything but DSI panels, as the DT hierarchy is based on > >>> control buses. Are you sure we have other panels instantiated through > >>> that mechanism ? > > > > Some panels have no control bus, so were do we place them? > > I'd say under the root node, like all similar control-less devices. > > > I would say the hierarchy is based on buses with a preference for the > > control bus when there are multiple buses. I'm not a fan of just sticking > > things are the top level. > > OK, so much for my comment a few lines up :-) > > The problem with placing non-DSI panels as children of the display > controller and not using OF graph is that the panel bindings become > dependent of the display controller being used. A display controller using > OF graph would require the panel to do the same, while a display controller > expecting a panel child node (with a specific name) would require DT > properties for the panel node. > > I'm also not sure the complexity of OF graph is really that prohibitive if > you compare it to panels as child nodes. To get the panel driver to bind to > the panel DT node the display controller driver would need to create a > platform device for the panel and register it. That's not very difficult, > but parsing a single port and endpoint isn't either (and we could even > provide a helper function for that, a version of of_drm_find_panel() that > would take as an argument the display controller device node instead of the > panel device node). Ping ? I'd like to standardize on one model for panel DT bindings, but I'm not sure that can be achieved given that we already have multiple competing models. In any case, is that blocking to merge this patch ? I only describe one connectivity model here as that's what my panel driver needs, but I have no issue adding more models later when needed. I believe this patch is a good step forward already. > >> Ping ? > >> > >> Please note that this file documents properties common to multiple panel > >> DT bindings, but in no way makes it mandatory to use the OF graph > >> bindings for panels. The decision is left to individual bindings. > > > > It is mandatory in the sense that we don't want more cases of "fsl,panel". > > That I agree with :-)
On Sun, Dec 18, 2016 at 2:54 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Rob, > > On Tuesday 29 Nov 2016 20:23:41 Laurent Pinchart wrote: >> On Tuesday 29 Nov 2016 09:14:09 Rob Herring wrote: >> > On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart wrote: >> >> On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: >> >>> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: >> >>>> On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: >> >>>>> Document properties common to several display panels in a central >> >>>>> location that can be referenced by the panel device tree bindings. >> >>>> >> >>>> Looks good. Just one comment... >> >>>> >> >>>> [...] >> >>>> >> >>>>> +Connectivity >> >>>>> +------------ >> >>>>> + >> >>>>> +- ports: Panels receive video data through one or multiple >> >>>>> connections. While >> >>>>> + the nature of those connections is specific to the panel type, the >> >>>>> + connectivity is expressed in a standard fashion using ports as >> >>>>> specified in >> >>>>> + the device graph bindings defined in >> >>>>> + Documentation/devicetree/bindings/graph.txt. >> >>>> >> >>>> We allow panels to either use graph binding or be a child of the >> >>>> display controller. >> >>> >> >>> I knew that some display controllers use a phandle to the panel (see >> >>> the fsl,panel and nvidia,panel properties), but I didn't know we had >> >>> panels as children of display controller nodes. I don't think we should >> >>> allow that for anything but DSI panels, as the DT hierarchy is based on >> >>> control buses. Are you sure we have other panels instantiated through >> >>> that mechanism ? >> > >> > Some panels have no control bus, so were do we place them? >> >> I'd say under the root node, like all similar control-less devices. >> >> > I would say the hierarchy is based on buses with a preference for the >> > control bus when there are multiple buses. I'm not a fan of just sticking >> > things are the top level. >> >> OK, so much for my comment a few lines up :-) >> >> The problem with placing non-DSI panels as children of the display >> controller and not using OF graph is that the panel bindings become >> dependent of the display controller being used. A display controller using >> OF graph would require the panel to do the same, while a display controller >> expecting a panel child node (with a specific name) would require DT >> properties for the panel node. Not sure I follow. They become dependent on the controller driver to probe the panel, but the contents of the panel node would not be controller dependent. >> I'm also not sure the complexity of OF graph is really that prohibitive if >> you compare it to panels as child nodes. To get the panel driver to bind to >> the panel DT node the display controller driver would need to create a >> platform device for the panel and register it. That's not very difficult, >> but parsing a single port and endpoint isn't either (and we could even >> provide a helper function for that, a version of of_drm_find_panel() that >> would take as an argument the display controller device node instead of the >> panel device node). > > Ping ? > > I'd like to standardize on one model for panel DT bindings, but I'm not sure > that can be achieved given that we already have multiple competing models. In > any case, is that blocking to merge this patch ? I only describe one > connectivity model here as that's what my panel driver needs, but I have no > issue adding more models later when needed. I believe this patch is a good > step forward already. It is an improvement which I appreciate, so yes I guess we can address it later when needed. Rob
Hi Rob, On Monday 19 Dec 2016 09:38:49 Rob Herring wrote: > On Sun, Dec 18, 2016 at 2:54 PM, Laurent Pinchart wrote: > > On Tuesday 29 Nov 2016 20:23:41 Laurent Pinchart wrote: > >> On Tuesday 29 Nov 2016 09:14:09 Rob Herring wrote: > >>> On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart wrote: > >>>> On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: > >>>>> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: > >>>>>> On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: > >>>>>>> Document properties common to several display panels in a central > >>>>>>> location that can be referenced by the panel device tree bindings. > >>>>>> > >>>>>> Looks good. Just one comment... > >>>>>> > >>>>>> [...] > >>>>>> > >>>>>>> +Connectivity > >>>>>>> +------------ > >>>>>>> + > >>>>>>> +- ports: Panels receive video data through one or multiple > >>>>>>> connections. While > >>>>>>> + the nature of those connections is specific to the panel type, > >>>>>>> the > >>>>>>> + connectivity is expressed in a standard fashion using ports as > >>>>>>> specified in > >>>>>>> + the device graph bindings defined in > >>>>>>> + Documentation/devicetree/bindings/graph.txt. > >>>>>> > >>>>>> We allow panels to either use graph binding or be a child of the > >>>>>> display controller. > >>>>> > >>>>> I knew that some display controllers use a phandle to the panel (see > >>>>> the fsl,panel and nvidia,panel properties), but I didn't know we had > >>>>> panels as children of display controller nodes. I don't think we > >>>>> should allow that for anything but DSI panels, as the DT hierarchy is > >>>>> based on control buses. Are you sure we have other panels instantiated > >>>>> through that mechanism ? > >>> > >>> Some panels have no control bus, so were do we place them? > >> > >> I'd say under the root node, like all similar control-less devices. > >> > >>> I would say the hierarchy is based on buses with a preference for the > >>> control bus when there are multiple buses. I'm not a fan of just > >>> sticking things are the top level. > >> > >> OK, so much for my comment a few lines up :-) > >> > >> The problem with placing non-DSI panels as children of the display > >> controller and not using OF graph is that the panel bindings become > >> dependent of the display controller being used. A display controller > >> using OF graph would require the panel to do the same, while a display > >> controller expecting a panel child node (with a specific name) would > >> require DT properties for the panel node. > > Not sure I follow. Sorry, I meant "would not requite DT properties". > They become dependent on the controller driver to probe the panel, but the > contents of the panel node would not be controller dependent. If a display controller uses OF graph then the panel DT node has to declare ports. If the display controller doesn't use OF graph but instead expects the panel to be a direct subnode, or points to the panel using a property such as fsl,panel, then the panel DT node will not have ports. > >> I'm also not sure the complexity of OF graph is really that prohibitive > >> if you compare it to panels as child nodes. To get the panel driver to > >> bind to the panel DT node the display controller driver would need to > >> create a platform device for the panel and register it. That's not very > >> difficult, but parsing a single port and endpoint isn't either (and we > >> could even provide a helper function for that, a version of > >> of_drm_find_panel() that would take as an argument the display controller > >> device node instead of the panel device node). > > > > Ping ? > > > > I'd like to standardize on one model for panel DT bindings, but I'm not > > sure that can be achieved given that we already have multiple competing > > models. In any case, is that blocking to merge this patch ? I only > > describe one connectivity model here as that's what my panel driver > > needs, but I have no issue adding more models later when needed. I > > believe this patch is a good step forward already. > > It is an improvement which I appreciate, so yes I guess we can address > it later when needed. Thank you. Can I get your ack then ? :-)
On Mon, Dec 19, 2016 at 10:54 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Rob, > > On Monday 19 Dec 2016 09:38:49 Rob Herring wrote: >> On Sun, Dec 18, 2016 at 2:54 PM, Laurent Pinchart wrote: >> > On Tuesday 29 Nov 2016 20:23:41 Laurent Pinchart wrote: >> >> On Tuesday 29 Nov 2016 09:14:09 Rob Herring wrote: >> >>> On Tue, Nov 29, 2016 at 2:27 AM, Laurent Pinchart wrote: >> >>>> On Tuesday 22 Nov 2016 11:36:55 Laurent Pinchart wrote: >> >>>>> On Monday 21 Nov 2016 10:48:15 Rob Herring wrote: >> >>>>>> On Sat, Nov 19, 2016 at 05:28:01AM +0200, Laurent Pinchart wrote: >> >>>>>>> Document properties common to several display panels in a central >> >>>>>>> location that can be referenced by the panel device tree bindings. >> >>>>>> >> >>>>>> Looks good. Just one comment... >> >>>>>> >> >>>>>> [...] >> >>>>>> >> >>>>>>> +Connectivity >> >>>>>>> +------------ >> >>>>>>> + >> >>>>>>> +- ports: Panels receive video data through one or multiple >> >>>>>>> connections. While >> >>>>>>> + the nature of those connections is specific to the panel type, >> >>>>>>> the >> >>>>>>> + connectivity is expressed in a standard fashion using ports as >> >>>>>>> specified in >> >>>>>>> + the device graph bindings defined in >> >>>>>>> + Documentation/devicetree/bindings/graph.txt. >> >>>>>> >> >>>>>> We allow panels to either use graph binding or be a child of the >> >>>>>> display controller. >> >>>>> >> >>>>> I knew that some display controllers use a phandle to the panel (see >> >>>>> the fsl,panel and nvidia,panel properties), but I didn't know we had >> >>>>> panels as children of display controller nodes. I don't think we >> >>>>> should allow that for anything but DSI panels, as the DT hierarchy is >> >>>>> based on control buses. Are you sure we have other panels instantiated >> >>>>> through that mechanism ? >> >>> >> >>> Some panels have no control bus, so were do we place them? >> >> >> >> I'd say under the root node, like all similar control-less devices. >> >> >> >>> I would say the hierarchy is based on buses with a preference for the >> >>> control bus when there are multiple buses. I'm not a fan of just >> >>> sticking things are the top level. >> >> >> >> OK, so much for my comment a few lines up :-) >> >> >> >> The problem with placing non-DSI panels as children of the display >> >> controller and not using OF graph is that the panel bindings become >> >> dependent of the display controller being used. A display controller >> >> using OF graph would require the panel to do the same, while a display >> >> controller expecting a panel child node (with a specific name) would >> >> require DT properties for the panel node. >> >> Not sure I follow. > > Sorry, I meant "would not requite DT properties". > >> They become dependent on the controller driver to probe the panel, but the >> contents of the panel node would not be controller dependent. > > If a display controller uses OF graph then the panel DT node has to declare > ports. If the display controller doesn't use OF graph but instead expects the > panel to be a direct subnode, or points to the panel using a property such as > fsl,panel, then the panel DT node will not have ports. The controller should just ask for the panel via a common function. That function should then look for either a child node (called 'panel') or a graph port. Of course, for more complex cases, only OF graph may work. It really just the simple case of a controller with a single output and single panel that I'm talking about. >> >> I'm also not sure the complexity of OF graph is really that prohibitive >> >> if you compare it to panels as child nodes. To get the panel driver to >> >> bind to the panel DT node the display controller driver would need to >> >> create a platform device for the panel and register it. That's not very >> >> difficult, but parsing a single port and endpoint isn't either (and we >> >> could even provide a helper function for that, a version of >> >> of_drm_find_panel() that would take as an argument the display controller >> >> device node instead of the panel device node). >> > >> > Ping ? >> > >> > I'd like to standardize on one model for panel DT bindings, but I'm not >> > sure that can be achieved given that we already have multiple competing >> > models. In any case, is that blocking to merge this patch ? I only >> > describe one connectivity model here as that's what my panel driver >> > needs, but I have no issue adding more models later when needed. I >> > believe this patch is a good step forward already. >> >> It is an improvement which I appreciate, so yes I guess we can address >> it later when needed. > > Thank you. Can I get your ack then ? :-) Yes. Acked-by: Rob Herring <robh@kernel.org>
Hi Laurent, Pardon for reviving this old thread. I've noticed a couple of things which might want some attention. On 19 November 2016 at 03:28, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > + > +- panel-timing: Most display panels are restricted to a single resolution and > + require specific display timings. The panel-timing subnode expresses those > + timings as specified in the timing subnode section of the display timing > + bindings defined in > + Documentation/devicetree/bindings/display/display-timing.txt. Cannot find such a file in linux-next. Perhaps you meant Documentation/devicetree/bindings/display/panel/display-timing.txt? Documentation/devicetree/bindings/display/panel/panel.txt includes a "rotation" property, which we might want to fold here. Regards, Emil
Hi Emil, On Sunday 09 Apr 2017 12:47:01 Emil Velikov wrote: > Hi Laurent, > > Pardon for reviving this old thread. I've noticed a couple of things > which might want some attention. > > On 19 November 2016 at 03:28, Laurent Pinchart wrote: > > + > > +- panel-timing: Most display panels are restricted to a single resolution > > and + require specific display timings. The panel-timing subnode > > expresses those + timings as specified in the timing subnode section of > > the display timing + bindings defined in > > + Documentation/devicetree/bindings/display/display-timing.txt. > > Cannot find such a file in linux-next. Perhaps you meant > Documentation/devicetree/bindings/display/panel/display-timing.txt? Oops. My bad, I'll fix that. Thank you for noticing it. > Documentation/devicetree/bindings/display/panel/panel.txt includes a > "rotation" property, which we might want to fold here. I believe that panel.txt and panel-common.txt were added concurrently. We should indeed merge the two.
diff --git a/Documentation/devicetree/bindings/display/panel/panel-common.txt b/Documentation/devicetree/bindings/display/panel/panel-common.txt new file mode 100644 index 000000000000..ec52c472c845 --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/panel-common.txt @@ -0,0 +1,91 @@ +Common Properties for Display Panel +=================================== + +This document defines device tree properties common to several classes of +display panels. It doesn't constitue a device tree binding specification by +itself but is meant to be referenced by device tree bindings. + +When referenced from panel device tree bindings the properties defined in this +document are defined as follows. The panel device tree bindings are +responsible for defining whether each property is required or optional. + + +Descriptive Properties +---------------------- + +- width-mm, +- height-mm: The width-mm and height-mm specify the width and height of the + physical area where images are displayed. These properties are expressed in + millimeters and rounded to the closest unit. + +- label: The label property specifies a symbolic name for the panel as a + string suitable for use by humans. It typically contains a name inscribed on + the system (e.g. as an affixed label) or specified in the system's + documentation (e.g. in the user's manual). + + If no such name exists, and unless the property is mandatory according to + device tree bindings, it shall rather be omitted than constructed of + non-descriptive information. For instance an LCD panel in a system that + contains a single panel shall not be labelled "LCD" if that name is not + inscribed on the system or used in a descriptive fashion in system + documentation. + + +Display Timings +--------------- + +- panel-timing: Most display panels are restricted to a single resolution and + require specific display timings. The panel-timing subnode expresses those + timings as specified in the timing subnode section of the display timing + bindings defined in + Documentation/devicetree/bindings/display/display-timing.txt. + + +Connectivity +------------ + +- ports: Panels receive video data through one or multiple connections. While + the nature of those connections is specific to the panel type, the + connectivity is expressed in a standard fashion using ports as specified in + the device graph bindings defined in + Documentation/devicetree/bindings/graph.txt. + +- ddc-i2c-bus: Some panels expose EDID information through an I2C-compatible + bus such as DDC2 or E-DDC. For such panels the ddc-i2c-bus contains a + phandle to the system I2C controller connected to that bus. + + +Control I/Os +------------ + +Many display panels can be controlled through pins driven by GPIOs. The nature +and timing of those control signals are device-specific and left for panel +device tree bindings to specify. The following GPIO specifiers can however be +used for panels that implement compatible control signals. + +- enable-gpios: Specifier for a GPIO connected to the panel enable control + signal. The enable signal is active high and enables operation of the panel. + This property can also be used for panels implementing an active low power + down signal, which is a negated version of the enable signal. Active low + enable signals (or active high power down signals) can be supported by + inverting the GPIO specifier polarity flag. + + Note that the enable signal control panel operation only and must not be + confused with a backlight enable signal. + +- reset-gpios: Specifier for a GPIO coonnected to the panel reset control + signal. The reset signal is active low and resets the panel internal logic + while active. Active high reset signals can be supported by inverting the + GPIO specifier polarity flag. + + +Backlight +--------- + +Most display panels include a backlight. Some of them also include a backlight +controller exposed through a control bus such as I2C or DSI. Others expose +backlight control through GPIO, PWM or other signals connected to an external +backlight controller. + +- backlight: For panels whose backlight is controlled by an external backlight + controller, this property contains a phandle that references the controller.
Document properties common to several display panels in a central location that can be referenced by the panel device tree bindings. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- .../bindings/display/panel/panel-common.txt | 91 ++++++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/panel-common.txt