[PATCH] of: Add simple panel device tree binding

[Thierry Reding]

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. 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.

Yet, even with the availability of specifications and standard USB
classes, USB is still one of the areas where doing this is most common:

	$ git grep -n 'USB_DEVICE_.*(' -- drivers/ | wc -l
	829

And then there's things like drivers/hid/usbhid/hid-quirks.c and other
such things.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20131206/73eccdca/attachment.pgp>