On Fri, 11 Jul 2014 14:00:25 +0200 Boris BREZILLON <boris.brezillon@xxxxxxxxxxxxxxxxxx> wrote: > On Fri, 11 Jul 2014 12:37:46 +0200 > Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> > > > > > --- > > > > > > > > > > .../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 > -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
