| Message ID | 1378236372-15711-1-git-send-email-mikedunn@newsguy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@newsguy.com> > --- > > 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. 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? 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... =) 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. 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. Thierry
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@newsguy.com> >> --- >> >> 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-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/09/13 14:40, Mike Dunn wrote: > On 09/10/2013 10:21 AM, Thierry Reding wrote: >> 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 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. 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. Tomi
On Thu, Sep 26, 2013 at 01:13:18PM +0300, Tomi Valkeinen wrote: > On 11/09/13 14:40, Mike Dunn wrote: > > On 09/10/2013 10:21 AM, Thierry Reding wrote: > > >> 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. 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. > 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. But as I said above, the combination of an enable GPIO and power supply should be enough to get a lot of use-cases supported. > 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? Thierry
On 26/09/13 15:02, Thierry Reding wrote: > On Thu, Sep 26, 2013 at 01:13:18PM +0300, Tomi Valkeinen wrote: >> On 11/09/13 14:40, Mike Dunn wrote: >>> On 09/10/2013 10:21 AM, Thierry Reding wrote: >> >>>> 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? > 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. > >> 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. > But as I said above, the combination of an enable GPIO and power supply > should be enough to get a lot of use-cases supported. Yep. >> 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. Tomi
On Thu, Sep 26, 2013 at 03:26:13PM +0300, Tomi Valkeinen wrote: > On 26/09/13 15:02, Thierry Reding wrote: > > On Thu, Sep 26, 2013 at 01:13:18PM +0300, Tomi Valkeinen wrote: > >> On 11/09/13 14:40, Mike Dunn wrote: > >>> On 09/10/2013 10:21 AM, Thierry Reding wrote: > >> > >>>> 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). > > 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. > > > >> 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. > >> 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
On 26/09/13 15:50, Thierry Reding wrote: >> 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. Board hooks? Those cannot be used with DT boot. But yes, the power sequences without DT (or platform data) parts cannot handle with board-specific-things. I still think it'd be a very nice thing to have inside the drivers. A single driver could more easily handle bunch of somewhat similar devices, by specifying power sequences in a table, one sequence for each device. > 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. Sure, no disagreement there. Tomi
On 09/26/2013 05:50 AM, Thierry Reding wrote: > On Thu, Sep 26, 2013 at 03:26:13PM +0300, Tomi Valkeinen wrote: >> On 26/09/13 15:02, Thierry Reding wrote: >>> On Thu, Sep 26, 2013 at 01:13:18PM +0300, Tomi Valkeinen wrote: >>>> On 11/09/13 14:40, Mike Dunn wrote: >>>>> On 09/10/2013 10:21 AM, Thierry Reding wrote: >>>> >>>>>> 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). Yes, this is currently my best guess. This is reverse-engineered and unfortunately I'm not yet able to accurately describe my particular use-case. Probably as wacky as anything you can imagine, Thierry :) The gpio and regulator patches will probably suffice. Thierry, can you please point me to those patches? I don't see them in your gitorious tree. If they were posted to linux-pwm, I missed them; sorry. Thanks, Mike -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 27, 2013 at 06:35:31AM -0700, Mike Dunn wrote: > On 09/26/2013 05:50 AM, Thierry Reding wrote: > > On Thu, Sep 26, 2013 at 03:26:13PM +0300, Tomi Valkeinen wrote: > >> On 26/09/13 15:02, Thierry Reding wrote: > >>> On Thu, Sep 26, 2013 at 01:13:18PM +0300, Tomi Valkeinen wrote: > >>>> On 11/09/13 14:40, Mike Dunn wrote: > >>>>> On 09/10/2013 10:21 AM, Thierry Reding wrote: > >>>> > >>>>>> 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). > > > Yes, this is currently my best guess. This is reverse-engineered and > unfortunately I'm not yet able to accurately describe my particular use-case. > Probably as wacky as anything you can imagine, Thierry :) > > The gpio and regulator patches will probably suffice. Thierry, can you please > point me to those patches? I don't see them in your gitorious tree. If they > were posted to linux-pwm, I missed them; sorry. I've stumbled across this email and it's not marked as answered, so here goes: these patches will be part of my pull request for 3.13. They should now be in my tree, although that's now moved to kernel.org. You can find it here: https://git.kernel.org/cgit/linux/kernel/git/thierry.reding/linux-pwm.git Thierry
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. [0]: Documentation/devicetree/bindings/pwm/pwm.txt @@ -25,4 +28,5 @@ Example: brightness-levels = <0 4 8 16 32 64 128 255>; default-brightness-level = <6>; + gpios = <&gpio 77 0>, <&gpio 25 1>; /* gpio 25 is active low */ }; diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index 1fea627..1e2ab52 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -20,6 +20,12 @@ #include <linux/pwm.h> #include <linux/pwm_backlight.h> #include <linux/slab.h> +#include <linux/of_gpio.h> + +struct pwm_bl_gpio { + unsigned int gpio; + enum of_gpio_flags flags; +}; struct pwm_bl_data { struct pwm_device *pwm; @@ -27,6 +33,8 @@ struct pwm_bl_data { unsigned int period; unsigned int lth_brightness; unsigned int *levels; + unsigned int num_gpios; + struct pwm_bl_gpio *gpios; int (*notify)(struct device *, int brightness); void (*notify_after)(struct device *, @@ -94,14 +102,77 @@ static const struct backlight_ops pwm_backlight_ops = { }; #ifdef CONFIG_OF +static int pwm_backlight_dt_notify(struct device *dev, int brightness) +{ + struct backlight_device *bl = dev_get_drvdata(dev); + struct pwm_bl_data *pb = bl_get_data(bl); + int i; + + if (brightness) { + for (i = 0; i < pb->num_gpios; i++) { + if (pb->gpios[i].flags == OF_GPIO_ACTIVE_LOW) + gpio_set_value(pb->gpios[i].gpio, 0); + else + gpio_set_value(pb->gpios[i].gpio, 1); + } + return 0; + } + + /* de-assert gpios in reverse order, in case this is important */ + for (i = pb->num_gpios - 1; i >= 0; i--) { + if (pb->gpios[i].flags == OF_GPIO_ACTIVE_LOW) + gpio_set_value(pb->gpios[i].gpio, 1); + else + gpio_set_value(pb->gpios[i].gpio, 0); + } + return 0; +} + +static void pwm_backlight_dt_exit(struct pwm_bl_data *pb) +{ + int i; + + for (i = 0; i < pb->num_gpios; i++) + gpio_free(pb->gpios[i].gpio); +} + +static int pwm_backlight_dt_init(struct device *dev, struct pwm_bl_data *pb) +{ + int i, j, ret; + + /* request gpios and drive in the inactive state */ + for (i = 0; i < pb->num_gpios; i++) { + char gpio_name[32]; + unsigned long flags; + if (pb->gpios[i].flags == OF_GPIO_ACTIVE_LOW) + flags = GPIOF_OUT_INIT_LOW; + else + flags = GPIOF_OUT_INIT_HIGH; + snprintf(gpio_name, 32, "%s.%d", dev_name(dev), i); + ret = gpio_request_one(pb->gpios[i].gpio, flags, gpio_name); + if (ret) { + dev_err(dev, "gpio #%d request failed\n", i); + goto gpio_err; + } + } + return 0; + + gpio_err: + for (j = 0; j < i; j++) + gpio_free(pb->gpios[j].gpio); + return ret; +} + static int pwm_backlight_parse_dt(struct device *dev, - struct platform_pwm_backlight_data *data) + struct platform_pwm_backlight_data *data, + struct pwm_bl_data *pb) { struct device_node *node = dev->of_node; struct property *prop; int length; u32 value; - int ret; + int ret, i, num_gpios; + size_t gpiosize; if (!node) return -ENODEV; @@ -138,13 +209,29 @@ static int pwm_backlight_parse_dt(struct device *dev, data->max_brightness--; } - /* - * TODO: Most users of this driver use a number of GPIOs to control - * backlight power. Support for specifying these needs to be - * added. - */ + /* read gpios from DT property */ + num_gpios = of_gpio_count(node); + if (num_gpios == -ENOENT) + return 0; /* no 'gpios' property present */ + if (num_gpios < 0) { + dev_err(dev, "invalid DT node: gpios\n"); + return -EINVAL; + } + gpiosize = sizeof(struct pwm_bl_gpio) * num_gpios; + pb->gpios = devm_kzalloc(dev, gpiosize, GFP_KERNEL); + if (!pb->gpios) + return -ENOMEM; + for (i = 0; i < num_gpios; i++) { + int gpio; + enum of_gpio_flags flags; + gpio = of_get_gpio_flags(node, i, &flags); + pb->gpios[i].gpio = (unsigned int)gpio; + pb->gpios[i].flags = flags; + } + pb->num_gpios = (unsigned int)num_gpios; + pb->notify = pwm_backlight_dt_notify; - return 0; + return pwm_backlight_dt_init(dev, pb); } static struct of_device_id pwm_backlight_of_match[] = { @@ -155,10 +242,12 @@ static struct of_device_id pwm_backlight_of_match[] = { MODULE_DEVICE_TABLE(of, pwm_backlight_of_match); #else static int pwm_backlight_parse_dt(struct device *dev, - struct platform_pwm_backlight_data *data) + struct platform_pwm_backlight_data *data, + struct pwm_bl_data *pb) { return -ENODEV; } +static void pwm_backlight_dt_exit(struct pwm_bl_data *pb) {} #endif static int pwm_backlight_probe(struct platform_device *pdev) @@ -171,8 +260,14 @@ static int pwm_backlight_probe(struct platform_device *pdev) unsigned int max; int ret; + pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL); + if (!pb) { + dev_err(&pdev->dev, "no memory for state\n"); + return -ENOMEM; + } + if (!data) { - ret = pwm_backlight_parse_dt(&pdev->dev, &defdata); + ret = pwm_backlight_parse_dt(&pdev->dev, &defdata, pb); if (ret < 0) { dev_err(&pdev->dev, "failed to find platform data\n"); return ret; @@ -187,20 +282,14 @@ static int pwm_backlight_probe(struct platform_device *pdev) return ret; } - pb = devm_kzalloc(&pdev->dev, sizeof(*pb), GFP_KERNEL); - if (!pb) { - dev_err(&pdev->dev, "no memory for state\n"); - ret = -ENOMEM; - goto err_alloc; - } - if (data->levels) { max = data->levels[data->max_brightness]; pb->levels = data->levels; } else max = data->max_brightness; - pb->notify = data->notify; + if (pb->notify == NULL) /* not using DT and its built-in notify() */ + pb->notify = data->notify; pb->notify_after = data->notify_after; pb->check_fb = data->check_fb; pb->exit = data->exit; @@ -250,9 +339,9 @@ static int pwm_backlight_probe(struct platform_device *pdev) } bl->props.brightness = data->dft_brightness; + platform_set_drvdata(pdev, bl); backlight_update_status(bl); - platform_set_drvdata(pdev, bl); return 0; err_alloc: @@ -271,6 +360,7 @@ static int pwm_backlight_remove(struct platform_device *pdev) pwm_disable(pb->pwm); if (pb->exit) pb->exit(&pdev->dev); + pwm_backlight_dt_exit(pb); return 0; }
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@newsguy.com> --- Thanks for looking! .../bindings/video/backlight/pwm-backlight.txt | 4 + drivers/video/backlight/pwm_bl.c | 128 ++++++++++++++++++--- 2 files changed, 113 insertions(+), 19 deletions(-)