Re: [PATCH 05/33] arm: omap: board-cm-t35: use generic dpi panel's gpio handling

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

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

On 2013-02-14 10:37, Igor Grinberg wrote:
> On 02/14/13 09:09, Tomi Valkeinen wrote:
>> On 2013-02-14 08:56, Igor Grinberg wrote:
>>> On 02/13/13 17:59, Tomi Valkeinen wrote:
> 
>>>> Okay, so I just realized there's an spi backlight driver used here, and
>>>> that backlight driver is actually handling the SPI transactions with the
>>>> panel, instead of the panel driver. So this looks quite messed up.
>>>
>>> Yep, it always was.
>>> The whole DSS specific panel handling inside the
>>> drivers/video/omap2/displays is a mess.
> 
>> Well, that's not mess itself, it's just omap specific panel framework.
>> But dividing single device handling into two separate places is a mess.
> 
> Yes, you are right it is not the mess, but it prevents the panel to
> be used on other systems and that is BAD.
> At the very least, drivers/video/backlight is a generic place that can be
> used not just on OMAP.

True, it's generic, but does it work reliably? The panel hardware is now
partly handled in the backlight driver, and partly in the omap's panel
driver (and wherever on other platforms).

At least currently there's a dependency between the two, as the LCD_EN
gpio is handled by the panel driver, which affects the functioning of
the backlight driver. Is it ensured that the panel driver does not
disable the panel when the backlight driver does spi transactions?

That's what I meant with the mess, it's difficult to make it work
reliably. I know that for some panels such a two-driver approach would
not work at all. Although I guess it's working well enough for you for
this panel.

Thinking about it, if you do move the gpio handling to the backlight
driver, the panel driver will only handle the DPI video stream. Then it
should not have any effect on the SPI side (presuming the panel doesn't
use the pixel clock as func clock), although there's probably still
possibility for display artifacts on enable and disable, if the order of
operations goes the wrong way.

> And since the toppoly was and is used on other systems, why the hell
> should anyone duplicate the driver just to please the OMAP specific
> panel framework? The real problem is that this framework should not be

Not to please. To make it reliable.

> OMAP specific...
> Of course I'm aware of the fact that currently there is no generic
> panel framework, but forging something OMAP specific which is obviously
> used on most of the other architectures/platforms (and I mean
> panel<->controller relations), is not a good way to go.

Well, if duplicating the code gives us reliable drivers, versus
unreliable without duplicating, then... I don't see it as that bad.

> Although, I'm also aware of the fact that most things are done this way:
> do several specific drivers/frameworks, find the common stuff, and extract
> it into a core driver/framework. So I don't want to blame anyone - that's
> just the way how we do things, right?

If it was easy, somebody would've done it.

>>>> For a quick solution, can we just set the LCD_EN at boot time (with the
>>>> msleep), and not touch it after that?
>>>
>>> That would be sensible for now, so this series can be merged.
>>> As a more appropriate (and long term) solution,
>>> I plan on moving the panel reset pin handling to the spi backlight
>>> driver itself.
> 
>> Well, if you must. But I suggest moving the whole panel handling into a
>> (omap specific) panel driver, as it's done for other panels. That way
>> you'll have a proper panel driver for it, for omap, and when CDF comes,
>> you'll get a platform independent panel driver for it.
> 
> You can't just move generic architecture/platform independent stuff
> into OMAP specific framework... Just think about this... It's insane.

As I said, reliable vs unreliable. That's not insane.

But again, if you can handle this particular panel reliably with the
two-driver approach, I'm fine with it.

> In addition, AFAIR, the reset pin is the property of the toppoly panel
> hardware, so that is why I think, we should let the toppoly driver
> (currently spi backlight, later hopefully CDF) handle it correctly
> along with the spi sequences.

Yes, that sounds ok.

 Tomi


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