On Fri, Mar 8, 2019 at 5:05 PM Eric Anholt <eric@xxxxxxxxxx> wrote: > > Rob Herring <robh+dt@xxxxxxxxxx> writes: > > > On Fri, Mar 8, 2019 at 3:45 AM Thierry Reding <thierry.reding@xxxxxxxxx> wrote: > >> > >> On Thu, Mar 07, 2019 at 11:10:30AM +0100, Maxime Ripard wrote: > >> > The kernel has a device tree binding for panel-dpi which allows for the > >> > panel timings to be described in the device-tree, however it wasn't > >> > supported so far except in a (small) number of KMS drivers that had an > >> > ad-hoc solution for this (omapdrm for example). > >> > >> I'm growing really tired of having to repeat these discussions... > >> > >> As far as I can tell, this binding was never reviewed by device tree > >> maintainers, so I'm not sure whether there's concensus that this should > >> be proliferated. Adding Rob and the device tree mailing list for a wider > >> audience. > >> > >> > Just like we've seen with panel-lvds, and even though the current dogma is > >> > to set the timings within the driver, having them in the device tree > >> > provides a number of benefits. > >> > >> I don't think there was concensus on that. But Rob acked it, so I guess > >> he thought it was acceptable. > >> > >> Rob, can we use this thread as an opportunity to write down some of the > >> rules regarding this? We've discussed this numerous times in the past > >> but there still doesn't seem to be concensus. > >> > >> I know you're very tired of this as well, but perhaps we can bite the > >> bullet now and produce clear documentation and guidelines that we can > >> point people at (or put in an obvious location so that people find it > >> themselves) in the future, so that we don't have to keep having this > >> discussion. > >> > >> Summarizing what our latest discussion on this was: > >> > >> * Generic compatible strings should typically be used as a fallback > >> only. Exceptions could be made if there's a specific standard that > >> is sufficiently strict to not require any quirks, and hence avoid > >> the need for panel-specific compatible strings. > >> > >> * So in general device tree nodes for panels need to have a specific > >> compatible string that uniquely identifies that panel. This is in > >> line with existing practice for other devices and a good idea in > >> general so that we can implement quirks if necessary. > >> > >> * Panel nodes can optionally also list a generic compatible string, in > >> addition to the specific compatible string, that drivers could match > >> to support devices which are not specifically supported yet but that > >> may already work anyway. > >> > >> I'm not sure that's really practical. In the past we've seen that a > >> panel can work fine on one board but break on another because the > >> runtime execution timing is such that necessary delays in the power > >> sequence are noticeable on one but not another. I also suspect that > >> in some cases shortcuts were taken because things happened to work, > >> even if perhaps there was intermittent garbage on the screen because > >> the power sequence wasn't respected. > > > > If there's some problem with a panel working, then we just use the > > more specific compatible and deal with it in the driver. What's the > > issue here? > > > > A bigger issue is if later you need to tweak the timings, do you > > update the timings in DT or add timings to kernel? > > > >> * Mechanisms that probe information from a panel at runtime (such as > >> EDID) are to see preferential treatment. In other words, if a DDC > >> channel exists to a panel, the driver should parse video timings > >> from the EDID. > > > > That pretty well summarizes things. > > I love how EDID keeps getting brought up in discussions how how to > handle non-EDID devices. If anyone had EDIDs, we'd use them because > that's easy, but we don't. This is mainly in reference to recent discussions on eDP panels which do have EDID and even a standard connector including power (though I'd guess not required). So a generic-ish compatible makes sense in that context as it has some meaning behind it. > >> One thing that's not clear to me is whether or not we want to allow > >> video timings to be specified in DT. I used to think that we didn't, > >> because the video timings are implied by the specific compatible string > >> (which we already determined is mandatory anyway), but the panel-lvds > >> bindings suggest that from a device tree perspective this would be fine? > >> I also notice that the panel-lvds doesn't make any provisions for power > >> sequences. The same is true for panel-dpi. Are both LVDS and DPI panels > >> always guaranteed not to need any specific power sequences? > > > > As long as the specific compatible is there, I'd don't really care > > that much. I think we've found over time with the LVDS binding that > > the specific compatible ends up being needed. > > > > I'm fine with timings in DT, but I'm on the fence whether a > > 'panel-dpi' compatible is a good thing or not. At least with LVDS, > > that implies something about the interface. For DPI, there is no > > standard really (MIPI does define something, but following MIPI is > > pretty optional). There's lots of ways to wire up the data lines. It > > could be a continual addition of timing flags which I don't want to > > see. My controller has fine grained clock controls and I need to > > control the duty cycle or delay the pixel clock some number of ns, for > > example. I think most of that goes away with LVDS. > > I think now that we have 85 struct panel_desc in panel-simple.c, we can > just go look at what's actually needed to drive panels. Detailed power > timing requirements are infrequently needed, being specified for 23 of > those. One thing to consider is whether power timing was added later or in general if the panels' data changed at all. Having incomplete bindings that have to evolve is one part of what we try to avoid. And do any of the others (not 23) just not have power control at all (because firmware turns on power or the platform doesn't have power control yet)? > I keep hearing that more detailed timing is necessary, so I > looked at the other non-DSI, non-SPI panels in tree (since those should > always require a driver for their register writes) and found a total of: > > - panel-arm-versatile: has register writes to a syscon > - olinuxino: reads modes from an eeprom. > - seiko-43wvf1g: has two power rails to manage > - panel-lvds: is basically doing the thing people are asking panel-dpi > to do but for lvds. > > If DT is supposed to "describe the hardware" as people keep telling me, > then I think we have strong evidence of commonality between hardware > that could be described in data instead of code, using panel-simple's > data structures as the model. "describe the h/w" also means the h/w description doesn't evolve because the h/w itself doesn't evolve. That being said, I'm not against moving things to DT if that's the right answer. Sometimes we don't know and it is better to put data in the kernel until it is clear what should and shouldn't go in DT. Rob
