On Mon, Jul 15, 2019 at 05:28:53PM -0700, Vasily Khoruzhick wrote:

On Fri, Jul 12, 2019 at 1:15 PM Maxime Ripard [off-list ref] wrote:

quoted

On Wed, Jul 10, 2019 at 03:11:04PM -0700, Vasily Khoruzhick wrote:

quoted

On Wed, Jul 10, 2019 at 4:40 AM Maxime Ripard [off-list ref] wrote:

quoted

quoted

quoted

quoted

There's another issue: if we introduce edp-connector we'll have to
specify power up delays somewhere (in dts? or in platform driver?), so
edp-connector doesn't really solve the issue of multiple panels with
same motherboard.

And that's what that compatible is about :)

Sorry, I fail to see how it would be different from using existing
panels infrastructure and different panels compatibles. I think Rob's
idea was to introduce generic edp-connector.

Again, there's no such thing as a generic edp-connector. The spec
doesn't define anything related to the power sequence for example.

quoted

If we can't make it generic then let's use panel infrastructure.

Which uses a device specific compatible. Really, I'm not sure what
your objection and / or argument is here.

In addition, when that was brought up in the discussion, you rejected
it because it was inconvenient:
https://patchwork.freedesktop.org/patch/283012/?series=56163&rev=1#comment_535206

It is inconvenient, but I don't understand how having board-specific
connectors fixes it.

How it would not fix it?

I think I got your idea, but yet I think it's not the best solution.

Do I understand correctly that you're proposing to introduce
board-specific edp-connector driver that will be aware of worst case
power up delays and will control backlight and power?

Then why not to add another board-specific panel (e.g.
"pine64,pinebook-panel") to simple-panel.c that does the same?

That would be fine for me too. Thierry was against it though IIRC, and
I don't recall why exactly.

quoted

You'll have one connector, without the need to describe each and every
panel in the device tree and rely on the EDID instead, and you'll have
the option to power up the regulator you need.

I really don't understand what's the issue here, so let's take a step
back. What are is the issue , what are your requirements, and how
would you like that to be described ?

We have a device (Pinebook) that uses the same board with multiple edp
panels. So far there're pinebooks with 3 different panels: 11" with
768p panel, 11" with 1080p panel, 14" with 768p panel.

Currently there's no way to describe all pinebooks with a single dts.
There's a simple workaround though -- we can just specify a panel with
worst power up delays in dts and it'll work since anx6345 driver
ignores panel modes anyway and uses EDID.

Originally I proposed to extend simple-panel driver to support generic
edp-panel but it was rejected. I still believe that it's the best
solution assuming we can specify delays in dts, since panels list is
specific to particular device and it probably can't be reused, i.e.
there's no good reason to move it into C code.

Rob Herring proposed to introduce edp-connector. While I still believe
that it's not accurate description of hardware since it'll have to
have backlight node (backlight is actually panel property) I was OK
with this approach assuming we can store delays in dts.

Later it evolved into board-specific edp-connector.

I think you got that wrong. As far as I'm concerned, the plan was to
have two compatibles: the board-specific one, and the generic one.

Something like compatible = "pine64,pinebook-edp-connector",
"edp-connector"; or whatever.

So far I don't understand why everyone is trying to avoid introducing
edp-panel driver that can read delays from dts. Basically, I don't
understand what's the magic behind simple-panel.c and why new panels
should be added there rather than described in dts. [1] Doesn't
explain that.

So others might have different viewpoints here as well, but the major
downside I see in putting those kind of values in the device tree is
that at some point, someone will get it wrong, and chances are that
even for the same panel, everyone will use a slightly different set of
timings.

And once it's wrong, then it's a mess to fix. You have to track down
every DT using it, make sure it's corrected, and then every user will
have to change their DT in their system. Whereas if you have just a
compatible and those timings in the kernel, then the only thing
required is a kernel update, which should be a pretty standard
operation.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com