Re: [PATCH 3/4] OMAPDSS: panel-sharp-ls037v7dw01: add device tree support

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5571 bytes --]

On 19/05/14 18:57, Tony Lindgren wrote:

>> I'm not sure if it's even possible to load both of those drivers at the
>> same time, but if it was, and the first one would get probe() called for
>> a device, and the driver would return an error, I'm quite sure the
>> driver framework won't continue trying with the other driver, as the
>> first driver already matched for the device.
> 
> No need to load multiple drivers at this point. That's why I'm saying
> you can bail out on the undesired display controller probes using
> of_machine_is_compatible test. It's not that uncommon for drivers to do:

Hmm, I don't follow. If both, say, omap and exynos have a panel driver
for the same sharp panel, both need a driver for it (presuming exynos
has a proper display component driver system, which may not be true).

Both drivers would be loaded at boot time for the probe to be even to be
possible. Even if the other one bails out from a device probe using
of_machine_is_compatible, afaik the driver framework will stop probing
at that point, as that driver reports being compatible with the device
in question.

...

Ah, ok, now I see. I think you mean module init, not probe? For some
reason I was just thinking about the probe stage.

Maybe that would work. I need to test tomorrow.

>>> it would be quite silly for each display controller to have to do
>>> their own remapping for shared panels?
>>
>> It would, but wouldn't it be just as silly (well, even more silly in my
>> opinion) to have each display controller require driver specific
>> compatible strings for the panels used?
> 
> Not driver specific, but hardware package specific. That's being done
> all the time. For example, compatible strings like "nvidia,tegra124",
> "ti,omap5" describe a package of an ARM core and various devices.

It feels to me rather wrong if I'd specify the compatible string for an
external piece of hardware based on the SoC it's connected to.

> But by using of_machine_is_compatible in the display drivers, you
> can use the right compatible flags from the start if you want to go
> that way.

Yes, with that we could have right compatible flags in the .dts.

>> Well, I sure hope nobody else has to implement similar thing.
> 
> Suuuure.. I've heard that about 20 times before and guess what
> happenened? Things never got fixed until some poor maintainer
> had to start fixing it all over the place about three years later.

The poor maintainer here being me =). And it's still a case of
pick-your-poison. We need to hack around the issue, in some form or
another. So in any case the poor maintainer has to fix it at some point
in the future.

But all this is fixed when we have a common display framework. And
that's something that is really badly needed in some form or another,
the sooner the better.

So the "fix" here is not just some internal thing that could be left at
that and forgotten.

>> To open that up a bit, traditionally other display driver architectures
>> have not had drivers for each display "entity", like the panels. So you
>> would really just have the display subsystem in the .dts, and some raw
>> data there (panel timings) to get the panel running.
> 
> Right, I believe that is the way to go, no doubt about it.

No, that's exactly _not_ the way to go =). We need proper drivers for
display components, instead of trying to avoid that and embed panel data
into the display controller's data.

> Here's a list of things that bothers me:
> 
> 1. Searching through the dtb from a driver instead of relying on the
>    driver probe mechanism for the components in question. If the parsing
>    of the dtb needs to be done it should be done in a Linux generic way
>    from some framework instead.

The reason I haven't made the conversion code available for others to
use is that there are no other users at the moment. And I hope there
will be no other user until we get the common display framework. If
there is, we'll need to move the code to a generic place.

> 2. Having to do this all early before we even have any debug console
>    available. We are trying to move everything to happen later on to avoid
>    all the issues related to initializing things early.

It worked fine for me when doing it as subsys_init level. Why do you say
it'd be early, before debug console?

All the drivers with converted compatible strings are normal drivers, so
afaik things work fine if we just convert the strings before the module
init level.

> 3. Having to maintain a database in kernel of display mappings separately
>    in addition to the .dts files.

Yes, that's slightly annoying. Not really a big burden, though, as it's
quite rare to get a new encoder or panel driver.

> 4. Having this hack limited to device tree based booting while we are
>    trying to unify the functions for drivers to use to get their
>    resources.

I don't understand this one. With non-DT boot we don't have the issue at
all, there's no need for hacks.

> 5. Seeing the possibility of this spreading to other drivers.

Well, until we have a common display framework, one of the hacky options
we've discussed will spread to other drivers if they need to have their
own drivers for the same hardware devices.

Which is not nice, but not something we can escape. And using the
of_machine_is_compatible or having the compatible strings in .dts
appended with "dss" or such doesn't change that, it just changes which
hack may spread.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]