| Message ID | 1404751384-5077-7-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Boris, Thank you for the patch. On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote: > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > controller device. > > The HLCDC block provides a single RGB output port, and only supports LCD > panels connection to LCD panels for now. > > The atmel,panel property link the HLCDC RGB output with the LCD panel > connected on this port (note that the HLCDC RGB connector implementation > makes use of the DRM panel framework). > > Connection to other external devices (DRM bridges) might be added later by > mean of a new atmel,xxx (atmel,bridge) property. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > --- > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode > 100644 > index 0000000..594bdb2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > @@ -0,0 +1,59 @@ > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver > + > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > + > +Required properties: > + - compatible: value should be one of the following: > + "atmel,hlcdc-dc" > + - interrupts: the HLCDC interrupt definition > + - pinctrl-names: the pin control state names. Should contain "default", > + "rgb-444", "rgb-565", "rgb-666" and "rgb-888". > + - pinctrl-[0-4]: should contain the pinctrl states described by pinctrl > + names. Do you need to switch between the different pinctrl configurations at runtime, or is the configuration selected from the panel type, which doesn't change ? > + - atmel,panel: Should contain a phandle with 2 parameters. > + The first cell is a phandle to a DRM panel device > + The second cell encodes the RGB mode, which can take the following > values: > + * 0: RGB444 > + * 1: RGB565 > + * 2: RGB666 > + * 3: RGB888 > + The third cell encodes specific flags describing LCD signals > configuration > + (see Atmel's datasheet for a full description of these > fields): > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > + * bit 4: DISPPOL: Display Signal Polarity > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup > Configuration > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold > Configuration > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time If I'm not mistaken, those are HLCDC configuration values that depend on the panel type and characteristics. Shouldn't they then be queries from the panel through the drm_panel API at runtime instead of being specified in DT ? This would likely require extending the drm_panel API. > + > +Example: > + > + hlcdc: hlcdc@f0030000 { > + compatible = "atmel,sama5d3-hlcdc"; > + reg = <0xf0030000 0x2000>; > + clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; > + clock-names = "periph_clk","sys_clk", "slow_clk"; > + status = "disabled"; > + > + hlcdc-display-controller { > + compatible = "atmel,hlcdc-dc"; > + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; > + pinctrl-names = "default", "rgb-444", "rgb-565", "rgb-666", "rgb-888"; > + pinctrl-0 = <&pinctrl_lcd_base>; > + pinctrl-1 = <&pinctrl_lcd_base &pinctrl_lcd_rgb444>; > + pinctrl-2 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>; > + pinctrl-3 = <&pinctrl_lcd_base &pinctrl_lcd_rgb666>; > + pinctrl-4 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>; > + }; > + > + hlcdc_pwm: hlcdc-pwm { > + compatible = "atmel,hlcdc-pwm"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_lcd_pwm>; > + #pwm-cells = <3>; > + }; > + };
Hi Laurent, On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > Thank you for the patch. > > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote: > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > controller device. > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > panels connection to LCD panels for now. > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > connected on this port (note that the HLCDC RGB connector implementation > > makes use of the DRM panel framework). > > > > Connection to other external devices (DRM bridges) might be added later by > > mean of a new atmel,xxx (atmel,bridge) property. > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > --- > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode > > 100644 > > index 0000000..594bdb2 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > @@ -0,0 +1,59 @@ > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver > > + > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > + > > +Required properties: > > + - compatible: value should be one of the following: > > + "atmel,hlcdc-dc" > > + - interrupts: the HLCDC interrupt definition > > + - pinctrl-names: the pin control state names. Should contain "default", > > + "rgb-444", "rgb-565", "rgb-666" and "rgb-888". > > + - pinctrl-[0-4]: should contain the pinctrl states described by pinctrl > > + names. > > Do you need to switch between the different pinctrl configurations at runtime, > or is the configuration selected from the panel type, which doesn't change ? At the moment no, but if we ever need to support different devices on the same RGB connector (actually Atmel's sama5d3xek boards have an RGB to HDMI bridge connected on the same RGB connector) and these devices do not support the same RGB mode (say your LCD panel supports RGB888 and your RGB to HDMI bridge supports RGB555), then depending on the output you select you'll have to change your pinctrl config at runtime. I'd say we could get rid of this runtime pinctrl config as a first step if DT ABI stability was not required. But it is, and I'd like to have a future proof binding to handle these tricky cases when they occurs (if they ever do). Anyway, I'm open to any other alternative that could let me add support for this later on. BTW, is there any reason for not defining an RGB connector type (I'm currently defining HLCDC connector as an LVDS connector) ? > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > + The first cell is a phandle to a DRM panel device > > + The second cell encodes the RGB mode, which can take the following > > values: > > + * 0: RGB444 > > + * 1: RGB565 > > + * 2: RGB666 > > + * 3: RGB888 > > + The third cell encodes specific flags describing LCD signals > > configuration > > + (see Atmel's datasheet for a full description of these > > fields): > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > + * bit 4: DISPPOL: Display Signal Polarity > > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup > > Configuration > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold > > Configuration > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > If I'm not mistaken, those are HLCDC configuration values that depend on the > panel type and characteristics. Shouldn't they then be queries from the panel > through the drm_panel API at runtime instead of being specified in DT ? This > would likely require extending the drm_panel API. HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the GUARDTIME value. Another question I had regarding these flags is whether they were LCD panel specific or a mix of panel and board implementation. Take the VSYNC HSYNC polarity, of course the LCD panel defines what it expects in terms of polarity, but nothing prevents the HW designer from inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ? A solution would be to override some drm_display_mode settings with informations taken from the DT. Thanks for your review. Best Regards, Boris
Hi Boris, On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote: > On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart wrote: > > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote: > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > > controller device. > > > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > > panels connection to LCD panels for now. > > > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > > connected on this port (note that the HLCDC RGB connector implementation > > > makes use of the DRM panel framework). > > > > > > Connection to other external devices (DRM bridges) might be added later > > > by mean of a new atmel,xxx (atmel,bridge) property. > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > --- > > > > > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++ > > > 1 file changed, 59 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode > > > 100644 > > > index 0000000..594bdb2 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > @@ -0,0 +1,59 @@ > > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver > > > + > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD > > > device. > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more > > > details. > > > + > > > +Required properties: > > > + - compatible: value should be one of the following: > > > + "atmel,hlcdc-dc" > > > + - interrupts: the HLCDC interrupt definition > > > + - pinctrl-names: the pin control state names. Should contain > > > "default", > > > + "rgb-444", "rgb-565", "rgb-666" and "rgb-888". > > > + - pinctrl-[0-4]: should contain the pinctrl states described by > > > pinctrl > > > + names. > > > > Do you need to switch between the different pinctrl configurations at > > runtime, or is the configuration selected from the panel type, which > > doesn't change ? > > At the moment no, but if we ever need to support different devices on > the same RGB connector (actually Atmel's sama5d3xek boards have an > RGB to HDMI bridge connected on the same RGB connector) and these > devices do not support the same RGB mode (say your LCD panel supports > RGB888 and your RGB to HDMI bridge supports RGB555), then depending on > the output you select you'll have to change your pinctrl config at > runtime. Just to make sure I understand the use case correctly, are you talking about two devices (for example an RGB666 panel and an RGB888 RGB to HDMI bridge) connected to the same output, with the ability to switch between the two at runtime ? That's a valid case (on a side note we shouldn't forget that the option of using both devices at the same time should be supported as well), but I would probably go for a fixed pinctrl configuration that supports both, although switching configurations at runtime would be a micro-optimization that might make sense. > I'd say we could get rid of this runtime pinctrl config as a first step > if DT ABI stability was not required. > But it is, and I'd like to have a future proof binding to handle these > tricky cases when they occurs (if they ever do). I think we have a shortcoming of the pinctrl API here in the general case. The API only allows you to select a single configuration per device. Imagine the same display controller, with two DPI outputs, each of them configurable in 444, 565, 666 or 888 modes. With the current API we would have to create 4*4 = 16 pinctrl configurations for all combinations. That obviously wouldn't scale, so we'll have to fix this eventually. From a DT stability point of view, I would thus avoid specifying multiple pinctrl configurations now until we come up with a standard way to support this use case. > Anyway, I'm open to any other alternative that could let me add support > for this later on. > > BTW, is there any reason for not defining an RGB connector type (I'm > currently defining HLCDC connector as an LVDS connector) ? Not that I know of. The DRM API has been developed before display on embedded systems became such a hot topic. If we had to redo it today, panels might be exposed to userspace as such, with a connector. We have to live with the past, so the connector will stay, but adding a new RGB connector type could make sense (although we might need a different name, in a way the VGA and LVDS connectors also carry RGB signals). > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > + The first cell is a phandle to a DRM panel device > > > + The second cell encodes the RGB mode, which can take the following > > > values: > > > + * 0: RGB444 > > > + * 1: RGB565 > > > + * 2: RGB666 > > > + * 3: RGB888 > > > + The third cell encodes specific flags describing LCD signals > > > configuration > > > + (see Atmel's datasheet for a full description of these > > > fields): > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > + * bit 4: DISPPOL: Display Signal Polarity > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal > > > Synchronization > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup > > > Configuration > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold > > > Configuration > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > If I'm not mistaken, those are HLCDC configuration values that depend on > > the panel type and characteristics. Shouldn't they then be queries from > > the panel through the drm_panel API at runtime instead of being specified > > in DT ? This would likely require extending the drm_panel API. > > HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and > DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the > GUARDTIME value. > > Another question I had regarding these flags is whether they were LCD > panel specific or a mix of panel and board implementation. > Take the VSYNC HSYNC polarity, of course the LCD panel defines what it > expects in terms of polarity, but nothing prevents the HW designer from > inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ? > > A solution would be to override some drm_display_mode settings with > informations taken from the DT. Given that I gave the exact same argument during a V4L2 DT bindings design review, I can only agree :-) It thus makes sense to specify polarities in the HLCDC DT node. The RGB mode, however, should probably be queried from the panel, as I don't expect it to be board-dependent but only panel-dependent. I'm not sure about the other bits in the third cell, maybe we should discuss them in more details. I'm always wary when I see DT bindings referring to a datasheet :-) Getting the information from the panel by default, with a possible override, is an interesting option. You would thus likely need several DT properties associated with each connection to a panel. Would it then make sense to use the OF graph DT bindings instead of the atmel,panel property to specify connections ? You could store per-connection data in the endpoint and/or port nodes. > Thanks for your review. You're welcome. Sorry for not having had time to review the driver itself. Given my limited bandwidth at the moment I've decided to concentrate on the DT bindings first.
On Fri, 11 Jul 2014 12:37:46 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote: > > On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart wrote: > > > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote: > > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > > > controller device. > > > > > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > > > panels connection to LCD panels for now. > > > > > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > > > connected on this port (note that the HLCDC RGB connector implementation > > > > makes use of the DRM panel framework). > > > > > > > > Connection to other external devices (DRM bridges) might be added later > > > > by mean of a new atmel,xxx (atmel,bridge) property. > > > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > > --- > > > > > > > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++ > > > > 1 file changed, 59 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode > > > > 100644 > > > > index 0000000..594bdb2 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > @@ -0,0 +1,59 @@ > > > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver > > > > + > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD > > > > device. > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more > > > > details. > > > > + > > > > +Required properties: > > > > + - compatible: value should be one of the following: > > > > + "atmel,hlcdc-dc" > > > > + - interrupts: the HLCDC interrupt definition > > > > + - pinctrl-names: the pin control state names. Should contain > > > > "default", > > > > + "rgb-444", "rgb-565", "rgb-666" and "rgb-888". > > > > + - pinctrl-[0-4]: should contain the pinctrl states described by > > > > pinctrl > > > > + names. > > > > > > Do you need to switch between the different pinctrl configurations at > > > runtime, or is the configuration selected from the panel type, which > > > doesn't change ? > > > > At the moment no, but if we ever need to support different devices on > > the same RGB connector (actually Atmel's sama5d3xek boards have an > > RGB to HDMI bridge connected on the same RGB connector) and these > > devices do not support the same RGB mode (say your LCD panel supports > > RGB888 and your RGB to HDMI bridge supports RGB555), then depending on > > the output you select you'll have to change your pinctrl config at > > runtime. > > Just to make sure I understand the use case correctly, are you talking about > two devices (for example an RGB666 panel and an RGB888 RGB to HDMI bridge) > connected to the same output, with the ability to switch between the two at > runtime ? Exactly. > That's a valid case (on a side note we shouldn't forget that the > option of using both devices at the same time should be supported as well), AFAICT this is only possible if both devices connected to the RGB connector use the same mode. > but I would probably go for a fixed pinctrl configuration that supports both, > although switching configurations at runtime would be a micro-optimization > that might make sense. Yep, it should work, and I agree that we're unlikely to reuse some RGB pins for other usage when the active device is the one using RGB666 mode. > > > I'd say we could get rid of this runtime pinctrl config as a first step > > if DT ABI stability was not required. > > But it is, and I'd like to have a future proof binding to handle these > > tricky cases when they occurs (if they ever do). > > I think we have a shortcoming of the pinctrl API here in the general case. The > API only allows you to select a single configuration per device. Imagine the > same display controller, with two DPI outputs, each of them configurable in > 444, 565, 666 or 888 modes. With the current API we would have to create 4*4 = > 16 pinctrl configurations for all combinations. That obviously wouldn't scale, > so we'll have to fix this eventually. From a DT stability point of view, I > would thus avoid specifying multiple pinctrl configurations now until we come > up with a standard way to support this use case. Given your inputs, I guess I'll drop dynamic pinctrl config for the next version. > > > Anyway, I'm open to any other alternative that could let me add support > > for this later on. > > > > BTW, is there any reason for not defining an RGB connector type (I'm > > currently defining HLCDC connector as an LVDS connector) ? > > Not that I know of. The DRM API has been developed before display on embedded > systems became such a hot topic. If we had to redo it today, panels might be > exposed to userspace as such, with a connector. We have to live with the past, > so the connector will stay, but adding a new RGB connector type could make > sense (although we might need a different name, in a way the VGA and LVDS > connectors also carry RGB signals). I had the same concern: I didn't find how this kind of connectors was named (most of the time they're just referenced as RGB) :-). What about RAW_RGB ? > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > + The first cell is a phandle to a DRM panel device > > > > + The second cell encodes the RGB mode, which can take the following > > > > values: > > > > + * 0: RGB444 > > > > + * 1: RGB565 > > > > + * 2: RGB666 > > > > + * 3: RGB888 > > > > + The third cell encodes specific flags describing LCD signals > > > > configuration > > > > + (see Atmel's datasheet for a full description of these > > > > fields): > > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > > + * bit 4: DISPPOL: Display Signal Polarity > > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal > > > > Synchronization > > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup > > > > Configuration > > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold > > > > Configuration > > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > > > If I'm not mistaken, those are HLCDC configuration values that depend on > > > the panel type and characteristics. Shouldn't they then be queries from > > > the panel through the drm_panel API at runtime instead of being specified > > > in DT ? This would likely require extending the drm_panel API. > > > > HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and > > DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the > > GUARDTIME value. > > > > Another question I had regarding these flags is whether they were LCD > > panel specific or a mix of panel and board implementation. > > Take the VSYNC HSYNC polarity, of course the LCD panel defines what it > > expects in terms of polarity, but nothing prevents the HW designer from > > inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ? > > > > A solution would be to override some drm_display_mode settings with > > informations taken from the DT. > > Given that I gave the exact same argument during a V4L2 DT bindings design > review, I can only agree :-) It thus makes sense to specify polarities in the > HLCDC DT node. The RGB mode, however, should probably be queried from the > panel, as I don't expect it to be board-dependent but only panel-dependent. Yes, what I had in mind is some kind of RGB connector framework (or just helper functions), where you could define the supported RGB modes and rely on another infrastructure to define the supported drm display modes. Because RGB connector is not just related to panels: see this RGB to HDMI bridge [1] (this is the one used on Atmel's dev boards). Moreover, simple panels are connector agnostics for now, and I'm not sure we want to change that. [1]http://pdf1.alldatasheet.fr/datasheet-pdf/view/218068/SILICONIMAGE/SII9022.html > > I'm not sure about the other bits in the third cell, maybe we should discuss > them in more details. I'm always wary when I see DT bindings referring to a > datasheet :-) Getting the information from the panel by default, with a > possible override, is an interesting option. You would thus likely need > several DT properties associated with each connection to a panel. Would it > then make sense to use the OF graph DT bindings instead of the atmel,panel > property to specify connections ? You could store per-connection data in the > endpoint and/or port nodes. I'll take a look at these bindings and let you know if they match my needs. > > > Thanks for your review. > > You're welcome. Sorry for not having had time to review the driver itself. > Given my limited bandwidth at the moment I've decided to concentrate on the DT > bindings first. No problem, after all DT bindings is the most tricky part of our work nowadays, isn't it ;-) ? Thanks, Boris
On Fri, 11 Jul 2014 14:00:25 +0200 Boris BREZILLON <boris.brezillon@free-electrons.com> wrote: > On Fri, 11 Jul 2014 12:37:46 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > > Hi Boris, > > > > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote: > > > On Thu, 10 Jul 2014 13:16:21 +0200 Laurent Pinchart wrote: > > > > On Monday 07 July 2014 18:42:59 Boris BREZILLON wrote: > > > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > > > > controller device. > > > > > > > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > > > > panels connection to LCD panels for now. > > > > > > > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > > > > connected on this port (note that the HLCDC RGB connector implementation > > > > > makes use of the DRM panel framework). > > > > > > > > > > Connection to other external devices (DRM bridges) might be added later > > > > > by mean of a new atmel,xxx (atmel,bridge) property. > > > > > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > > > --- > > > > > > > > > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++ > > > > > 1 file changed, 59 insertions(+) > > > > > create mode 100644 > > > > > Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode > > > > > 100644 > > > > > index 0000000..594bdb2 > > > > > --- /dev/null > > > > > +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > @@ -0,0 +1,59 @@ > > > > > +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver > > > > > + > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD > > > > > device. > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more > > > > > details. > > > > > + > > > > > +Required properties: > > > > > + - compatible: value should be one of the following: > > > > > + "atmel,hlcdc-dc" > > > > > + - interrupts: the HLCDC interrupt definition > > > > > + - pinctrl-names: the pin control state names. Should contain > > > > > "default", > > > > > + "rgb-444", "rgb-565", "rgb-666" and "rgb-888". > > > > > + - pinctrl-[0-4]: should contain the pinctrl states described by > > > > > pinctrl > > > > > + names. > > > > > > > > Do you need to switch between the different pinctrl configurations at > > > > runtime, or is the configuration selected from the panel type, which > > > > doesn't change ? > > > > > > At the moment no, but if we ever need to support different devices on > > > the same RGB connector (actually Atmel's sama5d3xek boards have an > > > RGB to HDMI bridge connected on the same RGB connector) and these > > > devices do not support the same RGB mode (say your LCD panel supports > > > RGB888 and your RGB to HDMI bridge supports RGB555), then depending on > > > the output you select you'll have to change your pinctrl config at > > > runtime. > > > > Just to make sure I understand the use case correctly, are you talking about > > two devices (for example an RGB666 panel and an RGB888 RGB to HDMI bridge) > > connected to the same output, with the ability to switch between the two at > > runtime ? > > Exactly. > > > That's a valid case (on a side note we shouldn't forget that the > > option of using both devices at the same time should be supported as well), > > AFAICT this is only possible if both devices connected to the RGB > connector use the same mode. > > > but I would probably go for a fixed pinctrl configuration that supports both, > > although switching configurations at runtime would be a micro-optimization > > that might make sense. > > Yep, it should work, and I agree that we're unlikely to reuse some RGB > pins for other usage when the active device is the one using RGB666 > mode. > > > > > > I'd say we could get rid of this runtime pinctrl config as a first step > > > if DT ABI stability was not required. > > > But it is, and I'd like to have a future proof binding to handle these > > > tricky cases when they occurs (if they ever do). > > > > I think we have a shortcoming of the pinctrl API here in the general case. The > > API only allows you to select a single configuration per device. Imagine the > > same display controller, with two DPI outputs, each of them configurable in > > 444, 565, 666 or 888 modes. With the current API we would have to create 4*4 = > > 16 pinctrl configurations for all combinations. That obviously wouldn't scale, > > so we'll have to fix this eventually. From a DT stability point of view, I > > would thus avoid specifying multiple pinctrl configurations now until we come > > up with a standard way to support this use case. > > Given your inputs, I guess I'll drop dynamic pinctrl config for the > next version. > > > > > > Anyway, I'm open to any other alternative that could let me add support > > > for this later on. > > > > > > BTW, is there any reason for not defining an RGB connector type (I'm > > > currently defining HLCDC connector as an LVDS connector) ? > > > > Not that I know of. The DRM API has been developed before display on embedded > > systems became such a hot topic. If we had to redo it today, panels might be > > exposed to userspace as such, with a connector. We have to live with the past, > > so the connector will stay, but adding a new RGB connector type could make > > sense (although we might need a different name, in a way the VGA and LVDS > > connectors also carry RGB signals). > > I had the same concern: I didn't find how this kind of connectors > was named (most of the time they're just referenced as RGB) :-). > What about RAW_RGB ? Okay, actually there is a widely used name for this kind of connectors and it's "Parallel RGB" (thanks Thomas ;-)). > > > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > + The first cell is a phandle to a DRM panel device > > > > > + The second cell encodes the RGB mode, which can take the following > > > > > values: > > > > > + * 0: RGB444 > > > > > + * 1: RGB565 > > > > > + * 2: RGB666 > > > > > + * 3: RGB888 > > > > > + The third cell encodes specific flags describing LCD signals > > > > > configuration > > > > > + (see Atmel's datasheet for a full description of these > > > > > fields): > > > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > > > + * bit 4: DISPPOL: Display Signal Polarity > > > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal > > > > > Synchronization > > > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup > > > > > Configuration > > > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold > > > > > Configuration > > > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > > > > > If I'm not mistaken, those are HLCDC configuration values that depend on > > > > the panel type and characteristics. Shouldn't they then be queries from > > > > the panel through the drm_panel API at runtime instead of being specified > > > > in DT ? This would likely require extending the drm_panel API. > > > > > > HSPOL and VSPOL can be deduced from DRM_MODE_FLAG_[PN]HSYNC and > > > DRM_MODE_FLAG_[PN]VSYNC, I'm not sure for the other flags or the > > > GUARDTIME value. > > > > > > Another question I had regarding these flags is whether they were LCD > > > panel specific or a mix of panel and board implementation. > > > Take the VSYNC HSYNC polarity, of course the LCD panel defines what it > > > expects in terms of polarity, but nothing prevents the HW designer from > > > inverting the VSYNC/HSYNC polarity (expect common sense :-)), right ? > > > > > > A solution would be to override some drm_display_mode settings with > > > informations taken from the DT. > > > > Given that I gave the exact same argument during a V4L2 DT bindings design > > review, I can only agree :-) It thus makes sense to specify polarities in the > > HLCDC DT node. The RGB mode, however, should probably be queried from the > > panel, as I don't expect it to be board-dependent but only panel-dependent. > > Yes, what I had in mind is some kind of RGB connector framework (or > just helper functions), where you could define the supported RGB modes > and rely on another infrastructure to define the supported drm display > modes. > Because RGB connector is not just related to panels: see this RGB to > HDMI bridge [1] (this is the one used on Atmel's dev boards). > Moreover, simple panels are connector agnostics for now, and I'm not > sure we want to change that. > > [1]http://pdf1.alldatasheet.fr/datasheet-pdf/view/218068/SILICONIMAGE/SII9022.html > > > > > I'm not sure about the other bits in the third cell, maybe we should discuss > > them in more details. I'm always wary when I see DT bindings referring to a > > datasheet :-) Getting the information from the panel by default, with a > > possible override, is an interesting option. You would thus likely need > > several DT properties associated with each connection to a panel. Would it > > then make sense to use the OF graph DT bindings instead of the atmel,panel > > property to specify connections ? You could store per-connection data in the > > endpoint and/or port nodes. > > I'll take a look at these bindings and let you know if they match my > needs. > > > > > > Thanks for your review. > > > > You're welcome. Sorry for not having had time to review the driver itself. > > Given my limited bandwidth at the moment I've decided to concentrate on the DT > > bindings first. > > No problem, after all DT bindings is the most tricky part of our work > nowadays, isn't it ;-) ? > > Thanks, > > Boris >
On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > controller device. > > The HLCDC block provides a single RGB output port, and only supports LCD > panels connection to LCD panels for now. > > The atmel,panel property link the HLCDC RGB output with the LCD panel > connected on this port (note that the HLCDC RGB connector implementation > makes use of the DRM panel framework). > > Connection to other external devices (DRM bridges) might be added later by > mean of a new atmel,xxx (atmel,bridge) property. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > --- > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++++++++++++++++ > 1 file changed, 59 insertions(+) > create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt This is the wrong directory. Device tree bindings describe hardware, but DRM is a Linux-specific framework. And yes, there are already files in that directory, I know, but that doesn't make it any better. I suggest either devicetree/bindings/gpu or devicetree/bindings/video. > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt [...] > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. I think it's better to refer to these using relative filenames. When the device tree bindings are moved out of the kernel tree, they may no longer use the same hierarchy. > +Required properties: > + - compatible: value should be one of the following: > + "atmel,hlcdc-dc" There's only one value, so perhaps: should be "atmel,hlcdc-dc". > + - atmel,panel: Should contain a phandle with 2 parameters. > + The first cell is a phandle to a DRM panel device > + The second cell encodes the RGB mode, which can take the following values: > + * 0: RGB444 > + * 1: RGB565 > + * 2: RGB666 > + * 3: RGB888 These are properties of the panel and should be obtained from the panel directly rather than an additional cell in this specifier. > + The third cell encodes specific flags describing LCD signals configuration > + (see Atmel's datasheet for a full description of these fields): > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > + * bit 4: DISPPOL: Display Signal Polarity > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time Similarly for most of these: HSPOL and VSPOL seem to correspond to the DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And VSPDLYS as well as VSPDLYE sound like they may be vsync_start and vsync_end of the same structure. As for the others, maybe if you could explain what exactly they are we may be able to find a better fit. Thierry
On Fri, Jul 11, 2014 at 02:00:25PM +0200, Boris BREZILLON wrote: > On Fri, 11 Jul 2014 12:37:46 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote: [...] > > > BTW, is there any reason for not defining an RGB connector type (I'm > > > currently defining HLCDC connector as an LVDS connector) ? > > > > Not that I know of. The DRM API has been developed before display on embedded > > systems became such a hot topic. If we had to redo it today, panels might be > > exposed to userspace as such, with a connector. We have to live with the past, > > so the connector will stay, but adding a new RGB connector type could make > > sense (although we might need a different name, in a way the VGA and LVDS > > connectors also carry RGB signals). > > I had the same concern: I didn't find how this kind of connectors > was named (most of the time they're just referenced as RGB) :-). > What about RAW_RGB ? Are there even panels that take raw RGB as input? In all cases I've seen (which admittedly may not be all that many) there's always a transparent RGB/LVDS bridge, so the "connector" is in fact LVDS, not RGB, even if the display controller outputs RGB directly. Thierry
Hello Thierry, On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > controller device. > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > panels connection to LCD panels for now. > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > connected on this port (note that the HLCDC RGB connector implementation > > makes use of the DRM panel framework). > > > > Connection to other external devices (DRM bridges) might be added later by > > mean of a new atmel,xxx (atmel,bridge) property. > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > --- > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++++++++++++++++ > > 1 file changed, 59 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > This is the wrong directory. Device tree bindings describe hardware, but > DRM is a Linux-specific framework. And yes, there are already files in > that directory, I know, but that doesn't make it any better. > > I suggest either devicetree/bindings/gpu or devicetree/bindings/video. No problem, I'll move the documentation into devicetree/bindings/video (the HLCDC does not provide any 3D rendering functionality and thus I'm not sure moving the bindings documentation into devicetree/bindings/gpu makes sense). > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > [...] > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > I think it's better to refer to these using relative filenames. When the > device tree bindings are moved out of the kernel tree, they may no > longer use the same hierarchy. Sure. By relative path you mean ../../mfd/atmel-hlcdc.txt or just mfd/atmel-hlcdc.txt ? > > > +Required properties: > > + - compatible: value should be one of the following: > > + "atmel,hlcdc-dc" > > There's only one value, so perhaps: should be "atmel,hlcdc-dc". Yes, I'll fix that. > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > + The first cell is a phandle to a DRM panel device > > + The second cell encodes the RGB mode, which can take the following values: > > + * 0: RGB444 > > + * 1: RGB565 > > + * 2: RGB666 > > + * 3: RGB888 > > These are properties of the panel and should be obtained from the panel > directly rather than an additional cell in this specifier. Okay. What's the preferred way of doing this ? What about defining an rgb-mode property in the panel node. BTW, have you received this series [1] adding support for the LCD panel I'm testing this driver with. [1] https://lkml.org/lkml/2014/6/5/612 > > > + The third cell encodes specific flags describing LCD signals configuration > > + (see Atmel's datasheet for a full description of these fields): > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > + * bit 4: DISPPOL: Display Signal Polarity > > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and > vsync_end of the same structure. I agree with HSPOL and VSPOL. > > As for the others, maybe if you could explain what exactly they are we > may be able to find a better fit. Atmel datasheets include several timing diagrams [2] (chapter "32.6.17 Output Timing Generation" page 603), and I think you will get more informations from these diagrams than if I try to explain what I understood ;-). Best Regards, Boris [1]https://lkml.org/lkml/2014/6/5/612 [2]http://www.atmel.com/Images/Atmel_11121_32-bit-Cortex-A5-Microcontroller_SAMA5D3_Datasheet.pdf
Hi Boris, On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > > controller device. > > > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > > panels connection to LCD panels for now. > > > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > > connected on this port (note that the HLCDC RGB connector implementation > > > makes use of the DRM panel framework). > > > > > > Connection to other external devices (DRM bridges) might be added later > > > by mean of a new atmel,xxx (atmel,bridge) property. > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > --- > > > > > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++ > > > 1 file changed, 59 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt [snip] > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > + The first cell is a phandle to a DRM panel device > > > + The second cell encodes the RGB mode, which can take the following > > > values: + * 0: RGB444 > > > + * 1: RGB565 > > > + * 2: RGB666 > > > + * 3: RGB888 > > > > These are properties of the panel and should be obtained from the panel > > directly rather than an additional cell in this specifier. > > Okay. > What's the preferred way of doing this ? > What about defining an rgb-mode property in the panel node. You could do that, but it won't help you much, as the HLCDC driver must not parse properties from the panel node. You should instead extend the drm_panel API with a function to retrieve panel properties. The HLCDC driver will then query the panel driver at runtime for the interface type. The panel driver will get the information from hardcoded data in the driver, from DT or possibly in some cases by querying the panel hardware directly. > BTW, have you received this series [1] adding support for the LCD panel > I'm testing this driver with. > > [1] https://lkml.org/lkml/2014/6/5/612 > > > > + The third cell encodes specific flags describing LCD signals > > > configuration > > > + (see Atmel's datasheet for a full description of these fields): > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > + * bit 4: DISPPOL: Display Signal Polarity > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal > > > Synchronization > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup > > > Configuration > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold > > > Configuration > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the > > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And > > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and > > vsync_end of the same structure. > > I agree with HSPOL and VSPOL. > > > As for the others, maybe if you could explain what exactly they are we > > may be able to find a better fit. > > Atmel datasheets include several timing diagrams [2] (chapter "32.6.17 > Output Timing Generation" page 603), and I think you will get more > informations from these diagrams than if I try to explain what I > understood ;-). The VSP* bits fine-tune the VSYNC pulse generation timings by specifying the relationship between the VSYNC edges and the HSYNC pulses at the beginning and end of the VSYNC pulse. It might make sense to turn that into standard DRM properties, but we'll need to research if and how other vendor offer similar features. As for GUARDTIME, I would split it into its own property. What are the typical values you have seen being used in read systems ?
On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: [...] > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > [...] > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > > > I think it's better to refer to these using relative filenames. When the > > device tree bindings are moved out of the kernel tree, they may no > > longer use the same hierarchy. > > Sure. > By relative path you mean ../../mfd/atmel-hlcdc.txt or just > mfd/atmel-hlcdc.txt ? I think the former is more explicit. > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > + The first cell is a phandle to a DRM panel device > > > + The second cell encodes the RGB mode, which can take the following values: > > > + * 0: RGB444 > > > + * 1: RGB565 > > > + * 2: RGB666 > > > + * 3: RGB888 > > > > These are properties of the panel and should be obtained from the panel > > directly rather than an additional cell in this specifier. > > Okay. > What's the preferred way of doing this ? > What about defining an rgb-mode property in the panel node. There's .bpc in struct drm_display_info, I suspect that it could be used for this. Alternatively, maybe we could extend the list of color formats that go into drm_display_info.color_formats? RGB444 is already covered. Also, like Laurent said, this shouldn't go into the device tree, since it's already implied by the panel's compatible value, so we'd be duplicating information. > BTW, have you received this series [1] adding support for the LCD panel > I'm testing this driver with. > > [1] https://lkml.org/lkml/2014/6/5/612 I don't think I've seen it in my inbox, let me check my archives. > > > + The third cell encodes specific flags describing LCD signals configuration > > > + (see Atmel's datasheet for a full description of these fields): > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > + * bit 4: DISPPOL: Display Signal Polarity > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the > > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And > > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and > > vsync_end of the same structure. > > I agree with HSPOL and VSPOL. > > > > > As for the others, maybe if you could explain what exactly they are we > > may be able to find a better fit. > > Atmel datasheets include several timing diagrams [2] (chapter "32.6.17 > Output Timing Generation" page 603), and I think you will get more > informations from these diagrams than if I try to explain what I > understood ;-). These look like knobs to tune the signal in a very fine-grained manner. To be honest, maybe the best way to solve this would be by omitting them for now and choose some default that's likely to work on most devices. Does the panel that you use specify how it expects HSYNC to be timed vs. VSYNC? Thierry
On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > Hi Boris, > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > > > The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. > > > > at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display > > > > controller device. > > > > > > > > The HLCDC block provides a single RGB output port, and only supports LCD > > > > panels connection to LCD panels for now. > > > > > > > > The atmel,panel property link the HLCDC RGB output with the LCD panel > > > > connected on this port (note that the HLCDC RGB connector implementation > > > > makes use of the DRM panel framework). > > > > > > > > Connection to other external devices (DRM bridges) might be added later > > > > by mean of a new atmel,xxx (atmel,bridge) property. > > > > > > > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > > --- > > > > > > > > .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++++++ > > > > 1 file changed, 59 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > [snip] > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > + The first cell is a phandle to a DRM panel device > > > > + The second cell encodes the RGB mode, which can take the following > > > > values: + * 0: RGB444 > > > > + * 1: RGB565 > > > > + * 2: RGB666 > > > > + * 3: RGB888 > > > > > > These are properties of the panel and should be obtained from the panel > > > directly rather than an additional cell in this specifier. > > > > Okay. > > What's the preferred way of doing this ? > > What about defining an rgb-mode property in the panel node. > > You could do that, but it won't help you much, as the HLCDC driver must not > parse properties from the panel node. You should instead extend the drm_panel > API with a function to retrieve panel properties. The HLCDC driver will then > query the panel driver at runtime for the interface type. The panel driver > will get the information from hardcoded data in the driver, from DT or > possibly in some cases by querying the panel hardware directly. My preference for this would be that we either add this to some existing structure (struct drm_display_info seems like a good candidate) or if the number of parameters grows out of hands, then maybe even introduce a new type of device that's specific for the interface. DRM panels are an abstraction for panels, that is, interface-agnostic, and if we start exposing interface specific parameters things will start to become very unwieldy. Thierry
Hi Thierry, On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > >>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > >>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > >>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a > >>>> display controller device. > >>>> > >>>> The HLCDC block provides a single RGB output port, and only supports > >>>> LCD panels connection to LCD panels for now. > >>>> > >>>> The atmel,panel property link the HLCDC RGB output with the LCD > >>>> panel connected on this port (note that the HLCDC RGB connector > >>>> implementation makes use of the DRM panel framework). > >>>> > >>>> Connection to other external devices (DRM bridges) might be added > >>>> later by mean of a new atmel,xxx (atmel,bridge) property. > >>>> > >>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > >>>> --- > >>>> > >>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++ > >>>> 1 file changed, 59 insertions(+) > >>>> create mode 100644 > >>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > [snip] > > > >>>> + - atmel,panel: Should contain a phandle with 2 parameters. > >>>> + The first cell is a phandle to a DRM panel device > >>>> + The second cell encodes the RGB mode, which can take the > >>>> following values: > >>>> + * 0: RGB444 > >>>> + * 1: RGB565 > >>>> + * 2: RGB666 > >>>> + * 3: RGB888 > >>> > >>> These are properties of the panel and should be obtained from the > >>> panel directly rather than an additional cell in this specifier. > >> > >> Okay. > >> What's the preferred way of doing this ? > >> What about defining an rgb-mode property in the panel node. > > > > You could do that, but it won't help you much, as the HLCDC driver must > > not parse properties from the panel node. You should instead extend the > > drm_panel API with a function to retrieve panel properties. The HLCDC > > driver will then query the panel driver at runtime for the interface > > type. The panel driver will get the information from hardcoded data in > > the driver, from DT or possibly in some cases by querying the panel > > hardware directly. > > My preference for this would be that we either add this to some existing > structure (struct drm_display_info seems like a good candidate) or if > the number of parameters grows out of hands, then maybe even introduce a > new type of device that's specific for the interface. DRM panels are an > abstraction for panels, that is, interface-agnostic, and if we start > exposing interface specific parameters things will start to become very > unwieldy. I agree with the goal of keeping drm_panel interface-agnostic. However, one way or another, interface parameters will need to be communicated between the panel driver and the controller driver. My preference, if we need to extend the number and/or scope of parameters beyond what drm_display_info could reasonably contain, would be to implement a new drm_panel operation to query/configure interface parameters, using a structure that contains the interface type and a union of type-specific structures. This would keep the API generic in the sense of not requiring explicit knowledge of all interfaces in the drivers, while offering the flexibility we need with a way to easily detect the interface type at runtime and react on unknown/unsupported types.
On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > > On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > > >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > >>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > >>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > > >>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a > > >>>> display controller device. > > >>>> > > >>>> The HLCDC block provides a single RGB output port, and only supports > > >>>> LCD panels connection to LCD panels for now. > > >>>> > > >>>> The atmel,panel property link the HLCDC RGB output with the LCD > > >>>> panel connected on this port (note that the HLCDC RGB connector > > >>>> implementation makes use of the DRM panel framework). > > >>>> > > >>>> Connection to other external devices (DRM bridges) might be added > > >>>> later by mean of a new atmel,xxx (atmel,bridge) property. > > >>>> > > >>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > >>>> --- > > >>>> > > >>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++ > > >>>> 1 file changed, 59 insertions(+) > > >>>> create mode 100644 > > >>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > > [snip] > > > > > >>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > >>>> + The first cell is a phandle to a DRM panel device > > >>>> + The second cell encodes the RGB mode, which can take the > > >>>> following values: > > >>>> + * 0: RGB444 > > >>>> + * 1: RGB565 > > >>>> + * 2: RGB666 > > >>>> + * 3: RGB888 > > >>> > > >>> These are properties of the panel and should be obtained from the > > >>> panel directly rather than an additional cell in this specifier. > > >> > > >> Okay. > > >> What's the preferred way of doing this ? > > >> What about defining an rgb-mode property in the panel node. > > > > > > You could do that, but it won't help you much, as the HLCDC driver must > > > not parse properties from the panel node. You should instead extend the > > > drm_panel API with a function to retrieve panel properties. The HLCDC > > > driver will then query the panel driver at runtime for the interface > > > type. The panel driver will get the information from hardcoded data in > > > the driver, from DT or possibly in some cases by querying the panel > > > hardware directly. > > > > My preference for this would be that we either add this to some existing > > structure (struct drm_display_info seems like a good candidate) or if > > the number of parameters grows out of hands, then maybe even introduce a > > new type of device that's specific for the interface. DRM panels are an > > abstraction for panels, that is, interface-agnostic, and if we start > > exposing interface specific parameters things will start to become very > > unwieldy. > > I agree with the goal of keeping drm_panel interface-agnostic. However, one > way or another, interface parameters will need to be communicated between the > panel driver and the controller driver. My preference, if we need to extend > the number and/or scope of parameters beyond what drm_display_info could > reasonably contain, would be to implement a new drm_panel operation to > query/configure interface parameters, using a structure that contains the > interface type and a union of type-specific structures. This would keep the > API generic in the sense of not requiring explicit knowledge of all interfaces > in the drivers, while offering the flexibility we need with a way to easily > detect the interface type at runtime and react on unknown/unsupported types. That's exactly what I was hoping could be avoided. If instead we modeled the interface type as a bus, we could for example have an lvds_bus along with an lvds_device and then use that as the natural place to store these properties. Much like we do for DSI. Thierry
Hi Thierry, On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote: > On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote: > > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > >> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > >>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > >>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > >>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > >>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > >>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a > >>>>>> display controller device. > >>>>>> > >>>>>> The HLCDC block provides a single RGB output port, and only > >>>>>> supports LCD panels connection to LCD panels for now. > >>>>>> > >>>>>> The atmel,panel property link the HLCDC RGB output with the LCD > >>>>>> panel connected on this port (note that the HLCDC RGB connector > >>>>>> implementation makes use of the DRM panel framework). > >>>>>> > >>>>>> Connection to other external devices (DRM bridges) might be added > >>>>>> later by mean of a new atmel,xxx (atmel,bridge) property. > >>>>>> > >>>>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > >>>>>> --- > >>>>>> > >>>>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++++ > >>>>>> 1 file changed, 59 insertions(+) > >>>>>> create mode 100644 > >>>>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > >>> > >>> [snip] > >>> > >>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > >>>>>> + The first cell is a phandle to a DRM panel device > >>>>>> + The second cell encodes the RGB mode, which can take the > >>>>>> following values: > >>>>>> + * 0: RGB444 > >>>>>> + * 1: RGB565 > >>>>>> + * 2: RGB666 > >>>>>> + * 3: RGB888 > >>>>> > >>>>> These are properties of the panel and should be obtained from the > >>>> panel directly rather than an additional cell in this specifier. > >>>> > >>>> Okay. > >>>> What's the preferred way of doing this ? > >>>> What about defining an rgb-mode property in the panel node. > >>> > >>> You could do that, but it won't help you much, as the HLCDC driver > >>> must not parse properties from the panel node. You should instead > >>> extend the drm_panel API with a function to retrieve panel properties. > >>> The HLCDC driver will then query the panel driver at runtime for the > >>> interface type. The panel driver will get the information from > >>> hardcoded data in the driver, from DT or possibly in some cases by > >>> querying the panel hardware directly. > >> > >> My preference for this would be that we either add this to some existing > >> structure (struct drm_display_info seems like a good candidate) or if > >> the number of parameters grows out of hands, then maybe even introduce a > >> new type of device that's specific for the interface. DRM panels are an > >> abstraction for panels, that is, interface-agnostic, and if we start > >> exposing interface specific parameters things will start to become very > >> unwieldy. > > > > I agree with the goal of keeping drm_panel interface-agnostic. However, > > one way or another, interface parameters will need to be communicated > > between the panel driver and the controller driver. My preference, if we > > need to extend the number and/or scope of parameters beyond what > > drm_display_info could reasonably contain, would be to implement a new > > drm_panel operation to query/configure interface parameters, using a > > structure that contains the interface type and a union of type-specific > > structures. This would keep the API generic in the sense of not requiring > > explicit knowledge of all interfaces in the drivers, while offering the > > flexibility we need with a way to easily detect the interface type at > > runtime and react on unknown/unsupported types. > > That's exactly what I was hoping could be avoided. If instead we modeled > the interface type as a bus, we could for example have an lvds_bus along > with an lvds_device and then use that as the natural place to store > these properties. Much like we do for DSI. And I believe that's what we should avoid ;-) First of all, let's not forget that Linux models control busses, not data busses. DSI is a special case as it combines the control and data busses, but in the general case the same implementation isn't possible. An LVDS panel controlled through I2C needs to be an I2C device sitting on an I2C bus. Then, I believe it would make all drivers simpler if we had a single object type to deal with, with proper abstractions for bus types. A drm_panel that can model panels regardless of the data bus type, with one operation that conveys bus-specific information, makes storing the objects and communicating with them simpler than having to deal with different kind of devices.
On Mon, 14 Jul 2014 12:18:08 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Jul 11, 2014 at 02:00:25PM +0200, Boris BREZILLON wrote: > > On Fri, 11 Jul 2014 12:37:46 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > > On Thursday 10 July 2014 14:56:26 Boris BREZILLON wrote: > [...] > > > > BTW, is there any reason for not defining an RGB connector type (I'm > > > > currently defining HLCDC connector as an LVDS connector) ? > > > > > > Not that I know of. The DRM API has been developed before display on embedded > > > systems became such a hot topic. If we had to redo it today, panels might be > > > exposed to userspace as such, with a connector. We have to live with the past, > > > so the connector will stay, but adding a new RGB connector type could make > > > sense (although we might need a different name, in a way the VGA and LVDS > > > connectors also carry RGB signals). > > > > I had the same concern: I didn't find how this kind of connectors > > was named (most of the time they're just referenced as RGB) :-). > > What about RAW_RGB ? > > Are there even panels that take raw RGB as input? In all cases I've seen > (which admittedly may not be all that many) there's always a transparent > RGB/LVDS bridge, so the "connector" is in fact LVDS, not RGB, even if > the display controller outputs RGB directly. At least the LCD module I'm using (FL500WVR00-A0T) is taking raw RGB as input ;-). Best Regards, Boris
On Tue, 15 Jul 2014 12:52:54 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote: > > Hi Thierry, > > > > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > > > On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > > > > On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > > > >> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > > >>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > > >>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > > > >>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a > > > >>>> display controller device. > > > >>>> > > > >>>> The HLCDC block provides a single RGB output port, and only supports > > > >>>> LCD panels connection to LCD panels for now. > > > >>>> > > > >>>> The atmel,panel property link the HLCDC RGB output with the LCD > > > >>>> panel connected on this port (note that the HLCDC RGB connector > > > >>>> implementation makes use of the DRM panel framework). > > > >>>> > > > >>>> Connection to other external devices (DRM bridges) might be added > > > >>>> later by mean of a new atmel,xxx (atmel,bridge) property. > > > >>>> > > > >>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > > >>>> --- > > > >>>> > > > >>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 +++++++++++ > > > >>>> 1 file changed, 59 insertions(+) > > > >>>> create mode 100644 > > > >>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > > > > [snip] > > > > > > > >>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > > >>>> + The first cell is a phandle to a DRM panel device > > > >>>> + The second cell encodes the RGB mode, which can take the > > > >>>> following values: > > > >>>> + * 0: RGB444 > > > >>>> + * 1: RGB565 > > > >>>> + * 2: RGB666 > > > >>>> + * 3: RGB888 > > > >>> > > > >>> These are properties of the panel and should be obtained from the > > > >>> panel directly rather than an additional cell in this specifier. > > > >> > > > >> Okay. > > > >> What's the preferred way of doing this ? > > > >> What about defining an rgb-mode property in the panel node. > > > > > > > > You could do that, but it won't help you much, as the HLCDC driver must > > > > not parse properties from the panel node. You should instead extend the > > > > drm_panel API with a function to retrieve panel properties. The HLCDC > > > > driver will then query the panel driver at runtime for the interface > > > > type. The panel driver will get the information from hardcoded data in > > > > the driver, from DT or possibly in some cases by querying the panel > > > > hardware directly. > > > > > > My preference for this would be that we either add this to some existing > > > structure (struct drm_display_info seems like a good candidate) or if > > > the number of parameters grows out of hands, then maybe even introduce a > > > new type of device that's specific for the interface. DRM panels are an > > > abstraction for panels, that is, interface-agnostic, and if we start > > > exposing interface specific parameters things will start to become very > > > unwieldy. > > > > I agree with the goal of keeping drm_panel interface-agnostic. However, one > > way or another, interface parameters will need to be communicated between the > > panel driver and the controller driver. My preference, if we need to extend > > the number and/or scope of parameters beyond what drm_display_info could > > reasonably contain, would be to implement a new drm_panel operation to > > query/configure interface parameters, using a structure that contains the > > interface type and a union of type-specific structures. This would keep the > > API generic in the sense of not requiring explicit knowledge of all interfaces > > in the drivers, while offering the flexibility we need with a way to easily > > detect the interface type at runtime and react on unknown/unsupported types. > > That's exactly what I was hoping could be avoided. If instead we modeled > the interface type as a bus, we could for example have an lvds_bus along > with an lvds_device and then use that as the natural place to store > these properties. Much like we do for DSI. I understand this is not a simple case here, and this is why I left RGB mode config in the HLCDC node in the first place. Anyway, I agree that this rgb mode should not be defined in the hlcdc node but rather in the slave device. I said slave device and not panel device here because the device connected to the RGB connector is not necessarily an LCD panel (i.e. atmel is connecting a raw RGB to HDMI bridge on the RGB connector). And given that I definitely think an interface bus architecture is better: this way we could configure RGB mode no matter what kind of device is connected on this bus and we could keep slave devices interface-agnostic. This being said, I guess modeling interface (or connector) types as busses is not that simple. I really want to help here, so let me know what I can do. Just a side note: you are saying that RGB mode is a panel property, and this is not entirely true (it might depends on board design) :-). In some HW designs, LSB bits of the RGB connector are either connected to ground or to the first available MSB bit. This way you can use an LCD panel supporting RGB888 mode with an display controller supporting lower modes (RGB555 or RGB666). Best Regards, Boris
Hi Laurent, On Tue, 15 Jul 2014 13:07:58 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Thierry, > > On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote: > > On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote: > > > On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > > >> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > > >>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > > >>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > >>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > >>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > > >>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a > > >>>>>> display controller device. > > >>>>>> > > >>>>>> The HLCDC block provides a single RGB output port, and only > > >>>>>> supports LCD panels connection to LCD panels for now. > > >>>>>> > > >>>>>> The atmel,panel property link the HLCDC RGB output with the LCD > > >>>>>> panel connected on this port (note that the HLCDC RGB connector > > >>>>>> implementation makes use of the DRM panel framework). > > >>>>>> > > >>>>>> Connection to other external devices (DRM bridges) might be added > > >>>>>> later by mean of a new atmel,xxx (atmel,bridge) property. > > >>>>>> > > >>>>>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > > >>>>>> --- > > >>>>>> > > >>>>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++++ > > >>>>>> 1 file changed, 59 insertions(+) > > >>>>>> create mode 100644 > > >>>>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > >>> > > >>> [snip] > > >>> > > >>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > >>>>>> + The first cell is a phandle to a DRM panel device > > >>>>>> + The second cell encodes the RGB mode, which can take the > > >>>>>> following values: > > >>>>>> + * 0: RGB444 > > >>>>>> + * 1: RGB565 > > >>>>>> + * 2: RGB666 > > >>>>>> + * 3: RGB888 > > >>>>> > > >>>>> These are properties of the panel and should be obtained from the > > >>>> panel directly rather than an additional cell in this specifier. > > >>>> > > >>>> Okay. > > >>>> What's the preferred way of doing this ? > > >>>> What about defining an rgb-mode property in the panel node. > > >>> > > >>> You could do that, but it won't help you much, as the HLCDC driver > > >>> must not parse properties from the panel node. You should instead > > >>> extend the drm_panel API with a function to retrieve panel properties. > > >>> The HLCDC driver will then query the panel driver at runtime for the > > >>> interface type. The panel driver will get the information from > > >>> hardcoded data in the driver, from DT or possibly in some cases by > > >>> querying the panel hardware directly. > > >> > > >> My preference for this would be that we either add this to some existing > > >> structure (struct drm_display_info seems like a good candidate) or if > > >> the number of parameters grows out of hands, then maybe even introduce a > > >> new type of device that's specific for the interface. DRM panels are an > > >> abstraction for panels, that is, interface-agnostic, and if we start > > >> exposing interface specific parameters things will start to become very > > >> unwieldy. > > > > > > I agree with the goal of keeping drm_panel interface-agnostic. However, > > > one way or another, interface parameters will need to be communicated > > > between the panel driver and the controller driver. My preference, if we > > > need to extend the number and/or scope of parameters beyond what > > > drm_display_info could reasonably contain, would be to implement a new > > > drm_panel operation to query/configure interface parameters, using a > > > structure that contains the interface type and a union of type-specific > > > structures. This would keep the API generic in the sense of not requiring > > > explicit knowledge of all interfaces in the drivers, while offering the > > > flexibility we need with a way to easily detect the interface type at > > > runtime and react on unknown/unsupported types. > > > > That's exactly what I was hoping could be avoided. If instead we modeled > > the interface type as a bus, we could for example have an lvds_bus along > > with an lvds_device and then use that as the natural place to store > > these properties. Much like we do for DSI. > > And I believe that's what we should avoid ;-) First of all, let's not forget > that Linux models control busses, not data busses. DSI is a special case as it > combines the control and data busses, but in the general case the same > implementation isn't possible. An LVDS panel controlled through I2C needs to > be an I2C device sitting on an I2C bus. > > Then, I believe it would make all drivers simpler if we had a single object > type to deal with, with proper abstractions for bus types. A drm_panel that > can model panels regardless of the data bus type, with one operation that > conveys bus-specific information, makes storing the objects and communicating > with them simpler than having to deal with different kind of devices. > Could you detail a bit what you mean by "single object type" ? Is this about making a common abstraction class (by mean of drm_xxx and drm_xxx_funcs) that could represent any display device (drm_bridge, drm_panel, ...) ? Best Regards, Boris
Hi Boris, On Wednesday 16 July 2014 15:05:22 Boris BREZILLON wrote: > On Tue, 15 Jul 2014 13:07:58 +0200 Laurent Pinchart wrote: > > On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote: > >> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote: > >>> On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > >>>> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > >>>>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > >>>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > >>>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > >>>>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > >>>>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides > >>>>>>>> a display controller device. > >>>>>>>> > >>>>>>>> The HLCDC block provides a single RGB output port, and only > >>>>>>>> supports LCD panels connection to LCD panels for now. > >>>>>>>> > >>>>>>>> The atmel,panel property link the HLCDC RGB output with the LCD > >>>>>>>> panel connected on this port (note that the HLCDC RGB connector > >>>>>>>> implementation makes use of the DRM panel framework). > >>>>>>>> > >>>>>>>> Connection to other external devices (DRM bridges) might be added > >>>>>>>> later by mean of a new atmel,xxx (atmel,bridge) property. > >>>>>>>> > >>>>>>>> Signed-off-by: Boris BREZILLON > >>>>>>>> <boris.brezillon@free-electrons.com> > >>>>>>>> --- > >>>>>>>> > >>>>>>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++ > >>>>>>>> 1 file changed, 59 insertions(+) > >>>>>>>> create mode 100644 > >>>>>>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > >>>>> > >>>>> [snip] > >>>>> > >>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > >>>>>>>> + The first cell is a phandle to a DRM panel device > >>>>>>>> + The second cell encodes the RGB mode, which can take the > >>>>>>>> following values: > >>>>>>>> + * 0: RGB444 > >>>>>>>> + * 1: RGB565 > >>>>>>>> + * 2: RGB666 > >>>>>>>> + * 3: RGB888 > >>>>>>> > >>>>>>> These are properties of the panel and should be obtained from the > >>>>>>> panel directly rather than an additional cell in this specifier. > >>>>>> > >>>>>> Okay. > >>>>>> What's the preferred way of doing this ? > >>>>>> What about defining an rgb-mode property in the panel node. > >>>>> > >>>>> You could do that, but it won't help you much, as the HLCDC driver > >>>>> must not parse properties from the panel node. You should instead > >>>>> extend the drm_panel API with a function to retrieve panel > >>>>> properties. The HLCDC driver will then query the panel driver at > >>>>> runtime for the interface type. The panel driver will get the > >>>>> information from hardcoded data in the driver, from DT or possibly > >>>>> in some cases by querying the panel hardware directly. > >>>> > >>>> My preference for this would be that we either add this to some > >>>> existing structure (struct drm_display_info seems like a good > >>>> candidate) or if the number of parameters grows out of hands, then > >>>> maybe even introduce a new type of device that's specific for the > >>>> interface. DRM panels are an abstraction for panels, that is, > >>>> interface-agnostic, and if we start exposing interface specific > >>>> parameters things will start to become very unwieldy. > >>> > >>> I agree with the goal of keeping drm_panel interface-agnostic. > >>> However, one way or another, interface parameters will need to be > >>> communicated between the panel driver and the controller driver. My > >>> preference, if we need to extend the number and/or scope of parameters > >>> beyond what drm_display_info could reasonably contain, would be to > >>> implement a new drm_panel operation to query/configure interface > >>> parameters, using a structure that contains the interface type and a > >>> union of type-specific structures. This would keep the API generic in > >>> the sense of not requiring explicit knowledge of all interfaces in the > >>> drivers, while offering the flexibility we need with a way to easily > >>> detect the interface type at runtime and react on unknown/unsupported > >>> types. > >> > >> That's exactly what I was hoping could be avoided. If instead we modeled > >> the interface type as a bus, we could for example have an lvds_bus along > >> with an lvds_device and then use that as the natural place to store > >> these properties. Much like we do for DSI. > > > > And I believe that's what we should avoid ;-) First of all, let's not > > forget that Linux models control busses, not data busses. DSI is a > > special case as it combines the control and data busses, but in the > > general case the same implementation isn't possible. An LVDS panel > > controlled through I2C needs to be an I2C device sitting on an I2C bus. > > > > Then, I believe it would make all drivers simpler if we had a single > > object type to deal with, with proper abstractions for bus types. A > > drm_panel that can model panels regardless of the data bus type, with one > > operation that conveys bus-specific information, makes storing the objects > > and communicating with them simpler than having to deal with different > > kind of devices. > > Could you detail a bit what you mean by "single object type" ? > > Is this about making a common abstraction class (by mean of > drm_xxx and drm_xxx_funcs) that could represent any display device > (drm_bridge, drm_panel, ...) ? Exactly :-) This is similar to what exists in V4L, with a v4l2_subdev object able to model any kind of IP core or external chip. I don't think we will get there in one go, but I'd like to start by merging drm_encoder and drm_bridge on the kernel side. Both objects model the same hardware, a drm_encoder on one board could be a drm_bridge on another one. From a userspace point of view drm_encoder won't go away, and we can't chain multiple encoders, so the change would be internal to the kernel only. Then, as a next step, I believe using the same object to model panels would be a good idea, but there's no consensus on that.
On Wed, 16 Jul 2014 15:20:59 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > On Wednesday 16 July 2014 15:05:22 Boris BREZILLON wrote: > > On Tue, 15 Jul 2014 13:07:58 +0200 Laurent Pinchart wrote: > > > On Tuesday 15 July 2014 12:52:54 Thierry Reding wrote: > > >> On Tue, Jul 15, 2014 at 12:43:02PM +0200, Laurent Pinchart wrote: > > >>> On Tuesday 15 July 2014 12:37:19 Thierry Reding wrote: > > >>>> On Tue, Jul 15, 2014 at 12:20:02PM +0200, Laurent Pinchart wrote: > > >>>>> On Tuesday 15 July 2014 12:06:19 Boris BREZILLON wrote: > > >>>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > >>>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > >>>>>>>> The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs > > >>>>>>>> (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides > > >>>>>>>> a display controller device. > > >>>>>>>> > > >>>>>>>> The HLCDC block provides a single RGB output port, and only > > >>>>>>>> supports LCD panels connection to LCD panels for now. > > >>>>>>>> > > >>>>>>>> The atmel,panel property link the HLCDC RGB output with the LCD > > >>>>>>>> panel connected on this port (note that the HLCDC RGB connector > > >>>>>>>> implementation makes use of the DRM panel framework). > > >>>>>>>> > > >>>>>>>> Connection to other external devices (DRM bridges) might be added > > >>>>>>>> later by mean of a new atmel,xxx (atmel,bridge) property. > > >>>>>>>> > > >>>>>>>> Signed-off-by: Boris BREZILLON > > >>>>>>>> <boris.brezillon@free-electrons.com> > > >>>>>>>> --- > > >>>>>>>> > > >>>>>>>> .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++ > > >>>>>>>> 1 file changed, 59 insertions(+) > > >>>>>>>> create mode 100644 > > >>>>>>>> Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > >>>>> > > >>>>> [snip] > > >>>>> > > >>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > >>>>>>>> + The first cell is a phandle to a DRM panel device > > >>>>>>>> + The second cell encodes the RGB mode, which can take the > > >>>>>>>> following values: > > >>>>>>>> + * 0: RGB444 > > >>>>>>>> + * 1: RGB565 > > >>>>>>>> + * 2: RGB666 > > >>>>>>>> + * 3: RGB888 > > >>>>>>> > > >>>>>>> These are properties of the panel and should be obtained from the > > >>>>>>> panel directly rather than an additional cell in this specifier. > > >>>>>> > > >>>>>> Okay. > > >>>>>> What's the preferred way of doing this ? > > >>>>>> What about defining an rgb-mode property in the panel node. > > >>>>> > > >>>>> You could do that, but it won't help you much, as the HLCDC driver > > >>>>> must not parse properties from the panel node. You should instead > > >>>>> extend the drm_panel API with a function to retrieve panel > > >>>>> properties. The HLCDC driver will then query the panel driver at > > >>>>> runtime for the interface type. The panel driver will get the > > >>>>> information from hardcoded data in the driver, from DT or possibly > > >>>>> in some cases by querying the panel hardware directly. > > >>>> > > >>>> My preference for this would be that we either add this to some > > >>>> existing structure (struct drm_display_info seems like a good > > >>>> candidate) or if the number of parameters grows out of hands, then > > >>>> maybe even introduce a new type of device that's specific for the > > >>>> interface. DRM panels are an abstraction for panels, that is, > > >>>> interface-agnostic, and if we start exposing interface specific > > >>>> parameters things will start to become very unwieldy. > > >>> > > >>> I agree with the goal of keeping drm_panel interface-agnostic. > > >>> However, one way or another, interface parameters will need to be > > >>> communicated between the panel driver and the controller driver. My > > >>> preference, if we need to extend the number and/or scope of parameters > > >>> beyond what drm_display_info could reasonably contain, would be to > > >>> implement a new drm_panel operation to query/configure interface > > >>> parameters, using a structure that contains the interface type and a > > >>> union of type-specific structures. This would keep the API generic in > > >>> the sense of not requiring explicit knowledge of all interfaces in the > > >>> drivers, while offering the flexibility we need with a way to easily > > >>> detect the interface type at runtime and react on unknown/unsupported > > >>> types. > > >> > > >> That's exactly what I was hoping could be avoided. If instead we modeled > > >> the interface type as a bus, we could for example have an lvds_bus along > > >> with an lvds_device and then use that as the natural place to store > > >> these properties. Much like we do for DSI. > > > > > > And I believe that's what we should avoid ;-) First of all, let's not > > > forget that Linux models control busses, not data busses. DSI is a > > > special case as it combines the control and data busses, but in the > > > general case the same implementation isn't possible. An LVDS panel > > > controlled through I2C needs to be an I2C device sitting on an I2C bus. > > > > > > Then, I believe it would make all drivers simpler if we had a single > > > object type to deal with, with proper abstractions for bus types. A > > > drm_panel that can model panels regardless of the data bus type, with one > > > operation that conveys bus-specific information, makes storing the objects > > > and communicating with them simpler than having to deal with different > > > kind of devices. > > > > Could you detail a bit what you mean by "single object type" ? > > > > Is this about making a common abstraction class (by mean of > > drm_xxx and drm_xxx_funcs) that could represent any display device > > (drm_bridge, drm_panel, ...) ? > > Exactly :-) This is similar to what exists in V4L, with a v4l2_subdev object > able to model any kind of IP core or external chip. > > I don't think we will get there in one go, but I'd like to start by merging > drm_encoder and drm_bridge on the kernel side. Both objects model the same > hardware, a drm_encoder on one board could be a drm_bridge on another one. > From a userspace point of view drm_encoder won't go away, and we can't chain > multiple encoders, so the change would be internal to the kernel only. > > Then, as a next step, I believe using the same object to model panels would be > a good idea, but there's no consensus on that. > I would be happy to help with that, but AFAICT, this is a huge work and I'd like to get the HLCDC driver merged first ;-). How about defining what DT bindings should look like (for the RGB/LVDS output mode), and parsing this in atmel-hlcdc driver as a first step ? Then we can define proper RGB/LVDS helper functions and the whole drm_subdev abstraction you were talking about, and move the atmel-hlcdc driver to this solution when it's ready.
Hi Thierry, Oops, I missed this reply. On Tue, 15 Jul 2014 12:31:37 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > [...] > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > [...] > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > > > > > I think it's better to refer to these using relative filenames. When the > > > device tree bindings are moved out of the kernel tree, they may no > > > longer use the same hierarchy. > > > > Sure. > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just > > mfd/atmel-hlcdc.txt ? > > I think the former is more explicit. Okay. > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > + The first cell is a phandle to a DRM panel device > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > + * 0: RGB444 > > > > + * 1: RGB565 > > > > + * 2: RGB666 > > > > + * 3: RGB888 > > > > > > These are properties of the panel and should be obtained from the panel > > > directly rather than an additional cell in this specifier. > > > > Okay. > > What's the preferred way of doing this ? > > What about defining an rgb-mode property in the panel node. > > There's .bpc in struct drm_display_info, I suspect that it could be used > for this. Alternatively, maybe we could extend the list of color formats > that go into drm_display_info.color_formats? RGB444 is already covered. I don't think this color_formats field is intended to represent data stream format going through the bus. Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma subsampling rate) and not 12 bits signals (4 bits for each color). Anyway I'll propose a patch series adding a new field to drm_display_info to encode the mediabus format (as discussed with Laurent and you). > > Also, like Laurent said, this shouldn't go into the device tree, since > it's already implied by the panel's compatible value, so we'd be > duplicating information. Again, this is not necessarily true (depending on your board design). One can decide to connect an RGB888 panel on an RGB666 bus and connect the missing pins to ground. > > > BTW, have you received this series [1] adding support for the LCD panel > > I'm testing this driver with. > > > > [1] https://lkml.org/lkml/2014/6/5/612 > > I don't think I've seen it in my inbox, let me check my archives. > > > > > + The third cell encodes specific flags describing LCD signals configuration > > > > + (see Atmel's datasheet for a full description of these fields): > > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > > + * bit 4: DISPPOL: Display Signal Polarity > > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration > > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration > > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the > > > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And > > > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and > > > vsync_end of the same structure. > > > > I agree with HSPOL and VSPOL. > > > > > > > > As for the others, maybe if you could explain what exactly they are we > > > may be able to find a better fit. > > > > Atmel datasheets include several timing diagrams [2] (chapter "32.6.17 > > Output Timing Generation" page 603), and I think you will get more > > informations from these diagrams than if I try to explain what I > > understood ;-). > > These look like knobs to tune the signal in a very fine-grained manner. > To be honest, maybe the best way to solve this would be by omitting them > for now and choose some default that's likely to work on most devices. > Does the panel that you use specify how it expects HSYNC to be timed vs. > VSYNC? No it doesn't, and I agree that we should leave these specific timing tweaks unimplemented until we really need them. Regards, Boris
On Fri, 18 Jul 2014 16:51:52 +0200 Boris BREZILLON <boris.brezillon@free-electrons.com> wrote: > Hi Thierry, > > Oops, I missed this reply. > > On Tue, 15 Jul 2014 12:31:37 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > [...] > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > [...] > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > > > > > > > I think it's better to refer to these using relative filenames. When the > > > > device tree bindings are moved out of the kernel tree, they may no > > > > longer use the same hierarchy. > > > > > > Sure. > > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just > > > mfd/atmel-hlcdc.txt ? > > > > I think the former is more explicit. > > Okay. > > > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > + The first cell is a phandle to a DRM panel device > > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > > + * 0: RGB444 > > > > > + * 1: RGB565 > > > > > + * 2: RGB666 > > > > > + * 3: RGB888 > > > > > > > > These are properties of the panel and should be obtained from the panel > > > > directly rather than an additional cell in this specifier. > > > > > > Okay. > > > What's the preferred way of doing this ? > > > What about defining an rgb-mode property in the panel node. > > > > There's .bpc in struct drm_display_info, I suspect that it could be used > > for this. Alternatively, maybe we could extend the list of color formats > > that go into drm_display_info.color_formats? RGB444 is already covered. > I forgot to ask about bpc meaning. If, as I think, it means "bits per color" then it cannot be used to encode RGB565 where green color is encoded on 6 bits and red and blue are encoded on 5 bits. > I don't think this color_formats field is intended to represent data > stream format going through the bus. > Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma > subsampling rate) and not 12 bits signals (4 bits for each color). > > Anyway I'll propose a patch series adding a new field to > drm_display_info to encode the mediabus format (as discussed with > Laurent and you). > > > > > Also, like Laurent said, this shouldn't go into the device tree, since > > it's already implied by the panel's compatible value, so we'd be > > duplicating information. > > Again, this is not necessarily true (depending on your board design). > One can decide to connect an RGB888 panel on an RGB666 bus and connect > the missing pins to ground. > > > > > > BTW, have you received this series [1] adding support for the LCD panel > > > I'm testing this driver with. > > > > > > [1] https://lkml.org/lkml/2014/6/5/612 > > > > I don't think I've seen it in my inbox, let me check my archives. > > > > > > > + The third cell encodes specific flags describing LCD signals configuration > > > > > + (see Atmel's datasheet for a full description of these fields): > > > > > + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity > > > > > + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity > > > > > + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start > > > > > + * bit 3: VSPDLYE: Vertical Synchronization Pulse End > > > > > + * bit 4: DISPPOL: Display Signal Polarity > > > > > + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization > > > > > + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration > > > > > + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration > > > > > + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time > > > > > > > > Similarly for most of these: HSPOL and VSPOL seem to correspond to the > > > > DRM_MODE_FLAG_{{P,N},{H,V}}SYNC flags in struct drm_display_mode. And > > > > VSPDLYS as well as VSPDLYE sound like they may be vsync_start and > > > > vsync_end of the same structure. > > > > > > I agree with HSPOL and VSPOL. > > > > > > > > > > > As for the others, maybe if you could explain what exactly they are we > > > > may be able to find a better fit. > > > > > > Atmel datasheets include several timing diagrams [2] (chapter "32.6.17 > > > Output Timing Generation" page 603), and I think you will get more > > > informations from these diagrams than if I try to explain what I > > > understood ;-). > > > > These look like knobs to tune the signal in a very fine-grained manner. > > To be honest, maybe the best way to solve this would be by omitting them > > for now and choose some default that's likely to work on most devices. > > Does the panel that you use specify how it expects HSYNC to be timed vs. > > VSYNC? > > > No it doesn't, and I agree that we should leave these specific timing > tweaks unimplemented until we really need them. > > Regards, > > Boris > >
On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: [...] > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > > + The first cell is a phandle to a DRM panel device > > > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > > > + * 0: RGB444 > > > > > > + * 1: RGB565 > > > > > > + * 2: RGB666 > > > > > > + * 3: RGB888 > > > > > > > > > > These are properties of the panel and should be obtained from the panel > > > > > directly rather than an additional cell in this specifier. > > > > > > > > Okay. > > > > What's the preferred way of doing this ? > > > > What about defining an rgb-mode property in the panel node. > > > > > > There's .bpc in struct drm_display_info, I suspect that it could be used > > > for this. Alternatively, maybe we could extend the list of color formats > > > that go into drm_display_info.color_formats? RGB444 is already covered. > > > > I forgot to ask about bpc meaning. If, as I think, it means "bits per > color" then it cannot be used to encode RGB565 where green color is > encoded on 6 bits and red and blue are encoded on 5 bits. Yes, I agree that bps is not a good fit for what you need here. Thierry
On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > [...] > > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > > > + The first cell is a phandle to a DRM panel device > > > > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > > > > + * 0: RGB444 > > > > > > > + * 1: RGB565 > > > > > > > + * 2: RGB666 > > > > > > > + * 3: RGB888 > > > > > > > > > > > > These are properties of the panel and should be obtained from the panel > > > > > > directly rather than an additional cell in this specifier. > > > > > > > > > > Okay. > > > > > What's the preferred way of doing this ? > > > > > What about defining an rgb-mode property in the panel node. > > > > > > > > There's .bpc in struct drm_display_info, I suspect that it could be used > > > > for this. Alternatively, maybe we could extend the list of color formats > > > > that go into drm_display_info.color_formats? RGB444 is already covered. > > > > > > > I forgot to ask about bpc meaning. If, as I think, it means "bits per > > color" then it cannot be used to encode RGB565 where green color is > > encoded on 6 bits and red and blue are encoded on 5 bits. > > Yes, I agree that bps is not a good fit for what you need here. Okay, then I think we can replace bpc and color_formats by a bus_formats table containing all supported formats, and use an enum (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list the available formats. As this implies quite a few changes in crtc core and some drm drivers (nouveau, i915 and radeon), I'd like to be sure this is what both of you had in mind. Best Regards, Boris [1]http://lxr.free-electrons.com/source/include/uapi/linux/v4l2-mediabus.h#L37
Hi Boris, On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote: > On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote: > > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > > [...] > > > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > >>>>>>> + The first cell is a phandle to a DRM panel device > >>>>>>> + The second cell encodes the RGB mode, which can take the > >>>>>>> following values: > >>>>>>> + * 0: RGB444 > >>>>>>> + * 1: RGB565 > >>>>>>> + * 2: RGB666 > >>>>>>> + * 3: RGB888 > >>>>>> > >>>>>> These are properties of the panel and should be obtained from > >>>>>> the panel directly rather than an additional cell in this specifier. > >>>>> > >>>>> Okay. > >>>>> What's the preferred way of doing this ? > >>>>> What about defining an rgb-mode property in the panel node. > >>>> > >>>> There's .bpc in struct drm_display_info, I suspect that it could be > >>>> used for this. Alternatively, maybe we could extend the list of color > >>>> formats that go into drm_display_info.color_formats? RGB444 is already > >>>> covered. > >> > >> I forgot to ask about bpc meaning. If, as I think, it means "bits per > >> color" then it cannot be used to encode RGB565 where green color is > >> encoded on 6 bits and red and blue are encoded on 5 bits. > > > > Yes, I agree that bps is not a good fit for what you need here. > > Okay, then I think we can replace bpc and color_formats by a bus_formats > table containing all supported formats, and use an enum (something > similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list > the available formats. > > As this implies quite a few changes in crtc core and some drm drivers > (nouveau, i915 and radeon), I'd like to be sure this is what both of you > had in mind. I think it is, but just to make sure I understand you correctly, could you just show how the drm_display_info structure would look like ?
On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote: > > > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > > > [...] > > > > > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > >>>>>>> + The first cell is a phandle to a DRM panel device > > >>>>>>> + The second cell encodes the RGB mode, which can take the > > >>>>>>> following values: > > >>>>>>> + * 0: RGB444 > > >>>>>>> + * 1: RGB565 > > >>>>>>> + * 2: RGB666 > > >>>>>>> + * 3: RGB888 > > >>>>>> > > >>>>>> These are properties of the panel and should be obtained from > > >>>>>> the panel directly rather than an additional cell in this specifier. > > >>>>> > > >>>>> Okay. > > >>>>> What's the preferred way of doing this ? > > >>>>> What about defining an rgb-mode property in the panel node. > > >>>> > > >>>> There's .bpc in struct drm_display_info, I suspect that it could be > > >>>> used for this. Alternatively, maybe we could extend the list of color > > >>>> formats that go into drm_display_info.color_formats? RGB444 is already > > >>>> covered. > > >> > > >> I forgot to ask about bpc meaning. If, as I think, it means "bits per > > >> color" then it cannot be used to encode RGB565 where green color is > > >> encoded on 6 bits and red and blue are encoded on 5 bits. > > > > > > Yes, I agree that bps is not a good fit for what you need here. > > > > Okay, then I think we can replace bpc and color_formats by a bus_formats > > table containing all supported formats, and use an enum (something > > similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list > > the available formats. > > > > As this implies quite a few changes in crtc core and some drm drivers > > (nouveau, i915 and radeon), I'd like to be sure this is what both of you > > had in mind. > > I think it is, but just to make sure I understand you correctly, could you > just show how the drm_display_info structure would look like ? > The new drm_display_info structure should look like this [2] (except that color_formats and bpc have not be removed yet), and [1] is just here to show how the video_bus_format enum would look like. [1] http://code.bulix.org/rfd0yx-86557 [2] http://code.bulix.org/7n03b4-86556
On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > On Mon, 21 Jul 2014 11:32:55 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > > Hi Boris, > > > > On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote: > > > On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote: > > > > On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > > > > [...] > > > > > > > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > > >>>>>>> + The first cell is a phandle to a DRM panel device > > > >>>>>>> + The second cell encodes the RGB mode, which can take the > > > >>>>>>> following values: > > > >>>>>>> + * 0: RGB444 > > > >>>>>>> + * 1: RGB565 > > > >>>>>>> + * 2: RGB666 > > > >>>>>>> + * 3: RGB888 > > > >>>>>> > > > >>>>>> These are properties of the panel and should be obtained from > > > >>>>>> the panel directly rather than an additional cell in this specifier. > > > >>>>> > > > >>>>> Okay. > > > >>>>> What's the preferred way of doing this ? > > > >>>>> What about defining an rgb-mode property in the panel node. > > > >>>> > > > >>>> There's .bpc in struct drm_display_info, I suspect that it could be > > > >>>> used for this. Alternatively, maybe we could extend the list of color > > > >>>> formats that go into drm_display_info.color_formats? RGB444 is already > > > >>>> covered. > > > >> > > > >> I forgot to ask about bpc meaning. If, as I think, it means "bits per > > > >> color" then it cannot be used to encode RGB565 where green color is > > > >> encoded on 6 bits and red and blue are encoded on 5 bits. > > > > > > > > Yes, I agree that bps is not a good fit for what you need here. > > > > > > Okay, then I think we can replace bpc and color_formats by a bus_formats > > > table containing all supported formats, and use an enum (something > > > similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h [1]) to list > > > the available formats. > > > > > > As this implies quite a few changes in crtc core and some drm drivers > > > (nouveau, i915 and radeon), I'd like to be sure this is what both of you > > > had in mind. > > > > I think it is, but just to make sure I understand you correctly, could you > > just show how the drm_display_info structure would look like ? > > > > The new drm_display_info structure should look like this [2] (except > that color_formats and bpc have not be removed yet), and [1] is just > here to show how the video_bus_format enum would look like. > > [1] http://code.bulix.org/rfd0yx-86557 > [2] http://code.bulix.org/7n03b4-86556 Quoting from your paste: + const enum video_bus_format *bus_formats; + int nbus_formats; Do we really need more than one? Thierry
On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote: > Hi Thierry, > > Oops, I missed this reply. > > On Tue, 15 Jul 2014 12:31:37 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > [...] > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > [...] > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > > > > > > > I think it's better to refer to these using relative filenames. When the > > > > device tree bindings are moved out of the kernel tree, they may no > > > > longer use the same hierarchy. > > > > > > Sure. > > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just > > > mfd/atmel-hlcdc.txt ? > > > > I think the former is more explicit. > > Okay. > > > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > + The first cell is a phandle to a DRM panel device > > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > > + * 0: RGB444 > > > > > + * 1: RGB565 > > > > > + * 2: RGB666 > > > > > + * 3: RGB888 > > > > > > > > These are properties of the panel and should be obtained from the panel > > > > directly rather than an additional cell in this specifier. > > > > > > Okay. > > > What's the preferred way of doing this ? > > > What about defining an rgb-mode property in the panel node. > > > > There's .bpc in struct drm_display_info, I suspect that it could be used > > for this. Alternatively, maybe we could extend the list of color formats > > that go into drm_display_info.color_formats? RGB444 is already covered. > > I don't think this color_formats field is intended to represent data > stream format going through the bus. > Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma > subsampling rate) and not 12 bits signals (4 bits for each color). > > Anyway I'll propose a patch series adding a new field to > drm_display_info to encode the mediabus format (as discussed with > Laurent and you). > > > > > Also, like Laurent said, this shouldn't go into the device tree, since > > it's already implied by the panel's compatible value, so we'd be > > duplicating information. > > Again, this is not necessarily true (depending on your board design). > One can decide to connect an RGB888 panel on an RGB666 bus and connect > the missing pins to ground. I think in that case the board design itself could be considered as an RGB888 to RGB666 bridge, and I think that's what the device tree should be describing rather than a panel with a variable number of input formats. Thierry
Hi Thierry, On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart wrote: > >> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote: > >>> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote: > >>>> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > >>>> [...] > >>>> > >>>>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > >>>>>>>>>> + The first cell is a phandle to a DRM panel device > >>>>>>>>>> + The second cell encodes the RGB mode, which can take the > >>>>>>>>>> following values: > >>>>>>>>>> + * 0: RGB444 > >>>>>>>>>> + * 1: RGB565 > >>>>>>>>>> + * 2: RGB666 > >>>>>>>>>> + * 3: RGB888 > >>>>>>>>> > >>>>>>>>> These are properties of the panel and should be obtained from > >>>>>>>>> the panel directly rather than an additional cell in this > >>>>>>>>> specifier. > >>>>>>>> > >>>>>>>> Okay. > >>>>>>>> What's the preferred way of doing this ? > >>>>>>>> What about defining an rgb-mode property in the panel node. > >>>>>>> > >>>>>>> There's .bpc in struct drm_display_info, I suspect that it could > >>>>>>> be used for this. Alternatively, maybe we could extend the list > >>>>>>> of color formats that go into drm_display_info.color_formats? > >>>>>>> RGB444 is already covered. > >>>>> > >>>>> I forgot to ask about bpc meaning. If, as I think, it means "bits > >>>>> per color" then it cannot be used to encode RGB565 where green > >>>>> color is encoded on 6 bits and red and blue are encoded on 5 bits. > >>>> > >>>> Yes, I agree that bps is not a good fit for what you need here. > >>> > >>> Okay, then I think we can replace bpc and color_formats by a > >>> bus_formats table containing all supported formats, and use an enum > >>> (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h > >>> [1]) to list the available formats. > >>> > >>> As this implies quite a few changes in crtc core and some drm drivers > >>> (nouveau, i915 and radeon), I'd like to be sure this is what both of > >>> you had in mind. > >> > >> I think it is, but just to make sure I understand you correctly, could > >> you just show how the drm_display_info structure would look like ? > > > > The new drm_display_info structure should look like this [2] (except > > that color_formats and bpc have not be removed yet), and [1] is just > > here to show how the video_bus_format enum would look like. > > > > [1] http://code.bulix.org/rfd0yx-86557 > > [2] http://code.bulix.org/7n03b4-86556 > > Quoting from your paste: > > + const enum video_bus_format *bus_formats; > + int nbus_formats; > > Do we really need more than one? We do if we want to replace the color_formats and bpc fields.
On Mon, 21 Jul 2014 14:15:16 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote: > > Hi Thierry, > > > > Oops, I missed this reply. > > > > On Tue, 15 Jul 2014 12:31:37 +0200 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > > > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > > [...] > > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > [...] > > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > > > > > > > > > I think it's better to refer to these using relative filenames. When the > > > > > device tree bindings are moved out of the kernel tree, they may no > > > > > longer use the same hierarchy. > > > > > > > > Sure. > > > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just > > > > mfd/atmel-hlcdc.txt ? > > > > > > I think the former is more explicit. > > > > Okay. > > > > > > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > > + The first cell is a phandle to a DRM panel device > > > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > > > + * 0: RGB444 > > > > > > + * 1: RGB565 > > > > > > + * 2: RGB666 > > > > > > + * 3: RGB888 > > > > > > > > > > These are properties of the panel and should be obtained from the panel > > > > > directly rather than an additional cell in this specifier. > > > > > > > > Okay. > > > > What's the preferred way of doing this ? > > > > What about defining an rgb-mode property in the panel node. > > > > > > There's .bpc in struct drm_display_info, I suspect that it could be used > > > for this. Alternatively, maybe we could extend the list of color formats > > > that go into drm_display_info.color_formats? RGB444 is already covered. > > > > I don't think this color_formats field is intended to represent data > > stream format going through the bus. > > Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma > > subsampling rate) and not 12 bits signals (4 bits for each color). > > > > Anyway I'll propose a patch series adding a new field to > > drm_display_info to encode the mediabus format (as discussed with > > Laurent and you). > > > > > > > > Also, like Laurent said, this shouldn't go into the device tree, since > > > it's already implied by the panel's compatible value, so we'd be > > > duplicating information. > > > > Again, this is not necessarily true (depending on your board design). > > One can decide to connect an RGB888 panel on an RGB666 bus and connect > > the missing pins to ground. > > I think in that case the board design itself could be considered as an > RGB888 to RGB666 bridge, and I think that's what the device tree should > be describing rather than a panel with a variable number of input > formats. So, you're suggesting to add an RGB to RGB drm_bridge driver (and the appropriate DT bindings) to handle this case, right ? I don't know much about drm bridges, but I'll take a look. Anyway, at the moment I don't have such hardware (one connecting an RGB888 panel on an RGB666 bus), I was just wondering how I could represent it ;-). Thanks, Boris
On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Thierry, > > On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > > On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart wrote: > > >> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote: > > >>> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote: > > >>>> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > > >>>> [...] > > >>>> > > >>>>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > >>>>>>>>>> + The first cell is a phandle to a DRM panel device > > >>>>>>>>>> + The second cell encodes the RGB mode, which can take the > > >>>>>>>>>> following values: > > >>>>>>>>>> + * 0: RGB444 > > >>>>>>>>>> + * 1: RGB565 > > >>>>>>>>>> + * 2: RGB666 > > >>>>>>>>>> + * 3: RGB888 > > >>>>>>>>> > > >>>>>>>>> These are properties of the panel and should be obtained from > > >>>>>>>>> the panel directly rather than an additional cell in this > > >>>>>>>>> specifier. > > >>>>>>>> > > >>>>>>>> Okay. > > >>>>>>>> What's the preferred way of doing this ? > > >>>>>>>> What about defining an rgb-mode property in the panel node. > > >>>>>>> > > >>>>>>> There's .bpc in struct drm_display_info, I suspect that it could > > >>>>>>> be used for this. Alternatively, maybe we could extend the list > > >>>>>>> of color formats that go into drm_display_info.color_formats? > > >>>>>>> RGB444 is already covered. > > >>>>> > > >>>>> I forgot to ask about bpc meaning. If, as I think, it means "bits > > >>>>> per color" then it cannot be used to encode RGB565 where green > > >>>>> color is encoded on 6 bits and red and blue are encoded on 5 bits. > > >>>> > > >>>> Yes, I agree that bps is not a good fit for what you need here. > > >>> > > >>> Okay, then I think we can replace bpc and color_formats by a > > >>> bus_formats table containing all supported formats, and use an enum > > >>> (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h > > >>> [1]) to list the available formats. > > >>> > > >>> As this implies quite a few changes in crtc core and some drm drivers > > >>> (nouveau, i915 and radeon), I'd like to be sure this is what both of > > >>> you had in mind. > > >> > > >> I think it is, but just to make sure I understand you correctly, could > > >> you just show how the drm_display_info structure would look like ? > > > > > > The new drm_display_info structure should look like this [2] (except > > > that color_formats and bpc have not be removed yet), and [1] is just > > > here to show how the video_bus_format enum would look like. > > > > > > [1] http://code.bulix.org/rfd0yx-86557 > > > [2] http://code.bulix.org/7n03b4-86556 > > > > Quoting from your paste: > > > > + const enum video_bus_format *bus_formats; > > + int nbus_formats; > > > > Do we really need more than one? > > We do if we want to replace the color_formats and bpc fields. > Yes, that's what I was about to answer :-).
On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > On Mon, 21 Jul 2014 14:16:42 +0200 > Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > > Hi Thierry, > > > > On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > > On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > > > On Mon, 21 Jul 2014 11:32:55 +0200 Laurent Pinchart wrote: > > > >> On Monday 21 July 2014 11:24:38 Boris BREZILLON wrote: > > > >>> On Mon, 21 Jul 2014 10:59:12 +0200 Thierry Reding wrote: > > > >>>> On Fri, Jul 18, 2014 at 05:43:34PM +0200, Boris BREZILLON wrote: > > > >>>> [...] > > > >>>> > > > >>>>>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > > >>>>>>>>>> + The first cell is a phandle to a DRM panel device > > > >>>>>>>>>> + The second cell encodes the RGB mode, which can take the > > > >>>>>>>>>> following values: > > > >>>>>>>>>> + * 0: RGB444 > > > >>>>>>>>>> + * 1: RGB565 > > > >>>>>>>>>> + * 2: RGB666 > > > >>>>>>>>>> + * 3: RGB888 > > > >>>>>>>>> > > > >>>>>>>>> These are properties of the panel and should be obtained from > > > >>>>>>>>> the panel directly rather than an additional cell in this > > > >>>>>>>>> specifier. > > > >>>>>>>> > > > >>>>>>>> Okay. > > > >>>>>>>> What's the preferred way of doing this ? > > > >>>>>>>> What about defining an rgb-mode property in the panel node. > > > >>>>>>> > > > >>>>>>> There's .bpc in struct drm_display_info, I suspect that it could > > > >>>>>>> be used for this. Alternatively, maybe we could extend the list > > > >>>>>>> of color formats that go into drm_display_info.color_formats? > > > >>>>>>> RGB444 is already covered. > > > >>>>> > > > >>>>> I forgot to ask about bpc meaning. If, as I think, it means "bits > > > >>>>> per color" then it cannot be used to encode RGB565 where green > > > >>>>> color is encoded on 6 bits and red and blue are encoded on 5 bits. > > > >>>> > > > >>>> Yes, I agree that bps is not a good fit for what you need here. > > > >>> > > > >>> Okay, then I think we can replace bpc and color_formats by a > > > >>> bus_formats table containing all supported formats, and use an enum > > > >>> (something similar to v4l2_mbus_pixelcode defined in v4l2-mediabus.h > > > >>> [1]) to list the available formats. > > > >>> > > > >>> As this implies quite a few changes in crtc core and some drm drivers > > > >>> (nouveau, i915 and radeon), I'd like to be sure this is what both of > > > >>> you had in mind. > > > >> > > > >> I think it is, but just to make sure I understand you correctly, could > > > >> you just show how the drm_display_info structure would look like ? > > > > > > > > The new drm_display_info structure should look like this [2] (except > > > > that color_formats and bpc have not be removed yet), and [1] is just > > > > here to show how the video_bus_format enum would look like. > > > > > > > > [1] http://code.bulix.org/rfd0yx-86557 > > > > [2] http://code.bulix.org/7n03b4-86556 > > > > > > Quoting from your paste: > > > > > > + const enum video_bus_format *bus_formats; > > > + int nbus_formats; > > > > > > Do we really need more than one? > > > > We do if we want to replace the color_formats and bpc fields. > > > > Yes, that's what I was about to answer :-). Maybe we don't need to replace color_formats and bpc field immediately. That could be done in a follow-up patch. Thierry
On Mon, Jul 21, 2014 at 02:33:21PM +0200, Boris BREZILLON wrote: > On Mon, 21 Jul 2014 14:15:16 +0200 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote: > > > Hi Thierry, > > > > > > Oops, I missed this reply. > > > > > > On Tue, 15 Jul 2014 12:31:37 +0200 > > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > > On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > > > > > On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > > > [...] > > > > > > > diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt > > > > > > [...] > > > > > > > +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. > > > > > > > +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. > > > > > > > > > > > > I think it's better to refer to these using relative filenames. When the > > > > > > device tree bindings are moved out of the kernel tree, they may no > > > > > > longer use the same hierarchy. > > > > > > > > > > Sure. > > > > > By relative path you mean ../../mfd/atmel-hlcdc.txt or just > > > > > mfd/atmel-hlcdc.txt ? > > > > > > > > I think the former is more explicit. > > > > > > Okay. > > > > > > > > > > > > > > + - atmel,panel: Should contain a phandle with 2 parameters. > > > > > > > + The first cell is a phandle to a DRM panel device > > > > > > > + The second cell encodes the RGB mode, which can take the following values: > > > > > > > + * 0: RGB444 > > > > > > > + * 1: RGB565 > > > > > > > + * 2: RGB666 > > > > > > > + * 3: RGB888 > > > > > > > > > > > > These are properties of the panel and should be obtained from the panel > > > > > > directly rather than an additional cell in this specifier. > > > > > > > > > > Okay. > > > > > What's the preferred way of doing this ? > > > > > What about defining an rgb-mode property in the panel node. > > > > > > > > There's .bpc in struct drm_display_info, I suspect that it could be used > > > > for this. Alternatively, maybe we could extend the list of color formats > > > > that go into drm_display_info.color_formats? RGB444 is already covered. > > > > > > I don't think this color_formats field is intended to represent data > > > stream format going through the bus. > > > Moreover, AFAIU, RGB444 in this definition represent RGB 4:4:4 (chroma > > > subsampling rate) and not 12 bits signals (4 bits for each color). > > > > > > Anyway I'll propose a patch series adding a new field to > > > drm_display_info to encode the mediabus format (as discussed with > > > Laurent and you). > > > > > > > > > > > Also, like Laurent said, this shouldn't go into the device tree, since > > > > it's already implied by the panel's compatible value, so we'd be > > > > duplicating information. > > > > > > Again, this is not necessarily true (depending on your board design). > > > One can decide to connect an RGB888 panel on an RGB666 bus and connect > > > the missing pins to ground. > > > > I think in that case the board design itself could be considered as an > > RGB888 to RGB666 bridge, and I think that's what the device tree should > > be describing rather than a panel with a variable number of input > > formats. > > So, you're suggesting to add an RGB to RGB drm_bridge driver (and > the appropriate DT bindings) to handle this case, right ? Yes, exactly. Thierry
Hi Thierry, On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > >> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > >>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: [snip] > >>>> The new drm_display_info structure should look like this [2] (except > >>>> that color_formats and bpc have not be removed yet), and [1] is just > >>>> here to show how the video_bus_format enum would look like. > >>>> > >>>> [1] http://code.bulix.org/rfd0yx-86557 > >>>> [2] http://code.bulix.org/7n03b4-86556 > >>> > >>> Quoting from your paste: > >>> + const enum video_bus_format *bus_formats; > >>> + int nbus_formats; > >>> > >>> Do we really need more than one? > >> > >> We do if we want to replace the color_formats and bpc fields. > > > > Yes, that's what I was about to answer :-). > > Maybe we don't need to replace color_formats and bpc field immediately. > That could be done in a follow-up patch. We don't need to replace them right now, but we should at least agree on how to replace them. Introducing a new field that would need to be replaced in the near future when removing color_formats and bpc would be a waste of time.
Hi Thierry, On Monday 21 July 2014 14:56:26 Thierry Reding wrote: > On Mon, Jul 21, 2014 at 02:33:21PM +0200, Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 14:15:16 +0200 Thierry Reding wrote: > >> On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote: > >>> On Tue, 15 Jul 2014 12:31:37 +0200 Thierry Reding wrote: > >>>> On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > >>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > >>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: [snip] > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > >>>>>>> + The first cell is a phandle to a DRM panel device > >>>>>>> + The second cell encodes the RGB mode, which can take the > >>>>>>> following values: + * 0: RGB444 > >>>>>>> + * 1: RGB565 > >>>>>>> + * 2: RGB666 > >>>>>>> + * 3: RGB888 > >>>>>> > >>>>>> These are properties of the panel and should be obtained from > >>>>>> the panel directly rather than an additional cell in this > >>>>>> specifier. > >>>>> > >>>>> Okay. > >>>>> What's the preferred way of doing this ? > >>>>> What about defining an rgb-mode property in the panel node. [snip] > >>>> Also, like Laurent said, this shouldn't go into the device tree, > >>>> since it's already implied by the panel's compatible value, so we'd > >>>> be duplicating information. > >>> > >>> Again, this is not necessarily true (depending on your board design). > >>> One can decide to connect an RGB888 panel on an RGB666 bus and connect > >>> the missing pins to ground. > >> > >> I think in that case the board design itself could be considered as an > >> RGB888 to RGB666 bridge, and I think that's what the device tree should > >> be describing rather than a panel with a variable number of input > >> formats. > > > > So, you're suggesting to add an RGB to RGB drm_bridge driver (and > > the appropriate DT bindings) to handle this case, right ? > > Yes, exactly. Wouldn't it be possible to implement RGB666 -> RGB888 support in a less complex way ? A standalone driver to describe signal routing seems like an overly complex solution to me. I would prefer making the routing a properly of the link instead of a separate device.
On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > > On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > >> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > >>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > [snip] > > > >>>> The new drm_display_info structure should look like this [2] (except > > >>>> that color_formats and bpc have not be removed yet), and [1] is just > > >>>> here to show how the video_bus_format enum would look like. > > >>>> > > >>>> [1] http://code.bulix.org/rfd0yx-86557 > > >>>> [2] http://code.bulix.org/7n03b4-86556 > > >>> > > >>> Quoting from your paste: > > >>> + const enum video_bus_format *bus_formats; > > >>> + int nbus_formats; > > >>> > > >>> Do we really need more than one? > > >> > > >> We do if we want to replace the color_formats and bpc fields. > > > > > > Yes, that's what I was about to answer :-). > > > > Maybe we don't need to replace color_formats and bpc field immediately. > > That could be done in a follow-up patch. > > We don't need to replace them right now, but we should at least agree on how > to replace them. Introducing a new field that would need to be replaced in the > near future when removing color_formats and bpc would be a waste of time. Sure. One of the problems I see with replacing color_formats and bpc with the above is that some of the bits within color_formats are set when the EDID is parsed. That implies that if they are replaced with an array of formats, the array would need to be reallocated during EDID parsing. That sounds like ugliness. But if you can find a nice way to make it work that'd be great. Thierry
On Mon, Jul 21, 2014 at 03:26:12PM +0200, Laurent Pinchart wrote: > Hi Thierry, > > On Monday 21 July 2014 14:56:26 Thierry Reding wrote: > > On Mon, Jul 21, 2014 at 02:33:21PM +0200, Boris BREZILLON wrote: > > > On Mon, 21 Jul 2014 14:15:16 +0200 Thierry Reding wrote: > > >> On Fri, Jul 18, 2014 at 04:51:52PM +0200, Boris BREZILLON wrote: > > >>> On Tue, 15 Jul 2014 12:31:37 +0200 Thierry Reding wrote: > > >>>> On Tue, Jul 15, 2014 at 12:06:19PM +0200, Boris BREZILLON wrote: > > >>>>> On Mon, 14 Jul 2014 12:05:43 +0200 Thierry Reding wrote: > > >>>>>> On Mon, Jul 07, 2014 at 06:42:59PM +0200, Boris BREZILLON wrote: > > [snip] > > > >>>>>>> + - atmel,panel: Should contain a phandle with 2 parameters. > > >>>>>>> + The first cell is a phandle to a DRM panel device > > >>>>>>> + The second cell encodes the RGB mode, which can take the > > >>>>>>> following values: + * 0: RGB444 > > >>>>>>> + * 1: RGB565 > > >>>>>>> + * 2: RGB666 > > >>>>>>> + * 3: RGB888 > > >>>>>> > > >>>>>> These are properties of the panel and should be obtained from > > >>>>>> the panel directly rather than an additional cell in this > > >>>>>> specifier. > > >>>>> > > >>>>> Okay. > > >>>>> What's the preferred way of doing this ? > > >>>>> What about defining an rgb-mode property in the panel node. > > [snip] > > > >>>> Also, like Laurent said, this shouldn't go into the device tree, > > >>>> since it's already implied by the panel's compatible value, so we'd > > >>>> be duplicating information. > > >>> > > >>> Again, this is not necessarily true (depending on your board design). > > >>> One can decide to connect an RGB888 panel on an RGB666 bus and connect > > >>> the missing pins to ground. > > >> > > >> I think in that case the board design itself could be considered as an > > >> RGB888 to RGB666 bridge, and I think that's what the device tree should > > >> be describing rather than a panel with a variable number of input > > >> formats. > > > > > > So, you're suggesting to add an RGB to RGB drm_bridge driver (and > > > the appropriate DT bindings) to handle this case, right ? > > > > Yes, exactly. > > Wouldn't it be possible to implement RGB666 -> RGB888 support in a less > complex way ? A standalone driver to describe signal routing seems like an > overly complex solution to me. I would prefer making the routing a properly of > the link instead of a separate device. I don't think the above is overly complex. After all the panel expects RGB888, so it makes no sense to make it "configurable" to anything else. Similarly if the encoder or bridge provides RGB666 then that's a fixed function, too. So to represent this combination accurately you'll need some form of translation entity inbetween, and it may just as well be a bridge than something custom for that particular link. Thierry
On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > > Hi Thierry, > > > > On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > > On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > > > On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > > >> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > > >>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > > > [snip] > > > > > >>>> The new drm_display_info structure should look like this [2] (except > > > >>>> that color_formats and bpc have not be removed yet), and [1] is just > > > >>>> here to show how the video_bus_format enum would look like. > > > >>>> > > > >>>> [1] http://code.bulix.org/rfd0yx-86557 > > > >>>> [2] http://code.bulix.org/7n03b4-86556 > > > >>> > > > >>> Quoting from your paste: > > > >>> + const enum video_bus_format *bus_formats; > > > >>> + int nbus_formats; > > > >>> > > > >>> Do we really need more than one? > > > >> > > > >> We do if we want to replace the color_formats and bpc fields. > > > > > > > > Yes, that's what I was about to answer :-). > > > > > > Maybe we don't need to replace color_formats and bpc field immediately. > > > That could be done in a follow-up patch. > > > > We don't need to replace them right now, but we should at least agree on how > > to replace them. Introducing a new field that would need to be replaced in the > > near future when removing color_formats and bpc would be a waste of time. > > Sure. One of the problems I see with replacing color_formats and bpc > with the above is that some of the bits within color_formats are set > when the EDID is parsed. That implies that if they are replaced with > an array of formats, the array would need to be reallocated during EDID > parsing. That sounds like ugliness. > > But if you can find a nice way to make it work that'd be great. How about using a list instead of an array ? This way we can add elements to this list when parsing the EDID. Or we can just define a maximum size for the bus_formats array when retrieving this info from EDID. If I'm correct we have at most 18 bus formats: - 3 color formats: * RGB 4:4:4 * YCbCr 4:4:4 * YCbCr 4:4:2 - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color
Hi Boris, On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > >> > >> [snip] > >> > >>>>>>> The new drm_display_info structure should look like this [2] > >>>>>>> (except that color_formats and bpc have not be removed yet), and > >>>>>>> [1] is just here to show how the video_bus_format enum would look > >>>>>>> like. > >>>>>>> > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557 > >>>>>>> [2] http://code.bulix.org/7n03b4-86556 > >>>>>> > >>>>>> Quoting from your paste: > >>>>>> + const enum video_bus_format *bus_formats; > >>>>>> + int nbus_formats; > >>>>>> > >>>>>> Do we really need more than one? > >>>>> > >>>>> We do if we want to replace the color_formats and bpc fields. > >>>> > >>>> Yes, that's what I was about to answer :-). > >>> > >>> Maybe we don't need to replace color_formats and bpc field > >>> immediately. That could be done in a follow-up patch. > >> > >> We don't need to replace them right now, but we should at least agree on > >> how to replace them. Introducing a new field that would need to be > >> replaced in the near future when removing color_formats and bpc would > >> be a waste of time. > > > > Sure. One of the problems I see with replacing color_formats and bpc > > with the above is that some of the bits within color_formats are set > > when the EDID is parsed. That implies that if they are replaced with > > an array of formats, the array would need to be reallocated during EDID > > parsing. That sounds like ugliness. > > > > But if you can find a nice way to make it work that'd be great. > > How about using a list instead of an array ? > This way we can add elements to this list when parsing the EDID. > > Or we can just define a maximum size for the bus_formats array when > retrieving this info from EDID. If I'm correct we have at most 18 bus > formats: > - 3 color formats: > * RGB 4:4:4 > * YCbCr 4:4:4 > * YCbCr 4:4:2 > - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color bpc isn't a bitmask, so EDID supports up to three formats only. The color_formats field is computed in the drm_add_display_info() function. You could easily turn it into a local variable and allocate and fill the formats array at the end of the function.
On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote: > Hi Boris, > > On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > >> > > >> [snip] > > >> > > >>>>>>> The new drm_display_info structure should look like this [2] > > >>>>>>> (except that color_formats and bpc have not be removed yet), and > > >>>>>>> [1] is just here to show how the video_bus_format enum would look > > >>>>>>> like. > > >>>>>>> > > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557 > > >>>>>>> [2] http://code.bulix.org/7n03b4-86556 > > >>>>>> > > >>>>>> Quoting from your paste: > > >>>>>> + const enum video_bus_format *bus_formats; > > >>>>>> + int nbus_formats; > > >>>>>> > > >>>>>> Do we really need more than one? > > >>>>> > > >>>>> We do if we want to replace the color_formats and bpc fields. > > >>>> > > >>>> Yes, that's what I was about to answer :-). > > >>> > > >>> Maybe we don't need to replace color_formats and bpc field > > >>> immediately. That could be done in a follow-up patch. > > >> > > >> We don't need to replace them right now, but we should at least agree on > > >> how to replace them. Introducing a new field that would need to be > > >> replaced in the near future when removing color_formats and bpc would > > >> be a waste of time. > > > > > > Sure. One of the problems I see with replacing color_formats and bpc > > > with the above is that some of the bits within color_formats are set > > > when the EDID is parsed. That implies that if they are replaced with > > > an array of formats, the array would need to be reallocated during EDID > > > parsing. That sounds like ugliness. > > > > > > But if you can find a nice way to make it work that'd be great. > > > > How about using a list instead of an array ? > > This way we can add elements to this list when parsing the EDID. > > > > Or we can just define a maximum size for the bus_formats array when > > retrieving this info from EDID. If I'm correct we have at most 18 bus > > formats: > > - 3 color formats: > > * RGB 4:4:4 > > * YCbCr 4:4:4 > > * YCbCr 4:4:2 > > - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > bpc isn't a bitmask, so EDID supports up to three formats only. > > The color_formats field is computed in the drm_add_display_info() function. > You could easily turn it into a local variable and allocate and fill the > formats array at the end of the function. But you also need to be careful to keep whatever formats the driver might have set explicitly. Thierry
On Mon, 21 Jul 2014 15:47:52 +0200 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Boris, > > On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > >> > > >> [snip] > > >> > > >>>>>>> The new drm_display_info structure should look like this [2] > > >>>>>>> (except that color_formats and bpc have not be removed yet), and > > >>>>>>> [1] is just here to show how the video_bus_format enum would look > > >>>>>>> like. > > >>>>>>> > > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557 > > >>>>>>> [2] http://code.bulix.org/7n03b4-86556 > > >>>>>> > > >>>>>> Quoting from your paste: > > >>>>>> + const enum video_bus_format *bus_formats; > > >>>>>> + int nbus_formats; > > >>>>>> > > >>>>>> Do we really need more than one? > > >>>>> > > >>>>> We do if we want to replace the color_formats and bpc fields. > > >>>> > > >>>> Yes, that's what I was about to answer :-). > > >>> > > >>> Maybe we don't need to replace color_formats and bpc field > > >>> immediately. That could be done in a follow-up patch. > > >> > > >> We don't need to replace them right now, but we should at least agree on > > >> how to replace them. Introducing a new field that would need to be > > >> replaced in the near future when removing color_formats and bpc would > > >> be a waste of time. > > > > > > Sure. One of the problems I see with replacing color_formats and bpc > > > with the above is that some of the bits within color_formats are set > > > when the EDID is parsed. That implies that if they are replaced with > > > an array of formats, the array would need to be reallocated during EDID > > > parsing. That sounds like ugliness. > > > > > > But if you can find a nice way to make it work that'd be great. > > > > How about using a list instead of an array ? > > This way we can add elements to this list when parsing the EDID. > > > > Or we can just define a maximum size for the bus_formats array when > > retrieving this info from EDID. If I'm correct we have at most 18 bus > > formats: > > - 3 color formats: > > * RGB 4:4:4 > > * YCbCr 4:4:4 > > * YCbCr 4:4:2 > > - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > bpc isn't a bitmask, so EDID supports up to three formats only. Yes, bpc only contains a single value for now, and it fits the DRM_EDID_DIGITAL_DEPTH field [1] (an enum defining the supported pixel depth). ITOH, DRM_EDID_HDMI_DC_XX [2] (which are referenced in the new drm_assign_hdmi_deep_color_info function) are just bitmasks and thus a display might support several color depth. As a result, I wonder if we shouldn't start supporting several color depths (as we do for color formats). [1]http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_edid.c#L3436 [2]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/drm_edid.c?id=refs/tags/v3.16-rc6#n3440
On Mon, 21 Jul 2014 15:54:12 +0200 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote: > > Hi Boris, > > > > On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > > > On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > > > > On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > > > >> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > > >>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > > >>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > > >>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > > >>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > > > >> > > > >> [snip] > > > >> > > > >>>>>>> The new drm_display_info structure should look like this [2] > > > >>>>>>> (except that color_formats and bpc have not be removed yet), and > > > >>>>>>> [1] is just here to show how the video_bus_format enum would look > > > >>>>>>> like. > > > >>>>>>> > > > >>>>>>> [1] http://code.bulix.org/rfd0yx-86557 > > > >>>>>>> [2] http://code.bulix.org/7n03b4-86556 > > > >>>>>> > > > >>>>>> Quoting from your paste: > > > >>>>>> + const enum video_bus_format *bus_formats; > > > >>>>>> + int nbus_formats; > > > >>>>>> > > > >>>>>> Do we really need more than one? > > > >>>>> > > > >>>>> We do if we want to replace the color_formats and bpc fields. > > > >>>> > > > >>>> Yes, that's what I was about to answer :-). > > > >>> > > > >>> Maybe we don't need to replace color_formats and bpc field > > > >>> immediately. That could be done in a follow-up patch. > > > >> > > > >> We don't need to replace them right now, but we should at least agree on > > > >> how to replace them. Introducing a new field that would need to be > > > >> replaced in the near future when removing color_formats and bpc would > > > >> be a waste of time. > > > > > > > > Sure. One of the problems I see with replacing color_formats and bpc > > > > with the above is that some of the bits within color_formats are set > > > > when the EDID is parsed. That implies that if they are replaced with > > > > an array of formats, the array would need to be reallocated during EDID > > > > parsing. That sounds like ugliness. > > > > > > > > But if you can find a nice way to make it work that'd be great. > > > > > > How about using a list instead of an array ? > > > This way we can add elements to this list when parsing the EDID. > > > > > > Or we can just define a maximum size for the bus_formats array when > > > retrieving this info from EDID. If I'm correct we have at most 18 bus > > > formats: > > > - 3 color formats: > > > * RGB 4:4:4 > > > * YCbCr 4:4:4 > > > * YCbCr 4:4:2 > > > - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > > > bpc isn't a bitmask, so EDID supports up to three formats only. > > > > The color_formats field is computed in the drm_add_display_info() function. > > You could easily turn it into a local variable and allocate and fill the > > formats array at the end of the function. > > But you also need to be careful to keep whatever formats the driver > might have set explicitly. Okay, in this case, using a list is a better idea, don't you think ?
On Mon, Jul 21, 2014 at 03:43:13PM +0200, Boris BREZILLON wrote: > How about using a list instead of an array ? > This way we can add elements to this list when parsing the EDID. > > Or we can just define a maximum size for the bus_formats array when > retrieving this info from EDID. If I'm correct we have at most 18 bus > formats: > - 3 color formats: > * RGB 4:4:4 > * YCbCr 4:4:4 > * YCbCr 4:4:2 > - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color This starts to worry me. What are we trying to do here - are we trying to encode the connection between the CRTC and the encoder, the encoder and the connector, or the connector and the device? The encoder to connector and connector to device is mostly a function of the interface spec itself (for example, many HDMI encoders take either a RGB or YUV input and can convert it to the HDMI specified colourspaces for transmission over the connector.) If you want to do encoder to connector, what about VGA or some other analogue signalling such as TV composite? It's easy to take this too far... Surely the only one which matters is the CRTC to the encoder - that's certainly true of all the setups I've come across so far. As for that interface, CRTCs I've seen can produce a /wide/ range of different representations. Some CRTCs (eg, AMBA CLCD) produce R, G, B signals scrunched down on to the LSB bits of a LCD data bus (so RGB888 uses 24 bits, RGB444 would use the LSB 12 bits of those 24 - rather than outputting the R4 bits on a subset of the R8 bits.) What about RGB565 - where you have differing number of bits for the green channel from red/blue? Then you have red/blue colour swapping at the CRTC (and similar for YUV) such as on Dove / Armada. Then there are some encoders have the ability to almost arbitarily map their input pins according to whatever you choose (eg, TDA998x). This problem isn't as quite as simple as "this is what EDID gives us" and "these are the number of bits representing a colour".
Hi Boris and Thierry, On Monday 21 July 2014 16:21:36 Boris BREZILLON wrote: > On Mon, 21 Jul 2014 15:54:12 +0200 Thierry Reding wrote: > > On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote: > >> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > >>> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > >>>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > >>>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > >>>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > >>>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > >>>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: [snip] > >>>>>>>>> Quoting from your paste: > >>>>>>>>> + const enum video_bus_format *bus_formats; > >>>>>>>>> + int nbus_formats; > >>>>>>>>> > >>>>>>>>> Do we really need more than one? > >>>>>>>> > >>>>>>>> We do if we want to replace the color_formats and bpc fields. > >>>>>>> > >>>>>>> Yes, that's what I was about to answer :-). > >>>>>> > >>>>>> Maybe we don't need to replace color_formats and bpc field > >>>>>> immediately. That could be done in a follow-up patch. > >>>>> > >>>>> We don't need to replace them right now, but we should at least > >>>>> agree on how to replace them. Introducing a new field that would > >>>>> need to be replaced in the near future when removing color_formats > >>>>> and bpc would be a waste of time. > >>>> > >>>> Sure. One of the problems I see with replacing color_formats and bpc > >>>> with the above is that some of the bits within color_formats are set > >>>> when the EDID is parsed. That implies that if they are replaced with > >>>> an array of formats, the array would need to be reallocated during > >>>> EDID parsing. That sounds like ugliness. > >>>> > >>>> But if you can find a nice way to make it work that'd be great. > >>> > >>> How about using a list instead of an array ? > >>> This way we can add elements to this list when parsing the EDID. > >>> > >>> Or we can just define a maximum size for the bus_formats array when > >>> retrieving this info from EDID. If I'm correct we have at most 18 bus > >>> > >>> formats: > >>> - 3 color formats: > >>> * RGB 4:4:4 > >>> * YCbCr 4:4:4 > >>> * YCbCr 4:4:2 > >>> - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > >> > >> bpc isn't a bitmask, so EDID supports up to three formats only. > >> > >> The color_formats field is computed in the drm_add_display_info() > >> function. You could easily turn it into a local variable and allocate > >> and fill the formats array at the end of the function. > > > > But you also need to be careful to keep whatever formats the driver might > > have set explicitly. Do we have drivers that explicitly add formats to the formats parsed from EDID data ? If so, what's the use case ? > Okay, in this case, using a list is a better idea, don't you think ? I'd prefer an array if possible, as that would be easier to use for drivers. In any case, we need to define who allocates and frees the array or the list elements, how and when.
Hi Boris, On Monday 21 July 2014 16:18:10 Boris BREZILLON wrote: > On Mon, 21 Jul 2014 15:47:52 +0200 Laurent Pinchart wrote: > > On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > >> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > >>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > >>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > >>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > >>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > >>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > >>>>>>>> On Mon, Jul 21, 2014 at 11:57:37AM +0200, Boris BREZILLON wrote: > >>>> [snip] > >>>> > >>>>>>>>> The new drm_display_info structure should look like this [2] > >>>>>>>>> (except that color_formats and bpc have not be removed yet), and > >>>>>>>>> [1] is just here to show how the video_bus_format enum would > >>>>>>>>> look like. > >>>>>>>>> > >>>>>>>>> [1] http://code.bulix.org/rfd0yx-86557 > >>>>>>>>> [2] http://code.bulix.org/7n03b4-86556 > >>>>>>>> > >>>>>>>> Quoting from your paste: > >>>>>>>> + const enum video_bus_format *bus_formats; > >>>>>>>> + int nbus_formats; > >>>>>>>> > >>>>>>>> Do we really need more than one? > >>>>>>> > >>>>>>> We do if we want to replace the color_formats and bpc fields. > >>>>>> > >>>>>> Yes, that's what I was about to answer :-). > >>>>> > >>>>> Maybe we don't need to replace color_formats and bpc field > >>>>> immediately. That could be done in a follow-up patch. > >>>> > >>>> We don't need to replace them right now, but we should at least agree > >>>> on how to replace them. Introducing a new field that would need to be > >>>> replaced in the near future when removing color_formats and bpc would > >>>> be a waste of time. > >>> > >>> Sure. One of the problems I see with replacing color_formats and bpc > >>> with the above is that some of the bits within color_formats are set > >>> when the EDID is parsed. That implies that if they are replaced with > >>> an array of formats, the array would need to be reallocated during > >>> EDID parsing. That sounds like ugliness. > >>> > >>> But if you can find a nice way to make it work that'd be great. > >> > >> How about using a list instead of an array ? > >> This way we can add elements to this list when parsing the EDID. > >> > >> Or we can just define a maximum size for the bus_formats array when > >> retrieving this info from EDID. If I'm correct we have at most 18 bus > >> > >> formats: > >> - 3 color formats: > >> * RGB 4:4:4 > >> * YCbCr 4:4:4 > >> * YCbCr 4:4:2 > >> > >> - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > > > bpc isn't a bitmask, so EDID supports up to three formats only. > > Yes, bpc only contains a single value for now, and it fits the > DRM_EDID_DIGITAL_DEPTH field [1] (an enum defining the supported pixel > depth). > ITOH, DRM_EDID_HDMI_DC_XX [2] (which are referenced in the new > drm_assign_hdmi_deep_color_info function) are just bitmasks and thus a > display might support several color depth. > > As a result, I wonder if we shouldn't start supporting several color depths > (as we do for color formats) If there's a use case for that, sure. It wouldn't be difficult, given that a bus format defines both the color format and the depths. > [1]http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_edid.c#L3436 > [2]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/driv > ers/gpu/drm/drm_edid.c?id=refs/tags/v3.16-rc6#n3440
On Mon, Jul 21, 2014 at 08:30:31PM +0200, Laurent Pinchart wrote: > Hi Boris and Thierry, > > On Monday 21 July 2014 16:21:36 Boris BREZILLON wrote: > > On Mon, 21 Jul 2014 15:54:12 +0200 Thierry Reding wrote: > > > On Mon, Jul 21, 2014 at 03:47:52PM +0200, Laurent Pinchart wrote: > > >> On Monday 21 July 2014 15:43:13 Boris BREZILLON wrote: > > >>> On Mon, 21 Jul 2014 15:30:35 +0200 Thierry Reding wrote: > > >>>> On Mon, Jul 21, 2014 at 03:22:05PM +0200, Laurent Pinchart wrote: > > >>>>> On Monday 21 July 2014 14:55:23 Thierry Reding wrote: > > >>>>>> On Mon, Jul 21, 2014 at 02:34:28PM +0200, Boris BREZILLON wrote: > > >>>>>>> On Mon, 21 Jul 2014 14:16:42 +0200 Laurent Pinchart wrote: > > >>>>>>>> On Monday 21 July 2014 14:12:47 Thierry Reding wrote: > > [snip] > > > >>>>>>>>> Quoting from your paste: > > >>>>>>>>> + const enum video_bus_format *bus_formats; > > >>>>>>>>> + int nbus_formats; > > >>>>>>>>> > > >>>>>>>>> Do we really need more than one? > > >>>>>>>> > > >>>>>>>> We do if we want to replace the color_formats and bpc fields. > > >>>>>>> > > >>>>>>> Yes, that's what I was about to answer :-). > > >>>>>> > > >>>>>> Maybe we don't need to replace color_formats and bpc field > > >>>>>> immediately. That could be done in a follow-up patch. > > >>>>> > > >>>>> We don't need to replace them right now, but we should at least > > >>>>> agree on how to replace them. Introducing a new field that would > > >>>>> need to be replaced in the near future when removing color_formats > > >>>>> and bpc would be a waste of time. > > >>>> > > >>>> Sure. One of the problems I see with replacing color_formats and bpc > > >>>> with the above is that some of the bits within color_formats are set > > >>>> when the EDID is parsed. That implies that if they are replaced with > > >>>> an array of formats, the array would need to be reallocated during > > >>>> EDID parsing. That sounds like ugliness. > > >>>> > > >>>> But if you can find a nice way to make it work that'd be great. > > >>> > > >>> How about using a list instead of an array ? > > >>> This way we can add elements to this list when parsing the EDID. > > >>> > > >>> Or we can just define a maximum size for the bus_formats array when > > >>> retrieving this info from EDID. If I'm correct we have at most 18 bus > > >>> > > >>> formats: > > >>> - 3 color formats: > > >>> * RGB 4:4:4 > > >>> * YCbCr 4:4:4 > > >>> * YCbCr 4:4:2 > > >>> - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > >> > > >> bpc isn't a bitmask, so EDID supports up to three formats only. > > >> > > >> The color_formats field is computed in the drm_add_display_info() > > >> function. You could easily turn it into a local variable and allocate > > >> and fill the formats array at the end of the function. > > > > > > But you also need to be careful to keep whatever formats the driver might > > > have set explicitly. > > Do we have drivers that explicitly add formats to the formats parsed from EDID > data ? If so, what's the use case ? Drivers could specifically add them if there's no EDID or if the EDID is known to be broken. If the former this is probably irrelevant. In the latter maybe a better option would be to ignore the EDID-probed ones rather than use the union of those provided by the driver and EDID. Thierry
On Mon, Jul 21, 2014 at 06:06:30PM +0100, Russell King - ARM Linux wrote: > On Mon, Jul 21, 2014 at 03:43:13PM +0200, Boris BREZILLON wrote: > > How about using a list instead of an array ? > > This way we can add elements to this list when parsing the EDID. > > > > Or we can just define a maximum size for the bus_formats array when > > retrieving this info from EDID. If I'm correct we have at most 18 bus > > formats: > > - 3 color formats: > > * RGB 4:4:4 > > * YCbCr 4:4:4 > > * YCbCr 4:4:2 > > - 6 color depths: 6, 8, 10, 12, 14 and 16 bits per color > > This starts to worry me. What are we trying to do here - are we trying > to encode the connection between the CRTC and the encoder, the encoder > and the connector, or the connector and the device? This is about the bus format of the panel device. That would make it the latter. > The encoder to connector and connector to device is mostly a function of > the interface spec itself (for example, many HDMI encoders take either a > RGB or YUV input and can convert it to the HDMI specified colourspaces for > transmission over the connector.) The discussion here started because we currently have no way to store information about the interface for raw RGB. That means currently all drivers need to hardcode assumptions about the mode. The idea was to make this information available via a field in drm_display_info so that drivers could reconfigure depending on what the attached panel expects. This doesn't only apply to panels, though, the issue is the same when a bridge (RGB/LVDS for example) is connected to the encoder. > If you want to do encoder to connector, what about VGA or some other > analogue signalling such as TV composite? It's easy to take this too > far... > > Surely the only one which matters is the CRTC to the encoder - that's > certainly true of all the setups I've come across so far. As for that > interface, CRTCs I've seen can produce a /wide/ range of different > representations. > > Some CRTCs (eg, AMBA CLCD) produce R, G, B signals scrunched down on to > the LSB bits of a LCD data bus (so RGB888 uses 24 bits, RGB444 would > use the LSB 12 bits of those 24 - rather than outputting the R4 bits on > a subset of the R8 bits.) > > What about RGB565 - where you have differing number of bits for the > green channel from red/blue? > > Then you have red/blue colour swapping at the CRTC (and similar for YUV) > such as on Dove / Armada. > > Then there are some encoders have the ability to almost arbitarily map > their input pins according to whatever you choose (eg, TDA998x). > > This problem isn't as quite as simple as "this is what EDID gives us" > and "these are the number of bits representing a colour". I think what we need is a way to pass information from whatever device is behind the encoder (be it a bridge or a panel) to the encoder. And likely we'll need a similar (or the same) way to pass that information from bridge to bridge or from bridge to panel. That way the encoder can ask the bridge (or panel) about the format it requires and reconfigure itself accordingly. This should be able to work with an arbitrary bridge -> [bridge... ->] panel chain where each element in the chain can reconfigure depending on what the next element requires (or fail if it can't work, which should really never happen). Thierry
diff --git a/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt new file mode 100644 index 0000000..594bdb2 --- /dev/null +++ b/Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt @@ -0,0 +1,59 @@ +Device-Tree bindings for Atmel's HLCDC (High LCD Controller) DRM driver + +The Atmel HLCDC Display Controller is subdevice of the HLCDC MFD device. +See Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt for more details. + +Required properties: + - compatible: value should be one of the following: + "atmel,hlcdc-dc" + - interrupts: the HLCDC interrupt definition + - pinctrl-names: the pin control state names. Should contain "default", + "rgb-444", "rgb-565", "rgb-666" and "rgb-888". + - pinctrl-[0-4]: should contain the pinctrl states described by pinctrl + names. + - atmel,panel: Should contain a phandle with 2 parameters. + The first cell is a phandle to a DRM panel device + The second cell encodes the RGB mode, which can take the following values: + * 0: RGB444 + * 1: RGB565 + * 2: RGB666 + * 3: RGB888 + The third cell encodes specific flags describing LCD signals configuration + (see Atmel's datasheet for a full description of these fields): + * bit 0: HSPOL: Horizontal Synchronization Pulse Polarity + * bit 1: VSPOL: Vertical Synchronization Pulse Polarity + * bit 2: VSPDLYS: Vertical Synchronization Pulse Start + * bit 3: VSPDLYE: Vertical Synchronization Pulse End + * bit 4: DISPPOL: Display Signal Polarity + * bit 7: DISPDLY: LCD Controller Display Power Signal Synchronization + * bit 12: VSPSU: LCD Controller Vertical synchronization Pulse Setup Configuration + * bit 13: VSPHO: LCD Controller Vertical synchronization Pulse Hold Configuration + * bit 16-20: GUARDTIME: LCD DISPLAY Guard Time + +Example: + + hlcdc: hlcdc@f0030000 { + compatible = "atmel,sama5d3-hlcdc"; + reg = <0xf0030000 0x2000>; + clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>; + clock-names = "periph_clk","sys_clk", "slow_clk"; + status = "disabled"; + + hlcdc-display-controller { + compatible = "atmel,hlcdc-dc"; + interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>; + pinctrl-names = "default", "rgb-444", "rgb-565", "rgb-666", "rgb-888"; + pinctrl-0 = <&pinctrl_lcd_base>; + pinctrl-1 = <&pinctrl_lcd_base &pinctrl_lcd_rgb444>; + pinctrl-2 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>; + pinctrl-3 = <&pinctrl_lcd_base &pinctrl_lcd_rgb666>; + pinctrl-4 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>; + }; + + hlcdc_pwm: hlcdc-pwm { + compatible = "atmel,hlcdc-pwm"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_lcd_pwm>; + #pwm-cells = <3>; + }; + };
The Atmel HLCDC (HLCD Controller) IP available on some Atmel SoCs (i.e. at91sam9n12, at91sam9x5 family or sama5d3 family) provides a display controller device. The HLCDC block provides a single RGB output port, and only supports LCD panels connection to LCD panels for now. The atmel,panel property link the HLCDC RGB output with the LCD panel connected on this port (note that the HLCDC RGB connector implementation makes use of the DRM panel framework). Connection to other external devices (DRM bridges) might be added later by mean of a new atmel,xxx (atmel,bridge) property. Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> --- .../devicetree/bindings/drm/atmel-hlcdc-dc.txt | 59 ++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 Documentation/devicetree/bindings/drm/atmel-hlcdc-dc.txt