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

[Mike Dunn]

On 09/10/2013 10:21 AM, Thierry Reding wrote:
> On Tue, Sep 03, 2013 at 12:26:12PM -0700, Mike Dunn wrote:
>> This patch adds support for controlling an arbitrary number of gpios to the
>> pwm-backlight driver.  This was left as a TODO when initial device tree support
>> was added by Thierry a while back.  This functionality replaces the callbacks
>> that are passed in the platform data for non-DT cases.  Users can avail
>> themselves of this feature by adding a 'gpios' property to the 'backlight' node.
>> When the update_status() callback in backlight_ops runs, the gpios listed in the
>> property are asserted/deasserted if the specified brightness is non-zero/zero.
>>
>> Tested on a pxa270-based Palm Treo 680.
>>
>> Signed-off-by: Mike Dunn <mikedunn@xxxxxxxxxxx>
>> ---
>>
>> Thanks for looking!
>>
>>  .../bindings/video/backlight/pwm-backlight.txt     |   4 +
>>  drivers/video/backlight/pwm_bl.c                   | 128 ++++++++++++++++++---
>>  2 files changed, 113 insertions(+), 19 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> index 1e4fc72..4583e68 100644
>> --- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> +++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight.txt
>> @@ -14,6 +14,9 @@ Required properties:
>>  Optional properties:
>>    - pwm-names: a list of names for the PWM devices specified in the
>>                 "pwms" property (see PWM binding[0])
>> +  - gpios:     An arbitrary number of gpios that must be asserted when the
>> +               backlight is on, and de-asserted when off.  They will be asserted
>> +               in the order they appear, and de-asserted in reverse order.
> 
> 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.
Timing doesn't seem to be an issue, but the order is important.  This is
reverse-engineered without any schematics, and honestly, I'm still guessing with
regard to how the board is wired.  It probably is not appropriate to manage all
four gpios in the backlight driver, but at least one needs to be, so I thought
I'd go ahead and prepare a patch that adds gpio support.  From what you are
telling me, I guess my approach was too simplistic.


> I have been working on a
> patch series to provide simple panel support, specifically to allow the
> separation of backlight and panel power sequencing. Would such a method
> work for your use-case as well? 


Sounds like it would, from what I know.


> The work that I'm doing is somewhat DRM
> centric, and I don't think there's a DRM driver for PXA, but perhaps it
> would be a good occasion to look at converting the PXA display drivers
> to DRM... =)


OK, thanks.  I'll look into it.  I'm clueless about DRM...  All I can say at
this point is that I'm not using X windows, and there is not really any hardware
acceleration in the pxa's lcd controller.


> 
> That said, I've had a patch similar to yours in a local tree (in fact in
> the same branch as the panel work I've been doing) for quite some time,
> which allows a single "enable" GPIO to be specified. I haven't gotten
> around to sending it out yet, but I'll do that shortly. The patch is a
> little more involved because it exposes the GPIO via platform data as
> well and therefore has to update a number of board files, too. While
> going over the various board files I found only a single board which can
> actually make use of the new functionality (which I also converted in
> the patch). Any other cases couldn't be implemented by the simple change
> so I suspect they can't either using your proposed binding.


Really?  Are we talking about the callbacks in struct
platform_pwm_backlight_data?  I thought the majority of them just
requested/released a gpio in init()/exit(), and wiggled it in notify().  I must
be missing something.


> 
> I've been trying for a while to come up with a way to support more use-
> cases, and I keep coming back to the same solution. Since the DT binding
> for power sequences was shot down some time ago, we probably have to
> represent any kind of sequencing in C code. So instead of trying to fit
> everything into a single binding, I think a more maintainable solution
> would be to create separate drivers for the more complex use-cases. That
> could either be done by using separate compatible values within the
> pwm-backlight driver or via completely different drivers. In the latter
> case we should probably think about exporting some of the pwm-backlight
> functionality so that drivers can easily reuse some of the code.


I guess I'll wait for your patch and for everything else to shake out... I may
be able to help out if you want to delegate some things.  Thanks for the reviews
and the info.  Any other pointers appreciated.  BTW, I'm working on a simple
patch to the pwm-backlight driver that will fix my inverted pwm output problem.

Thanks again,
Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/