Backlight control support for the PXA fram buffer.
Signed-off-by: Rodolfo Giometti <[email protected]>
---
Each platform should define the backlight properties in its own setup
file in "linux/arch/arm/mach-pxa/" as follow:
static int pxafb_bl_get_brightness(struct backlight_device *bl_dev)
{
return read_the_backlight_brightness();
}
static int pxafb_bl_update_status(struct backlight_device *bl_dev)
{
int perc, ret;
if (bl_dev->props->power != FB_BLANK_UNBLANK ||
bl_dev->props->fb_blank != FB_BLANK_UNBLANK)
perc = 0;
else
perc = bl_dev->props->brightness;
write_the_backlight_brightness(perc);
return 0;
}
static struct backlight_properties wwpc1100_backlight_props = {
.get_brightness = pxafb_bl_get_brightness,
.update_status = pxafb_bl_update_status,
.max_brightness = 100,
};
and then seting up the fb info as follow:
wwpc1100_pxafb_info.modes = &special_modes;
wwpc1100_pxafb_info.bl_props = &wwpc1100_backlight_props;
set_pxa_fb_info(&wwpc1100_pxafb_info);
Hello Rodolfo,
Wednesday, February 21, 2007, 4:53:53 PM, you wrote:
> Backlight control support for the PXA fram buffer.
Here're some comments: backlight support is already confusing
matter, and your patch IMHO makes it even more confusing for PXAFB.
Before even start with details, let's first think what backlight has
to do with framebuffer? The facts that the physical object implementing
the former is usually put in close proximity to the latter, and they mostly
should be powered on synchronously, probably don't yet mean that
each and every FB driver should include good chunk of code to support
doing BL in its own special way.
On the other hand, there's already
drivers/video/backlight/backlight.c which provides generic BL support,
implemented using notifier callback for FB core. Moreover, there's
corgi_bl.c driver which, contrary to its name is a generic driver for
embedded/PDA device backlight. It essentially subclassses pretty
abstract backlight.c, and provides good implementation for most BL
implementation. What you really need to do to use it, is to supply
single machine-dependent method, set_bl_intensity(). That method is
usually <10 lines.
With this in mind, adhoc backlight handlers in pxafb and few other
drivers are artifacts of older times. And it's sad they are tried to
be resurrected instead of being removed.
> Signed-off-by: Rodolfo Giometti <[email protected]>
> ---
> Each platform should define the backlight properties in its own setup
> file in "linux/arch/arm/mach-pxa/" as follow:
> static int pxafb_bl_get_brightness(struct backlight_device *bl_dev)
> {
> return read_the_backlight_brightness();
> }
> static int pxafb_bl_update_status(struct backlight_device *bl_dev)
> {
> int perc, ret;
> if (bl_dev->props->power != FB_BLANK_UNBLANK ||
> bl_dev->props->fb_blank != FB_BLANK_UNBLANK)
> perc = 0;
> else
> perc = bl_dev->props->brightness;
> write_the_backlight_brightness(perc);
> return 0;
> }
> static struct backlight_properties wwpc1100_backlight_props = {
> .get_brightness = pxafb_bl_get_brightness,
> .update_status = pxafb_bl_update_status,
> .max_brightness = 100,
> };
>
> and then seting up the fb info as follow:
> wwpc1100_pxafb_info.modes = &special_modes;
> wwpc1100_pxafb_info.bl_props = &wwpc1100_backlight_props;
> set_pxa_fb_info(&wwpc1100_pxafb_info);
--
Best regards,
Paul mailto:[email protected]
On Wed, Feb 21, 2007 at 06:00:37PM +0200, Paul Sokolovsky wrote:
>
> On the other hand, there's already
> drivers/video/backlight/backlight.c which provides generic BL support,
> implemented using notifier callback for FB core. Moreover, there's
My patch _uses_ that support.
> corgi_bl.c driver which, contrary to its name is a generic driver for
> embedded/PDA device backlight. It essentially subclassses pretty
> abstract backlight.c, and provides good implementation for most BL
> implementation. What you really need to do to use it, is to supply
> single machine-dependent method, set_bl_intensity(). That method is
> usually <10 lines.
I see, but in this manner you need to make a complete file for each
board, while with my patch you can just put few lines into machine's
definition file (a struct and 2 functions).
> With this in mind, adhoc backlight handlers in pxafb and few other
> drivers are artifacts of older times. And it's sad they are tried to
> be resurrected instead of being removed.
IMHO, the actual backlight support is not so much, infact I'd like to
generalize it to support also backlighted keyboards (or input
devices). :)
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Hello Rodolfo,
Wednesday, February 21, 2007, 6:12:10 PM, you wrote:
> On Wed, Feb 21, 2007 at 06:00:37PM +0200, Paul Sokolovsky wrote:
>>
>> On the other hand, there's already
>> drivers/video/backlight/backlight.c which provides generic BL support,
>> implemented using notifier callback for FB core. Moreover, there's
> My patch _uses_ that support.
Sorry if I missed it in quick review, as I told, there's a bit much
code ;-I
>> corgi_bl.c driver which, contrary to its name is a generic driver for
>> embedded/PDA device backlight. It essentially subclassses pretty
>> abstract backlight.c, and provides good implementation for most BL
>> implementation. What you really need to do to use it, is to supply
>> single machine-dependent method, set_bl_intensity(). That method is
>> usually <10 lines.
> I see, but in this manner you need to make a complete file for each
> board, while with my patch you can just put few lines into machine's
> definition file (a struct and 2 functions).
Why? It's the same, except that it already exists, generic one (not
limited to pxafb), and requires 1 function (too bad that C doesn't
support lambda's):
==============
static void h4000_set_bl_intensity(int intensity)
{
/* LCD brightness is driven by PWM0.
* We'll set the pre-scaler to 8, and the period to 1024, this
* means the backlight refresh rate will be 3686400/(8*1024) =
* 450 Hz which is quite enough.
*/
PWM_CTRL0 = 7; /* pre-scaler */
PWM_PWDUTY0 = intensity; /* duty cycle */
PWM_PERVAL0 = H4000_MAX_INTENSITY; /* period */
if (intensity > 0) {
pxa_set_cken(CKEN0_PWM0, 1);
asic3_set_gpio_out_b(&h4000_asic3.dev,
GPIOB_BACKLIGHT_POWER_ON, GPIOB_BACKLIGHT_POWER_ON);
} else {
pxa_set_cken(CKEN0_PWM0, 0);
asic3_set_gpio_out_b(&h4000_asic3.dev,
GPIOB_BACKLIGHT_POWER_ON, 0);
}
}
static struct corgibl_machinfo h4000_bl_machinfo = {
.default_intensity = H4000_MAX_INTENSITY / 4,
.limit_mask = 0x0fff,
.max_intensity = H4000_MAX_INTENSITY,
.set_bl_intensity = h4000_set_bl_intensity,
};
struct platform_device h4000_bl = {
.name = "corgi-bl",
.dev = {
.platform_data = &h4000_bl_machinfo,
},
};
==============
>> With this in mind, adhoc backlight handlers in pxafb and few other
>> drivers are artifacts of older times. And it's sad they are tried to
>> be resurrected instead of being removed.
> IMHO, the actual backlight support is not so much, infact I'd like to
> generalize it to support also backlighted keyboards (or input
> devices). :)
I sent a bit of criticism for that too ;-). YMMV, but kernel
solutions are just bound to be pretty simple and generic and lack
any "niceties", which you'd likely want to do anyway eventually. For
example, what if you'll want to implement "fade out" effect for
keyboard backlight? Doing it in adhoc manner in kernel? Whereas with
the LCD classdev, you can write generic "fade out" trigger and
attach/detach it from userspace.
> Ciao,
> Rodolfo
--
Best regards,
Paul mailto:[email protected]
On Wed, 2007-02-21 at 15:53 +0100, Rodolfo Giometti wrote:
> Backlight control support for the PXA fram buffer.
>
> Signed-off-by: Rodolfo Giometti <[email protected]>
>
> ---
>
> Each platform should define the backlight properties in its own setup
> file in "linux/arch/arm/mach-pxa/" as follow:
>
> static int pxafb_bl_get_brightness(struct backlight_device *bl_dev)
> {
> return read_the_backlight_brightness();
> }
>
> static int pxafb_bl_update_status(struct backlight_device *bl_dev)
> {
> int perc, ret;
>
> if (bl_dev->props->power != FB_BLANK_UNBLANK ||
> bl_dev->props->fb_blank != FB_BLANK_UNBLANK)
> perc = 0;
> else
> perc = bl_dev->props->brightness;
>
> write_the_backlight_brightness(perc);
>
> return 0;
> }
>
> static struct backlight_properties wwpc1100_backlight_props = {
> .get_brightness = pxafb_bl_get_brightness,
> .update_status = pxafb_bl_update_status,
> .max_brightness = 100,
> };
>
> and then seting up the fb info as follow:
>
> wwpc1100_pxafb_info.modes = &special_modes;
> wwpc1100_pxafb_info.bl_props = &wwpc1100_backlight_props;
> set_pxa_fb_info(&wwpc1100_pxafb_info);
Reading through the patch its:
1) Not against any mainline kernel
2) Not against a recent kernel
There were a number of backlight class changes just merged into mainline
and you need to sync up any patch against them.
As mentioned by others, there is no need to tie the backlight driver
into the framebuffer any more. Have a look at
drivers/video/backlight/corgi_bl.c for an example (its used by PXA
devices).
I have said elsewhere I will take patches to make corgi_bl a more
generic driver (or maybe create a simple generic backlight driver) along
the lines of what Paul mentioned.
Regards,
Richard
On Thu, Feb 22, 2007 at 12:59:06AM +0000, Richard Purdie wrote:
>
> Reading through the patch its:
>
> 1) Not against any mainline kernel
> 2) Not against a recent kernel
I'm sorry, but the patch applay against the latest kernel. Please, try
it.
> There were a number of backlight class changes just merged into mainline
> and you need to sync up any patch against them.
My patch uses current backlight class support in the kernel.
> As mentioned by others, there is no need to tie the backlight driver
> into the framebuffer any more. Have a look at
> drivers/video/backlight/corgi_bl.c for an example (its used by PXA
> devices).
That driver uses the backlight class support as my patch does into
pxafb.
> I have said elsewhere I will take patches to make corgi_bl a more
> generic driver (or maybe create a simple generic backlight driver) along
> the lines of what Paul mentioned.
I see.
I suppose you are the backlight support mantainer, so what do you
suggest to do to "make corgi_bl a more generic driver"?
I have to rename and modify it? Or just copy it to have backward
compatibility and the modify the new file?
I should mv backlight directory from the video one?
Thanks for your suggestions,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Wed, Feb 21, 2007 at 06:26:08PM +0200, Paul Sokolovsky wrote:
k> Why? It's the same, except that it already exists, generic one (not
> limited to pxafb), and requires 1 function (too bad that C doesn't
> support lambda's):
Ah, ok.
> I sent a bit of criticism for that too ;-). YMMV, but kernel
> solutions are just bound to be pretty simple and generic and lack
> any "niceties", which you'd likely want to do anyway eventually. For
> example, what if you'll want to implement "fade out" effect for
> keyboard backlight? Doing it in adhoc manner in kernel? Whereas with
> the LCD classdev, you can write generic "fade out" trigger and
> attach/detach it from userspace.
I agree. I just wish to add a backlight support for my LCD and
minikeypad.
What do you suggest to me in order to accomplish such task?
Thanks for your suggestions,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Thu, 2007-02-22 at 09:28 +0100, Rodolfo Giometti wrote:
> On Thu, Feb 22, 2007 at 12:59:06AM +0000, Richard Purdie wrote:
> >
> > Reading through the patch its:
> >
> > 1) Not against any mainline kernel
Sorry, I'd missed a patch entering mainline,
> > 2) Not against a recent kernel
>
> I'm sorry, but the patch applay against the latest kernel. Please, try
> it.
but this still applies as there were a number of backlight changes
merged just before 2.6.21-rc1 and your driver will not work with
2.6.21-rc1.
> > As mentioned by others, there is no need to tie the backlight driver
> > into the framebuffer any more. Have a look at
> > drivers/video/backlight/corgi_bl.c for an example (its used by PXA
> > devices).
> z
> That driver uses the backlight class support as my patch does into
> pxafb.
Yes, my point is that you shouldn't need to touch pxafb if you use the
backlight class. I know pxafb has backlight hooks but they are probably
going to get removed at some point as they should no longer be needed.
> > I have said elsewhere I will take patches to make corgi_bl a more
> > generic driver (or maybe create a simple generic backlight driver) along
> > the lines of what Paul mentioned.
>
> I see.
>
> I suppose you are the backlight support mantainer, so what do you
> suggest to do to "make corgi_bl a more generic driver"?
What changes do you need to it to be able to use it as a generic driver?
The main issue is that the structure definition is in one of the
sharpsl.h files at the moment so most drivers can't get to it. Fix that
and it should make a good generic driver.
Ideally I'd prefer to leave the name alone since there is broken
userspace on the Zaurus that uses that name but I can see why people
want it renamed if its to be used as a generic driver...
Richard
On Thu, Feb 22, 2007 at 09:27:09AM +0000, Richard Purdie wrote:
>
> Yes, my point is that you shouldn't need to touch pxafb if you use the
> backlight class. I know pxafb has backlight hooks but they are probably
> going to get removed at some point as they should no longer be needed.
I see.
> What changes do you need to it to be able to use it as a generic driver?
>
> The main issue is that the structure definition is in one of the
> sharpsl.h files at the moment so most drivers can't get to it. Fix that
> and it should make a good generic driver.
I'll do as you suggest!
> Ideally I'd prefer to leave the name alone since there is broken
> userspace on the Zaurus that uses that name but I can see why people
> want it renamed if its to be used as a generic driver...
Yes. :)
I'll wait for the 2.6.21 to see what to do... thanks a lot,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
Hello Rodolfo,
Thursday, February 22, 2007, 10:32:04 AM, you wrote:
> On Wed, Feb 21, 2007 at 06:26:08PM +0200, Paul Sokolovsky wrote:
k>> Why? It's the same, except that it already exists, generic one (not
>> limited to pxafb), and requires 1 function (too bad that C doesn't
>> support lambda's):
> Ah, ok.
>> I sent a bit of criticism for that too ;-). YMMV, but kernel
>> solutions are just bound to be pretty simple and generic and lack
>> any "niceties", which you'd likely want to do anyway eventually. For
>> example, what if you'll want to implement "fade out" effect for
>> keyboard backlight? Doing it in adhoc manner in kernel? Whereas with
>> the LCD classdev, you can write generic "fade out" trigger and
>> attach/detach it from userspace.
> I agree. I just wish to add a backlight support for my LCD and
> minikeypad.
> What do you suggest to me in order to accomplish such task?
Well, I write exactly to share experience and work towards having
best practices for backlight, etc. control, reusable on wide range of
embedded/handheld devices.
We in handhelds.org codebase have attached patch* to make corgi_bl
more suitable for general use. This patch was submitted to Richard
(so, more votes needed ;-) ). Otherwise, snippet I pasted is from real
machine implementation, HP iPaq h4000.
As for keyboard backlight, another port we have, HTC Universal, has:
normal indicator LEDs, keyboard backlight, flashlight, ring vibra. All
of these are handled via Generic LED API:
http://handhelds.org/cgi-bin/cvsweb.cgi/linux/kernel26/arch/arm/mach-pxa/htcuniversal/htcuniversal_leds.c?rev=1.6&content-type=text/x-cvsweb-markup
To show that it is not an anomaly, but hopefully, a trend, here's
vibra driver for the upcoming OpenMoko phone:
http://wiki.openmoko.org/wiki/Neo1973_Hardware#Vibrator
So, if you have freedom to add keyboard backlight control to your
userspace, that would allow you to do many interesting things without
disturbing the kernel.
[*] Optimized for size, full one should patch <asm/arch/sharpsl.h> of
course.
> Thanks for your suggestions,
> Rodolfo
--
Best regards,
Paul mailto:[email protected]
On Thu, Feb 22, 2007 at 12:33:35PM +0200, Paul Sokolovsky wrote:
>
> We in handhelds.org codebase have attached patch* to make corgi_bl
> more suitable for general use. This patch was submitted to Richard
> (so, more votes needed ;-) ). Otherwise, snippet I pasted is from real
> machine implementation, HP iPaq h4000.
You have my vote! :)
So let me suggest this patch who allows multiple driver instances (I
use it for both LCD and keypad backlighti devices).
Hope it could be merged into main line (maybe without the "corgi"
prefix ;-).
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Thu, 2007-02-22 at 17:37 +0100, Rodolfo Giometti wrote:
> On Thu, Feb 22, 2007 at 12:33:35PM +0200, Paul Sokolovsky wrote:
> >
> > We in handhelds.org codebase have attached patch* to make corgi_bl
> > more suitable for general use. This patch was submitted to Richard
> > (so, more votes needed ;-) ). Otherwise, snippet I pasted is from real
> > machine implementation, HP iPaq h4000.
>
> You have my vote! :)
>
> So let me suggest this patch who allows multiple driver instances (I
> use it for both LCD and keypad backlighti devices).
>
> Hope it could be merged into main line (maybe without the "corgi"
> prefix ;-).
Doing something along these lines is on my todo list but not at the top
since its 2.6.22 material now. Something should appear in -mm within a
few weeks but I don't have the time just at the moment.
Richard
> On Thu, Feb 22, 2007 at 12:33:35PM +0200, Paul Sokolovsky wrote:
> >
> > We in handhelds.org codebase have attached patch* to make corgi_bl
> > more suitable for general use. This patch was submitted to Richard
> > (so, more votes needed ;-) ). Otherwise, snippet I pasted is from real
> > machine implementation, HP iPaq h4000.
>
> You have my vote! :)
>
> So let me suggest this patch who allows multiple driver instances (I
> use it for both LCD and keypad backlighti devices).
>
> Hope it could be merged into main line (maybe without the "corgi"
> prefix ;-).
Nice patch. I have no problem with it.