[PATCH] of: Add simple panel device tree binding

Fri Dec 6 06:54:28 PST 2013

On Fri, Dec 06, 2013 at 02:58:00PM +0100, Thierry Reding wrote:
> On Thu, Dec 05, 2013 at 12:45:17AM +0100, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Tuesday 26 November 2013 09:59:12 Thierry Reding wrote:
> > > On Tue, Nov 26, 2013 at 02:54:41AM +0100, Laurent Pinchart wrote:
> > > > On Friday 22 November 2013 19:41:54 Thierry Reding wrote:
> > > > > This binding specifies a set of common properties for display panels. It
> > > > > can be used as a basis by bindings for specific panels.
> > > > > 
> > > > > Bindings for three specific panels are provided to show how the simple
> > > > > panel binding can be used.
> > > > > 
> > > > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > > > ---
> > > > > This binding has already been discussed earlier. Both Laurent and Tomi
> > > > > seemed to be generally fine with this.
> > > > 
> > > > That's correct, I'm generally fine with your approach, but there's still
> > > > one point I'd like to see addressed.
> > > > 
> > > > As I mentioned previously, I would like to avoid adding zillions of
> > > > compatible properties to the driver, when we could use a single property
> > > > in the DT bindings that would specify the timings instead. This would
> > > > lower the amount of changes made to the simple panel driver, while
> > > > keeping DT simple enough (at least in my opinion).
> > > > 
> > > > Specifying the full timings in every DT source file would be pretty
> > > > verbose and could be error-prone. However, using a single property to
> > > > specify one of the standard display timings, as orinally proposed by Hans
> > > > de Goede (CC'ed) during a chat at the kernel summit would in my opinion be
> > > > a good middle-ground solution.
> > > > 
> > > > Would you consider adding such a property to the simple panel bindings,
> > > > and define a common compatible string ? Each panel should of course list
> > > > its own compatibility string in addition to the common compatible string
> > > > in case the need for extensions arises in the future.
> > > 
> > > My gripe with this is that it duplicates information. By definition a simple
> > > panel supports a limited number of display modes (typically only a single
> > > one), so once you know the compatible value (which needs to be there anyway)
> > > you can derive the mode from it. Adding a property that specifies the
> > > display mode is redundant.
> > 
> > But that could be said of pretty much every device :-) Let's take an example I 
> > came across today. The SPI DT bindings have a maximum clock speed property for 
> > every device. This is usually a property of the hardware, which could have 
> > been stored in the device drivers.
> 
> No it isn't. It's a board-specific property. It depends on all sorts of
> factors such as how long the connections are between SPI master and
> slave as well as what other electrical components happen to be in that
> path.
> 
> > I don't particularly like expressing detailed device information in DT when 
> > the information is static for a given device and easily stored in the drivers. 
> > However, in this case, I believe the overhead of adding one mode property to 
> > DT is worth it, as there are zillion panel models in the wild. Going back to 
> > the SPI analogy, let's consider the m25p80 SPI flash memory driver. It 
> > supports many models, each of them implemented by a large number of 
> > manufacturers. The driver stores information for each model (such as the flash 
> > size), but the maximum SPI clock frequency is specified in DT as we don't want 
> > to have a list of all device models for all manufacturers in the driver 
> > (granted, one of the reasons to specify the max frequency in DT is that it can 
> > also be limited by the board, not only by the chip, but I believe my point 
> > remains valid).
> 
> On the contrary, it completely invalidates your point.
> 
> And in fact m25p80 is another perfect example to prove my point. Having
> display modes encoded within the panel driver and selected depending on
> the compatible string isn't anything other than the m25p_ids table.
> 
> When you want to support a new type of device, you need to add a new
> entry into the m25p_ids table, just like you would add a new display
> mode to the panel driver.
> 
> > > Dave Airlie proposed something else, namely to keep a list of common
> > > modes within the driver and have each device reference that mode
> > > instead. I think that could work similarily well. It still requires the
> > > driver to be touched for each new panel, but the changes will be very
> > > small. They'll be more of the "add a new table entry" sort of change
> > > that we have in drivers like the 8250/16550-compatible serial. Most of
> > > that could probably be wrapped into a macro to make it concise.
> > 
> > That would be better, but not perfect :-) Given the very large number of panel
> > models I would like to have a simple panel driver that doesn't need to be
> > patched for every model. The odd cases can of course be handled in C, but the
> > common case would really benefit from being handled in DT.
> 
> I really don't think the number of panels supported in mainline will be
> overly big.

I for once have hardly seen a display twice in different projects.

> Even if, I will gladly take patches that add them to the
> driver. And it isn't even a lot of work, you really just need to copy
> this stuff from the datasheet. And the larger the number of panels the
> kernel supports, the easier it will get to add new support because you
> may either just be able to reuse the compatible string because somebody
> else already added support for it, or you may be able to reuse an
> existing mode because your panel happens to match that mode.
> 
> > Imagine how annoying it would be to update the USB mass storage driver
> > with the VID:PID of every new USB flash device.
> 
> The electrical interface of USB is well defined and it is further well
> specified what USB mass storage means. In addition, USB has very good
> support for probing capabilities and characteristics. The equivalent for
> panels would be something like EDID. If the panel provided EDID, then of
> course you don't need to specify the mode, neither in DT nor in the
> driver.

What makes EDID useful is that it contains the modes. We don't have to
maintain a database of all monitors available out there in the kernel.
Still EDID data contains a vendor/model string which makes it possible
to add quirks for a specific monitor should we have to.

So I really vote for putting the mode data into dt for the 98% case and
an exact vendor/model for the remaining quirks. This is simple anyway
since we already have a binding for display modelines. Supporting this
in the panel driver is trivial.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |