[PATCH] drm: panels: Add MAINTAINERS entry for LVS panel driver

Wed Apr 12 15:26:58 UTC 2017

On Tue, Apr 11, 2017 at 12:10:47PM -0500, Rob Herring wrote:
> On Fri, Apr 7, 2017 at 12:33 PM, Thierry Reding <treding at nvidia.com> wrote:
> > On Fri, Apr 07, 2017 at 05:44:15AM +1000, Dave Airlie wrote:
> >> On 5 April 2017 at 16:51, Laurent Pinchart
> >> <laurent.pinchart+renesas at ideasonboard.com> wrote:
> >> > As the DRM LVDS panel driver uses a different approach to DT bindings
> >> > compared to what Thierry Reding advocates, add a specific MAINTAINERS
> >> > entry to avoid bothering Thierry with requests related to that driver.
> 
> This is not a good split. Panels should not be diverging based on
> interface. That's not to say we can't have sub-classed bindings based
> on interfaces.
> 
> >> Could you document a bit more in the patch summary the finer points of
> >> panel/dt doctrine, as I haven't got as much knowledge as I'd like.
> >>
> >> Just I believe, Thierry believes.
> >
> > I'm somewhat surprised how we arrived at the current situation. A very
> > long time ago when we first discussed device tree bindings for panels, a
> > number of attempts were made to generically describe everything in
> > device tree. All of those attempts failed because you simply couldn't
> > describe all of the required properties in DT in a sane way.
> >
> > Eventually everyone involved agreed that we would have to stick with the
> > device-specific compatible, and in the best case we would be able to
> > support many panels with a fairly generic driver. I think we did pretty
> > well with the panel-simple driver. It started out very simple and then
> > got improved over time as necessary to deal with more panels. And for
> > cases where it wasn't suitable we simply added a custom driver. That's a
> > completely natural way to write drivers. We do the same thing in other
> > areas, nothing special here.
> 
> That is still the case here.
> 
> > Ever since the simple-panel binding was introduced, which is now about
> > 3 1/2 years ago, people have kept asking why we couldn't simply put all
> > data in DT and why kernel drivers had to be modified in order to add
> > support for a new panel. I kept repeating myself a number of times until
> > I finally wrote it all up[0], after which it was enough to point people
> > to it. Still not everyone was convinced, but the people that were there
> > when we made the decision all agreed that this was still the right thing
> > to do. So, despite the many complaints I stuck to what we had agreed on
> > because I am convinced that it is the right thing to do.
> 
> The big difference was folks wanted "simple-panel" compatible strings
> and nothing else. That remains wrong and is a constant discussion. I'd
> say at least 30% of my reviews contain "needs a more specific
> compatible string". Panels are not the only "simple" or "generic"
> hardware. :)

But that's really what panel-lvds is all about. While the binding
documentation says that it needs "panel-lvds" and a panel-specific
compatible string, the reality is that if we only use "panel-lvds"
there's no way to fully describe the panel, and I suspect that we
will be painting ourselves into a corner.

For example, the panel-lvds binding defines a couple of required and
optional properties. None of that includes any power supplies. These
power supplies are defined in panel-specific bindings, because they
really are panel-specific. Matching on panel-lvds doesn't give you
information about the regulators. The fact that the driver doesn't
handle power supplies at all and has a large TODO comment about it
doesn't give me much confidence that this is well thought out.

So if matching on "panel-lvds" doesn't give you enough information to do
anything useful with the panel, then I think it really shouldn't be
there at all in the first place. If you look back at the history of some
early simple-panel users, we used to include "simple-panel" in the list
of compatible strings as well. Then we realized that there's really no
use for it to be there because by itself it doesn't provide concrete
information about the device. The panel-specific property already
implies it. It's equivalent to having:

	compatible = "nvidia,tegra20-gpio", "gpio-controller";

"gpio-controller" is much too abstract to be useful in any way. And so
is "simple-panel" or "panel-lvds".

It's also totally confusing because, although I haven't checked in
detail, I'm fairly sure that a number of panels already supported by the
panel-simple driver are actually LVDS panels. But adding "panel-lvds" to
their compatible string list would be wrong and the panel-lvds driver
wouldn't know what to do about it.

Then there's the additional fact that since we can define so many
properties in DT, we can also make up a completely fake device. With the
current bindings its possible to add a panel with compatible
"mitsubishi,aa121td01" but use property values from a panel that's
actually compatible with "mitsubishi,aa104xd12".

As a side-note, there's a typo in the binding for the latter, it
requires the compatible string list to contain the compatible for the
former. The example has the correct one, though.

> Parameterizing everything is indeed a losing battle. That doesn't mean
> we can't parameterize some things in DT if they are completely
> standard. IMO, roughly anything that can be in EDID could be in DT. So
> I don't have a big problem with timings or physical size of the
> display in DT. After all, we can always just ignore it.

That's effectively saying that display timings, if we decide to add them
to device tree, should be optional. Because if we can just ignore them,
it means they have to be implied by the compatible string. So we would
require the display timing database within drivers anyway, and the only
real use-case for DT display timings would be to override the ones
provided in the driver. That's something I had already agreed to, only
we never actually implemented it because it has never been needed.

That's not, however, what the panel-lvds binding proposal is about. It
makes the display timings mandatory such that you can fully define the
panels in DT. It means that we have to trust the timings in DT because
they are the only data we have. Choosing to ignore them is no longer an
option because there's no way to initialize the panel without them. The
driver implementation matches on the generic "panel-lvds" compatible and
therefore can only ever rely on what's in the DT. There's no information
about the required power sequencing or anything.

Essentially it does everything that I've been arguing against for the
past 3.5 years.

> > Now we have arrived at a point where apparently that decision has been
> > revoked, and I don't understand what's changed. This puts me in a very
> > difficult position. All of a sudden it's okay to do what everyone has
> > been asking for the last three years, and I'm the jerk who told everyone
> > that it couldn't be done.
> >
> > Maybe the discussions that we had back at the time are now far enough in
> > the past that people have forgotten about the earlier failures. I still
> > don't see how this new panel-lvds would be any more successful in
> > solving the problems we failed to solve with simple-panel. The issues
> > are still fundamentally the same. Now if this was a generic driver that
> > dealt with a different subset of panels because they are different, that
> > would've been okay with me. What I don't understand is why this has to
> > deviate from the simple-panel binding in fundamental ways. Now we've got
> > two bindings and we make life miserable for people because they have to
> > choose between the two.
> >
> > Thierry
> >
> > [0]: https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
> 
> I appreciate your excellent write-up very much. I've directed people
> to it numerous times.

This confuses me. You say that you agree with that write-up, and at the
same time you've acked the panel-lvds binding. But they both contradict
each other.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20170412/5befb610/attachment-0001.sig>