Re: [PATCH] pwm-backlight: add support for device tree gpio control

On Thu, Sep 26, 2013 at 03:26:13PM +0300, Tomi Valkeinen wrote:

On 26/09/13 15:02, Thierry Reding wrote:

quoted

On Thu, Sep 26, 2013 at 01:13:18PM +0300, Tomi Valkeinen wrote:

quoted

On 11/09/13 14:40, Mike Dunn wrote:

quoted

On 09/10/2013 10:21 AM, Thierry Reding wrote:

quoted

quoted

Do you have a real setup that actually needs multiple GPIOs? Usually
such a setup requires some kind of timing or other additional constraint
which can't be represented by this simple binding.

Looking at the Palm Treo code it seems like the reason why multiple
GPIOs are needed is because one is to enable the backlight, while the
other is in fact used to enable the LCD panel. 

There are actually four GPIOs involved!  (There is an embarrasingly horrible
hack in arch/arm/mach-pxa/palmtreo.c that handles the others.)  One is almost
certainly simply backlight power.  The other three are probably LCD related.

When you say "power", do you mean the gpio enables a regulator to feed
power to the backlight? If so, wouldn't that be a regulator, not gpio,
from the bl driver's point of view?

Generally speaking, this same problem appears in many places, but for
some reason especially in display. I'm a bit hesitant in adding "free
form" gpio/regulator support for drivers, as, as Thierry pointed out,
there are often timing requirements, or sometimes the gpios are
inverted, or sometimes the gpio is, in fact, a reset gpio, where you'll
assert the gpio for a short moment only.

I sent out another series a few days ago that somewhat obsoletes this
patch. What it does is basically add a single enable GPIO that can be
used to turn the backlight on and off. In a separate patch, support is
added for a power regulator. The combination of both should be able to
cover the majority of use-cases.

But Mike's case required 4 GPIOs? Or can that be reduced to one gpio and
one regulator?

Well, at least for the backlight it only seemed to involve a single
GPIO. The other three were probably related to LCD and therefore not
really suitable for a backlight driver. Traditionally it has been that
the backlight driver handled these things as well (via the callbacks
installed by board setup code). While really they should be handled by a
separate driver (for the LCD).

quoted

That series doesn't handle any timing requirements, but again, for the
majority of the setups supported by a power supply and enable GPIO the
timing doesn't matter.

quoted

I haven't seen new versions for the power sequences framework (without
DT support), I think it could help us here a bit by simplifying how we
present the sequences inside the driver. But we still need multiple
drivers or the same driver supporting multiple devices.

I'm not sure if power sequences will be very helpful here. There was an
attempt to get those merged, but the patches were NAKed in the end. I'm
not aware of any work currently being done on them.

I thought the NAK was for the DT parts, not for the sequences as such. I
don't remember anyone shooting down the idea of defining power sequences
inside a driver.

Yes, but the DT parts were the primary reason why they were written in
the first place. Without DT we can just use the existing hooks to do the
sequencing. There is not much to be gained from power sequences.

quoted

quoted

And I also think that the model where we have just one driver for, say,
backlight may not be enough. There may be multiple hardware components,
that used together will generate the backlight. And each component
requires specific management.

I'm not sure what other components you are talking about here. Can you
elaborate?

I don't have any specific case in mind, and maybe these are quite rare.
But there could be some kind of mix of muxes, regulators, gpios and
whatnot that need to be managed in certain way to make backlight (or
display) work.

I'm making this up, so I'm not sure if this is sensible, but consider a
board where there's a mux to select where a backlight gets its PWM
input, and the mux is controlled via i2c. And maybe insert some kind of
level shifter in between, enabled with a GPIO. And some third component,
as this hypothetical board is a development board, and hardware people
seem to love to make bizarre designs, that work in theory but the SW is
almost impossible to design...

So to enable the backlight, we might need to do multiple different
things, depending on which components this particular board has.
Especially for a mux controlled via i2c it would make sense to have a
separate driver. Having just a single backlight driver might not be enough.

Sometimes, or hopefully often, that kind of complexity can be hidden
behind common frameworks. For example, enabling a gpio could result in
the gpio driver enabling a regulator, sending i2c messages, or whatever.
But I don't think that's possible in all cases.

Anyway, this was really more of "thinking out loud" than any suggestion.

There is unfortunately always the next crazy setup that one can think
of. I personally prefer to support what we have (or at least the
majority of that) with something generic and tackle the more exotic
setups later on (or when they appear, as the case may be).

As things stand right now, there's no way to get the simplest panel
setup to work properly if you use DT. By adding both an enable GPIO and
a power supply regulator we can at least cover the sane use-cases with
some sane and pretty simple code.

Thierry