[PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Andrey Borzenkov]

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

Subject: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality
From: Andrey Borzenkov <arvidjaar@mail.ru>

Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>

---

This completes backlight series; toshiba_acpi was missing.

 drivers/acpi/toshiba_acpi.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)


diff --git a/drivers/acpi/toshiba_acpi.c b/drivers/acpi/toshiba_acpi.c
index a7db0c2..c140709 100644
--- a/drivers/acpi/toshiba_acpi.c
+++ b/drivers/acpi/toshiba_acpi.c
@@ -780,19 +780,20 @@ static int __init toshiba_acpi_init(void)
 		}
 	}
 
-	toshiba_backlight_device = backlight_device_register("toshiba",
+	if (!acpi_video_backlight_support()) {
+		toshiba_backlight_device = backlight_device_register("toshiba",
 						&toshiba_acpi.p_dev->dev,
 						NULL,
 						&toshiba_backlight_data);
-        if (IS_ERR(toshiba_backlight_device)) {
-		ret = PTR_ERR(toshiba_backlight_device);
-
-		printk(KERN_ERR "Could not register toshiba backlight device\n");
-		toshiba_backlight_device = NULL;
-		toshiba_acpi_exit();
-		return ret;
+		if (IS_ERR(toshiba_backlight_device)) {
+			ret = PTR_ERR(toshiba_backlight_device);
+			printk(KERN_ERR "Could not register toshiba backlight device\n");
+			toshiba_backlight_device = NULL;
+			toshiba_acpi_exit();
+			return ret;
+		}
+		toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
 	}
-        toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
 
 	/* Register rfkill switch for Bluetooth */
 	if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Len Brown]

This patch does what it is supposed to do, but it isn't clear
if that is a step forward.

I have satellite pro (original centrino about 4 years old)
where toshiba-acpi exports 8 brightness levels,
and acpi exports just 3 (min, 40%, max)

so when i apply this patch, i get fewer brightness levels.
I prefer to have all 8 brightness levels.

-Len

On Sat, 8 Nov 2008, Andrey Borzenkov wrote:

> Subject: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality
> From: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>
> 
> ---
> 
> This completes backlight series; toshiba_acpi was missing.
> 
>  drivers/acpi/toshiba_acpi.c |   19 ++++++++++---------
>  1 files changed, 10 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/drivers/acpi/toshiba_acpi.c b/drivers/acpi/toshiba_acpi.c
> index a7db0c2..c140709 100644
> --- a/drivers/acpi/toshiba_acpi.c
> +++ b/drivers/acpi/toshiba_acpi.c
> @@ -780,19 +780,20 @@ static int __init toshiba_acpi_init(void)
>  		}
>  	}
>  
> -	toshiba_backlight_device = backlight_device_register("toshiba",
> +	if (!acpi_video_backlight_support()) {
> +		toshiba_backlight_device = backlight_device_register("toshiba",
>  						&toshiba_acpi.p_dev->dev,
>  						NULL,
>  						&toshiba_backlight_data);
> -        if (IS_ERR(toshiba_backlight_device)) {
> -		ret = PTR_ERR(toshiba_backlight_device);
> -
> -		printk(KERN_ERR "Could not register toshiba backlight device\n");
> -		toshiba_backlight_device = NULL;
> -		toshiba_acpi_exit();
> -		return ret;
> +		if (IS_ERR(toshiba_backlight_device)) {
> +			ret = PTR_ERR(toshiba_backlight_device);
> +			printk(KERN_ERR "Could not register toshiba backlight device\n");
> +			toshiba_backlight_device = NULL;
> +			toshiba_acpi_exit();
> +			return ret;
> +		}
> +		toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
>  	}
> -        toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
>  
>  	/* Register rfkill switch for Bluetooth */
>  	if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {
> 

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

On Tuesday 11 November 2008 02:04:15 pm Len Brown wrote:
> This patch does what it is supposed to do, but it isn't clear
> if that is a step forward.
>
> I have satellite pro (original centrino about 4 years old)
4 years, that means it originally supported XP and Vista support got added?
That would explain both, vendor specific and generic brightness functions.

> where toshiba-acpi exports 8 brightness levels,
> and acpi exports just 3 (min, 40%, max)

> so when i apply this patch, i get fewer brightness levels.
> I prefer to have all 8 brightness levels.

The patch from Andrey is correct.
Looks like all, the ordinary user needs, and probably the way it works on a  
plain Vista OS is there: off, battery, full level.
You can still use the boot param for your specific needs:
acpi_display_output=vendor

     Thomas

> -Len
>
> On Sat, 8 Nov 2008, Andrey Borzenkov wrote:
> > Subject: [PATCH] toshiba_acpi: fingers off backlight if video.ko is
> > serving this functionality From: Andrey Borzenkov <arvidjaar@mail.ru>
> >
> > Signed-off-by: Andrey Borzenkov <arvidjaar@mail.ru>
> >
> > ---
> >
> > This completes backlight series; toshiba_acpi was missing.
> >
> >  drivers/acpi/toshiba_acpi.c |   19 ++++++++++---------
> >  1 files changed, 10 insertions(+), 9 deletions(-)
> >
> >
> > diff --git a/drivers/acpi/toshiba_acpi.c b/drivers/acpi/toshiba_acpi.c
> > index a7db0c2..c140709 100644
> > --- a/drivers/acpi/toshiba_acpi.c
> > +++ b/drivers/acpi/toshiba_acpi.c
> > @@ -780,19 +780,20 @@ static int __init toshiba_acpi_init(void)
> >  		}
> >  	}
> >
> > -	toshiba_backlight_device = backlight_device_register("toshiba",
> > +	if (!acpi_video_backlight_support()) {
> > +		toshiba_backlight_device = backlight_device_register("toshiba",
> >  						&toshiba_acpi.p_dev->dev,
> >  						NULL,
> >  						&toshiba_backlight_data);
> > -        if (IS_ERR(toshiba_backlight_device)) {
> > -		ret = PTR_ERR(toshiba_backlight_device);
> > -
> > -		printk(KERN_ERR "Could not register toshiba backlight device\n");
> > -		toshiba_backlight_device = NULL;
> > -		toshiba_acpi_exit();
> > -		return ret;
> > +		if (IS_ERR(toshiba_backlight_device)) {
> > +			ret = PTR_ERR(toshiba_backlight_device);
> > +			printk(KERN_ERR "Could not register toshiba backlight device\n");
> > +			toshiba_backlight_device = NULL;
> > +			toshiba_acpi_exit();
> > +			return ret;
> > +		}
> > +		toshiba_backlight_device->props.max_brightness =
> > HCI_LCD_BRIGHTNESS_LEVELS - 1; }
> > -        toshiba_backlight_device->props.max_brightness =
> > HCI_LCD_BRIGHTNESS_LEVELS - 1;
> >
> >  	/* Register rfkill switch for Bluetooth */
> >  	if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Matthew Garrett]

On Wed, Nov 12, 2008 at 05:41:35PM -0600, Thomas Renninger wrote:
> On Tuesday 11 November 2008 02:04:15 pm Len Brown wrote:
> > so when i apply this patch, i get fewer brightness levels.
> > I prefer to have all 8 brightness levels.
> 
> The patch from Andrey is correct.
> Looks like all, the ordinary user needs, and probably the way it works on a  
> plain Vista OS is there: off, battery, full level.
> You can still use the boot param for your specific needs:
> acpi_display_output=vendor

The Toshiba BIOSes I've looked at all implement the ACPI code on top of 
the legacy interface. Do you have examples where that's not the case? If 
not, I don't see the harm in exposing both.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Andrey Borzenkov]

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

On Thursday 13 November 2008, Matthew Garrett wrote:
> On Wed, Nov 12, 2008 at 05:41:35PM -0600, Thomas Renninger wrote:
> > On Tuesday 11 November 2008 02:04:15 pm Len Brown wrote:
> > > so when i apply this patch, i get fewer brightness levels.
> > > I prefer to have all 8 brightness levels.
> > 
> > The patch from Andrey is correct.
> > Looks like all, the ordinary user needs, and probably the way it works on a  
> > plain Vista OS is there: off, battery, full level.
> > You can still use the boot param for your specific needs:
> > acpi_display_output=vendor
> 
> The Toshiba BIOSes I've looked at all implement the ACPI code on top of 
> the legacy interface. Do you have examples where that's not the case? If 
> not, I don't see the harm in exposing both.
> 

See http://marc.info/?l=linux-acpi&m=122068736714509&w=2 and related thread.

Basically situation is as follows:

- since some time both video and toshiba_acpi attempt to drive brightness

- on some (many? most?) Toshiba laptops standard-conform ACPI implementation
is inferior in that it is less fine grained than HCI one

- exposing two knobs for the *same* thing confuses user level tools; you never
know which one is used and they compete behind your back

- so effectively on Toshiba you *must* use acpi_brightness=vendor or use
similar default in video_detect.c

- in which case we may just as well play it consistent and add support for
video/vendor switch to toshiba_acpi in case someone does want to use ACPI knob
(be it even only for testing).

So the actual question is - how we detect Toshiba and default to vendor
brightness control?

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Matthew Garrett]

On Thu, Nov 13, 2008 at 07:58:09AM +0300, Andrey Borzenkov wrote:

> - exposing two knobs for the *same* thing confuses user level tools; you never
> know which one is used and they compete behind your back

How do they compete? As I said, the implementations appear to be 
implemented on top of the same underlying functionality. The problem 
with providing both vendor and ACPI functionality comes when both use 
different mechanisms for changing the backlight and so can get out of 
sync with each other. Are there any machines supported by toshiba_acpi 
where this is the case?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Andrey Borzenkov]

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

On Thursday 13 November 2008, Matthew Garrett wrote:
> On Thu, Nov 13, 2008 at 07:58:09AM +0300, Andrey Borzenkov wrote:
> 
> > - exposing two knobs for the *same* thing confuses user level tools; you never
> > know which one is used and they compete behind your back
> 
> How do they compete?

In my case user level program (kpowersave) decided to use video.ko for
brightness control (or, may be, it used them both). Which gave me 2 levels
instead of 8. And there is no way to control it, at least known to me.

Have you looked at link I posted in previous message?

> As I said, the implementations appear to be  
> implemented on top of the same underlying functionality.

I have no idea and do not care how it is implemented internally. The fact
is, going via ACPI _BCM gives me 2 levels (three if zero counts). Going
via proprietary HCI gives 8.

> The problem  
> with providing both vendor and ACPI functionality comes when both use 
> different mechanisms for changing the backlight and so can get out of 
> sync with each other. Are there any machines supported by toshiba_acpi 
> where this is the case?
> 

Yes. Mine and Len's at least :)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Matthew Garrett]

On Sat, Nov 15, 2008 at 07:30:57PM +0300, Andrey Borzenkov wrote:
> On Thursday 13 November 2008, Matthew Garrett wrote:
> > On Thu, Nov 13, 2008 at 07:58:09AM +0300, Andrey Borzenkov wrote:
> > 
> > > - exposing two knobs for the *same* thing confuses user level tools; you never
> > > know which one is used and they compete behind your back
> > 
> > How do they compete?
> 
> In my case user level program (kpowersave) decided to use video.ko for
> brightness control (or, may be, it used them both). Which gave me 2 levels
> instead of 8. And there is no way to control it, at least known to me.

Right. But that doesn't mean they're competing, as such. If you set the 
brightness via toshiba_acpi to a value that isn't supported via the 
acpi driver, what value does the acpi backlight claim to be at?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Andrey Borzenkov]

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

On Saturday 15 November 2008, Matthew Garrett wrote:
> On Sat, Nov 15, 2008 at 07:30:57PM +0300, Andrey Borzenkov wrote:
> > On Thursday 13 November 2008, Matthew Garrett wrote:
> > > On Thu, Nov 13, 2008 at 07:58:09AM +0300, Andrey Borzenkov wrote:
> > > 
> > > > - exposing two knobs for the *same* thing confuses user level tools; you never
> > > > know which one is used and they compete behind your back
> > > 
> > > How do they compete?
> > 
> > In my case user level program (kpowersave) decided to use video.ko for
> > brightness control (or, may be, it used them both). Which gave me 2 levels
> > instead of 8. And there is no way to control it, at least known to me.
> 
> Right. But that doesn't mean they're competing, as such. If you set the 
> brightness via toshiba_acpi

I probably have problems with expressing myself as non-native English
speaker.

I am not interested in setting values via echoing into sysfs file. I
am interested in my desktop brightness control working out of the box.
And desktop driver control has no way to select, which of two sysfs
files to use. Nor do I understand why I have to create this problem
of selecting right driver when I already have possibility to avoid it.

If you think exposing both knobs is non-issue, why are all those patches
for other vendor drivers included in the kernel in the first place?

> to a value that isn't supported via the  
> acpi driver, what value does the acpi backlight claim to be at?
> 



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Matthew Garrett]

On Sat, Nov 15, 2008 at 08:05:04PM +0300, Andrey Borzenkov wrote:
> On Saturday 15 November 2008, Matthew Garrett wrote:
> > Right. But that doesn't mean they're competing, as such. If you set the 
> > brightness via toshiba_acpi
> 
> I probably have problems with expressing myself as non-native English
> speaker.
> 
> I am not interested in setting values via echoing into sysfs file. I
> am interested in my desktop brightness control working out of the box.
> And desktop driver control has no way to select, which of two sysfs
> files to use. Nor do I understand why I have to create this problem
> of selecting right driver when I already have possibility to avoid it.
> 
> If you think exposing both knobs is non-issue, why are all those patches
> for other vendor drivers included in the kernel in the first place?

Because in some of those cases, the ACPI and vendor function are 
implemented in different ways that can then get out of sync with each 
other. As a result, you can get garbage information. If the values in 
your two backlight interfaces are always sane, then there's no inherent 
need to hide one of them. The kernel exposes the available functionality 
and userland then determines the policy used to choose one over the 
other.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Andrey Borzenkov]

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

On Saturday 15 November 2008, Matthew Garrett wrote:
> On Sat, Nov 15, 2008 at 08:05:04PM +0300, Andrey Borzenkov wrote:
> > On Saturday 15 November 2008, Matthew Garrett wrote:
> > > Right. But that doesn't mean they're competing, as such. If you set the 
> > > brightness via toshiba_acpi
> > 
> > I probably have problems with expressing myself as non-native English
> > speaker.
> > 
> > I am not interested in setting values via echoing into sysfs file. I
> > am interested in my desktop brightness control working out of the box.
> > And desktop driver control has no way to select, which of two sysfs
> > files to use. Nor do I understand why I have to create this problem
> > of selecting right driver when I already have possibility to avoid it.
> > 
> > If you think exposing both knobs is non-issue, why are all those patches
> > for other vendor drivers included in the kernel in the first place?
> 
> Because in some of those cases, the ACPI and vendor function are 
> implemented in different ways that can then get out of sync with each 
> other. As a result, you can get garbage information. If the values in 
> your two backlight interfaces are always sane, then there's no inherent 
> need to hide one of them.

THE VALUE IN MY TWO BACKLIGHT INTERFACES ARE NOT THE SAME. I said this many
times already. What exactly is not clear in this sentence?

> The kernel exposes the available functionality  
> and userland then determines the policy used to choose one over the 
> other.
> 



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Matthew Garrett]

On Sat, Nov 15, 2008 at 08:17:52PM +0300, Andrey Borzenkov wrote:
> On Saturday 15 November 2008, Matthew Garrett wrote:
> > > If you think exposing both knobs is non-issue, why are all those patches
> > > for other vendor drivers included in the kernel in the first place?
> > 
> > Because in some of those cases, the ACPI and vendor function are 
> > implemented in different ways that can then get out of sync with each 
> > other. As a result, you can get garbage information. If the values in 
> > your two backlight interfaces are always sane, then there's no inherent 
> > need to hide one of them.
> 
> THE VALUE IN MY TWO BACKLIGHT INTERFACES ARE NOT THE SAME. I said this many
> times already. What exactly is not clear in this sentence?

Where did I say that they were? You never answered the question I asked 
- if you set the value in the toshiba specific backlight control to a 
value that isn't supported via the generic acpi one, what value does the 
generic acpi one claim to have?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Andrey Borzenkov]

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

On Saturday 15 November 2008, Matthew Garrett wrote:
> On Sat, Nov 15, 2008 at 08:17:52PM +0300, Andrey Borzenkov wrote:
> > On Saturday 15 November 2008, Matthew Garrett wrote:
> > > > If you think exposing both knobs is non-issue, why are all those patches
> > > > for other vendor drivers included in the kernel in the first place?
> > > 
> > > Because in some of those cases, the ACPI and vendor function are 
> > > implemented in different ways that can then get out of sync with each 
> > > other. As a result, you can get garbage information. If the values in 
> > > your two backlight interfaces are always sane, then there's no inherent 
> > > need to hide one of them.
> > 
> > THE VALUE IN MY TWO BACKLIGHT INTERFACES ARE NOT THE SAME. I said this many
> > times already. What exactly is not clear in this sentence?
> 
> Where did I say that they were? You never answered the question I asked 
> - if you set the value in the toshiba specific backlight control to a 
> value that isn't supported via the generic acpi one, what value does the 
> generic acpi one claim to have?
> 

sh-3.2# cat /sys/class/backlight/acpi_video0/brightness
2
sh-3.2# cat /sys/class/backlight/acpi_video0/actual_brightness
2
sh-3.2# cat /sys/class/backlight/acpi_video0/max_brightness
2
sh-3.2# cat /sys/class/backlight/toshiba/brightness
7
sh-3.2# cat /sys/class/backlight/toshiba/actual_brightness
7
sh-3.2# cat /sys/class/backlight/toshiba/max_brightness
7
sh-3.2# echo 5 > /sys/class/backlight/toshiba/brightnes
sh-3.2# cat /sys/class/backlight/acpi_video0/brightness
2
sh-3.2# cat /sys/class/backlight/acpi_video0/actual_brightness
2
sh-3.2# echo 5 > /sys/class/backlight/acpi_video0/brightness
sh: echo: write error: Invalid argument
sh-3.2# echo 1 > /sys/class/backlight/acpi_video0/brightness
sh-3.2# cat /sys/class/backlight/toshiba/brightness
5
sh-3.2# cat /sys/class/backlight/toshiba/actual_brightness
3
sh-3.2# echo 7 > /sys/class/backlight/toshiba/brightness
sh-3.2# cat /sys/class/backlight/acpi_video0/brightness
1
sh-3.2# cat /sys/class/backlight/acpi_video0/actual_brightness
1


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Matthew Garrett]

Ok, so they can get unsynchronised. I agree with Thomas that it's 
desirable to remove one of them in that case. As you and Len have 
pointed out, the difficulty is in knowing which one to remove.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Henrique de Moraes Holschuh]

On Sat, 15 Nov 2008, Matthew Garrett wrote:
> Ok, so they can get unsynchronised. I agree with Thomas that it's 
> desirable to remove one of them in that case. As you and Len have 
> pointed out, the difficulty is in knowing which one to remove.

May I point out the obvious, and *very* annoying fact?

It is clear by now that if we want to solve all border conditions nicely, we
will need centralized control of backlight interfaces to broker between
platform-specific drivers and ACPI generic.

My idea is: separate them in two layers.  Have the "backends" (ACPI generic
and platform specific drivers) register parameters with a "frontend".  The
"frontend" exposes a *single* backlight interface.  The backends expose
NOTHING (or at most a deprecated interface if it won't break things).

Backlight interfaces can have their parameters updated at runtime, so we do
just that if the frontend has to switch backends (i.e. due to module
loading/unloading).  Make sure the backend is informed when it is
connected/disconnected from driving the backlight.

Use an intelligent setup to select which backend should be driving the
backlight.  e.g: the backends provide a priority information and a quality
information (priority is low-medium-high.  Quality is the number of levels,
adjusted up or down if you know there is something special about it that
should lower or rise the quality).

If there is only one backend loaded, select that.  If there are more than
one, select the highest priority one.  When the priority of the backends is
the same, select the one with the highest quality. When quality and priority
are the same, select the generic ACPI.

Priority should always be medium, except when you know for sure that you
need to do something special for an specific platform/box.

Any other ideas, comments?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Thomas Renninger]

On Sunday 16 November 2008 06:51:14 am Henrique de Moraes Holschuh wrote:
> On Sat, 15 Nov 2008, Matthew Garrett wrote:
> > Ok, so they can get unsynchronised. I agree with Thomas that it's
> > desirable to remove one of them in that case. As you and Len have
> > pointed out, the difficulty is in knowing which one to remove.
>
> May I point out the obvious, and *very* annoying fact?
>
> It is clear by now that if we want to solve all border conditions nicely,
> we will need centralized control of backlight interfaces to broker between
> platform-specific drivers and ACPI generic.
>
> My idea is: separate them in two layers.  Have the "backends" (ACPI generic
> and platform specific drivers) register parameters with a "frontend".  The
> "frontend" exposes a *single* backlight interface.  The backends expose
> NOTHING (or at most a deprecated interface if it won't break things).
>
> Backlight interfaces can have their parameters updated at runtime, so we do
> just that if the frontend has to switch backends (i.e. due to module
> loading/unloading).  Make sure the backend is informed when it is
> connected/disconnected from driving the backlight.
>
> Use an intelligent setup to select which backend should be driving the
> backlight.  e.g: the backends provide a priority information and a quality
> information (priority is low-medium-high.  Quality is the number of levels,
> adjusted up or down if you know there is something special about it that
> should lower or rise the quality).
>
> If there is only one backend loaded, select that.  If there are more than
> one, select the highest priority one.  When the priority of the backends is
> the same, select the one with the highest quality. When quality and
> priority are the same, select the generic ACPI.
>
> Priority should always be medium, except when you know for sure that you
> need to do something special for an specific platform/box.
>
> Any other ideas, comments?

I would not add such complexity for a problem which isn't a real problem.
IMO we should either:
   1) Just do nothing and use video.ko even for Toshibas which only provide
       3 brightness states.
   2) DMI blacklist for Toshiba in general to use toshiba_laptop for
       brightness switching.

The whole problem of vendor.ko via video.ko is similar to
"should acpi be enabled or not" for about 5 year old machines.
It took years to find out most corner cases.
But if you have the wrong assumption made there the machine is not booting.

For the brightness level it's something else. A short documentation into the 
right forum/mailing list and everybody can google the 
acpi_backlight=vendor/video boot param in a second on his already running 
machine and is happy.

There where we know things are broken for a default choice we can add a short 
dmi blacklist.
The question is, are these 3 brightness levels to be considered as broken.

IMO it is something 95% of all toshiba users won't care, the brightness level 
can be dimmed for battery usage and if they care, they can still easily tune 
it. 
I very much expect that newer Toshibas export more brightness levels via 
video.ko (does someone have a new one and can double check?) and at some time 
the Toshiba specific functions may even vanish. Therefore I would prefer to 
not do an exception here and go for 1(see above).

   Thomas

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Henrique de Moraes Holschuh]

On Sun, 16 Nov 2008, Thomas Renninger wrote:
> I would not add such complexity for a problem which isn't a real problem.

Oh, it is very real.  It needs to be solved, and yes, it can be solved with
what we have and adding blacklists to ACPI video, instead of intelligent
decisions or hints from the platform driver.

> IMO we should either:
>    1) Just do nothing and use video.ko even for Toshibas which only provide
>        3 brightness states.

I would be quite annoyed at it if I were a Toshiba user and I would report
it as a regression.  But I guess we can see if any Toshiba users want to
chime in?

>    2) DMI blacklist for Toshiba in general to use toshiba_laptop for
>        brightness switching.

You won't get away with halfway broken measures like that, you will need to
blacklist *some* toshibas, but not all.  As you say yourself later on, it is
likely that newer ones might switch to ACPI generic, or have more levels in
ACPI generic backlight handling.

> For the brightness level it's something else. A short documentation into the 
> right forum/mailing list and everybody can google the 
> acpi_backlight=vendor/video boot param in a second on his already running 
> machine and is happy.

I thought we were trying to get things right automatically instead of
forcing users to add such parameters, nowadays?  Maybe if this was something
easy to change at runtime, but kernel parameters and Kconfig parameters
*are* supposed to be a last-resort option.

> The question is, are these 3 brightness levels to be considered as broken.

It is a regression, anyway.  It delivers reduced functionality.

> IMO it is something 95% of all toshiba users won't care, the brightness level 

But we care about the 5% that do.  Those are the users that are worth
bothering with, since they're the ones that do bug reports, stay around to
test patches, etc.

> I very much expect that newer Toshibas export more brightness levels via 
> video.ko (does someone have a new one and can double check?) and at some time 
> the Toshiba specific functions may even vanish. Therefore I would prefer to 
> not do an exception here and go for 1(see above).

I don't really care if it is done just by DMI blacklisting on ACPI video,
but I don't expect that to be the better solution for the long term.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] toshiba_acpi: fingers off backlight if video.ko is serving this functionality

[Len Brown]

I'm running FC10 on my toshiba, and pressing the hot-keys
gives me 8 levels, so somehow Fedora chooses the toshiba
levels instead of the ACPI video levels.  I too would view it
as a regression if this went down to 3 levels, even if it
is working by pure luck today.  Unfortunately, Andrey
is using a distro or window system that is less lucky?:-)

I like Henrique's suggestion of having the drivers register
a quality with the backlight sub-system and somehow having
the highest quality driver win.  In the case of a tie,
however, I'd want the ACPI video driver to win:-)

cheers,
-Len

Video.ko-vs-toshiba.ko-more-brightness-levels-win

[Thomas Renninger]

Does this work out?

I don't like the acpi_video_device_lcd_query_levels exported
in video_detect.c now.
But duplicating the code also did not look nice (maybe it's
still better?)
Better ideas?

Compile tested against latest Linus git tree.
Please test/review.

   Thomas

[PATCH 1/2] ACPI: acpi_video_backlight_support return found generic video brightness levels

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/video.c        |   38 +++------------------
 drivers/acpi/video_detect.c |   74 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 73 insertions(+), 39 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index baa4419..239800b 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -445,37 +445,6 @@ acpi_video_device_set_state(struct acpi_video_device *device, int state)
 }
 
 static int
-acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
-				   union acpi_object **levels)
-{
-	int status;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-
-
-	*levels = NULL;
-
-	status = acpi_evaluate_object(device->dev->handle, "_BCL", NULL, &buffer);
-	if (!ACPI_SUCCESS(status))
-		return status;
-	obj = (union acpi_object *)buffer.pointer;
-	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
-		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
-		status = -EFAULT;
-		goto err;
-	}
-
-	*levels = obj;
-
-	return 0;
-
-      err:
-	kfree(buffer.pointer);
-
-	return status;
-}
-
-static int
 acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 {
 	int status = AE_OK;
@@ -625,6 +594,10 @@ acpi_video_bus_DOS(struct acpi_video_bus *video, int bios_flag, int lcd_flag)
 	return status;
 }
 
+/* from video_detect.c */
+int
+acpi_video_device_lcd_query_levels(acpi_handle handle,
+				   union acpi_object **levels);
 /*
  *  Arg:	
  *  	device	: video output device (LCD, CRT, ..)
@@ -643,7 +616,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br = NULL;
 
-	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
+	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->dev->
+							     handle, &obj))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
 						"LCD brightness level\n"));
 		goto out;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index f022eb6..29e317c 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -42,19 +42,62 @@ ACPI_MODULE_NAME("video");
 
 static long acpi_video_support;
 static bool acpi_video_caps_checked;
+static int brightness_levels;
+
+/* also used in video.c */
+int
+acpi_video_device_lcd_query_levels(acpi_handle handle,
+				   union acpi_object **levels)
+{
+	int status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+
+	*levels = NULL;
+
+	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
+	if (!ACPI_SUCCESS(status))
+		return status;
+	obj = (union acpi_object *)buffer.pointer;
+	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
+		status = -EFAULT;
+		goto err;
+	}
+
+	*levels = obj;
+
+	return 0;
+
+      err:
+	kfree(buffer.pointer);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(acpi_video_device_lcd_query_levels);
 
 static acpi_status
 acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
-			  void **retyurn_value)
+			  void **return_value)
 {
 	long *cap = context;
 	acpi_handle h_dummy;
+	union acpi_object *obj = NULL;
+	int ret;
 
 	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy)) &&
 	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
 				  "support\n"));
-		*cap |= ACPI_VIDEO_BACKLIGHT;
+
+		ret = acpi_video_device_lcd_query_levels(handle, &obj);
+		if (!ret)
+			brightness_levels = obj->package.count;
+		kfree(obj);
+
+		if (brightness_levels > 1)
+			*cap |= ACPI_VIDEO_BACKLIGHT;
+		
 		/* We have backlight support, no need to scan further */
 		return AE_CTRL_TERMINATE;
 	}
@@ -178,7 +221,14 @@ long acpi_video_get_capabilities(acpi_handle graphics_handle)
 }
 EXPORT_SYMBOL(acpi_video_get_capabilities);
 
-/* Returns true if video.ko can do backlight switching */
+/*
+ * Returns:
+ * >  1 - found _BCL brightness levels -> generic video support
+ * == 1 - could not get brightness level through _BCL, but generic video
+ *        support enforced, due to dmi or boot param, should not happen
+ * == 0 - no generic video functions found or vendor specific
+ *        support enforced via dmi or boot param.
+ */
 int acpi_video_backlight_support(void)
 {
 	/*
@@ -192,16 +242,26 @@ int acpi_video_backlight_support(void)
 	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
 		return 0;
 	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
-		return 1;
+		goto video;
 
 	/* Then check for DMI blacklist -> second highest prio */
 	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
 		return 0;
 	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
-		return 1;
+		goto video;
 
-	/* Then go the default way */
-	return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
+ video:
+	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT) {
+		if (brightness_levels < 2) {
+			printk(KERN_WARNING "Default Video backlight interface"
+			       " chosen, but no brightness levels found\n");
+			return 1;
+		}
+		else
+			return brightness_levels;
+	}
+	else
+		return 0;
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 
-- 
1.6.0.2

Re: [PATCH 1/2] ACPI: acpi_video_backlight_support return found generic video brightness levels

[Thomas Renninger]

On Thursday 27 November 2008 12:39:26 Thomas Renninger wrote:
> Signed-off-by: Thomas Renninger <trenn@suse.de>
> ---
>  drivers/acpi/video.c        |   38 +++------------------
>  drivers/acpi/video_detect.c |   74
> ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 73
> insertions(+), 39 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index baa4419..239800b 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -445,37 +445,6 @@ acpi_video_device_set_state(struct acpi_video_device
> *device, int state) }
>
>  static int
> -acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
> -				   union acpi_object **levels)
> -{
> -	int status;
> -	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> -	union acpi_object *obj;
> -
> -
> -	*levels = NULL;
> -
> -	status = acpi_evaluate_object(device->dev->handle, "_BCL", NULL,
> &buffer); -	if (!ACPI_SUCCESS(status))
> -		return status;
> -	obj = (union acpi_object *)buffer.pointer;
> -	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> -		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
> -		status = -EFAULT;
> -		goto err;
> -	}
> -
> -	*levels = obj;
> -
> -	return 0;
> -
> -      err:
> -	kfree(buffer.pointer);
> -
> -	return status;
> -}
> -
> -static int
>  acpi_video_device_lcd_set_level(struct acpi_video_device *device, int
> level) {
>  	int status = AE_OK;
> @@ -625,6 +594,10 @@ acpi_video_bus_DOS(struct acpi_video_bus *video, int
> bios_flag, int lcd_flag) return status;
>  }
>
> +/* from video_detect.c */
> +int
> +acpi_video_device_lcd_query_levels(acpi_handle handle,
> +				   union acpi_object **levels);
>  /*
>   *  Arg:
>   *  	device	: video output device (LCD, CRT, ..)
> @@ -643,7 +616,8 @@ acpi_video_init_brightness(struct acpi_video_device
> *device) union acpi_object *o;
>  	struct acpi_video_device_brightness *br = NULL;
>
> -	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
> +	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->dev->
> +							     handle, &obj))) {
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
>  						"LCD brightness level\n"));
>  		goto out;
> diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
> index f022eb6..29e317c 100644
> --- a/drivers/acpi/video_detect.c
> +++ b/drivers/acpi/video_detect.c
> @@ -42,19 +42,62 @@ ACPI_MODULE_NAME("video");
>
>  static long acpi_video_support;
>  static bool acpi_video_caps_checked;
> +static int brightness_levels;
> +
> +/* also used in video.c */
> +int
> +acpi_video_device_lcd_query_levels(acpi_handle handle,
> +				   union acpi_object **levels)
> +{
> +	int status;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *obj;
> +
> +	*levels = NULL;
> +
> +	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
> +	if (!ACPI_SUCCESS(status))
> +		return status;
> +	obj = (union acpi_object *)buffer.pointer;
> +	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
> +		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
> +		status = -EFAULT;
> +		goto err;
> +	}
> +
> +	*levels = obj;
> +
> +	return 0;
> +
> +      err:
> +	kfree(buffer.pointer);
> +
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(acpi_video_device_lcd_query_levels);
>
>  static acpi_status
>  acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
> -			  void **retyurn_value)
> +			  void **return_value)
>  {
>  	long *cap = context;
>  	acpi_handle h_dummy;
> +	union acpi_object *obj = NULL;
> +	int ret;
>
>  	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy)) &&
>  	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy))) {
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
>  				  "support\n"));
> -		*cap |= ACPI_VIDEO_BACKLIGHT;
> +
> +		ret = acpi_video_device_lcd_query_levels(handle, &obj);
> +		if (!ret)
> +			brightness_levels = obj->package.count;
> +		kfree(obj);
> +
> +		if (brightness_levels > 1)
> +			*cap |= ACPI_VIDEO_BACKLIGHT;
> +
>  		/* We have backlight support, no need to scan further */
>  		return AE_CTRL_TERMINATE;
>  	}
> @@ -178,7 +221,14 @@ long acpi_video_get_capabilities(acpi_handle
> graphics_handle) }
>  EXPORT_SYMBOL(acpi_video_get_capabilities);
>
> -/* Returns true if video.ko can do backlight switching */
> +/*
> + * Returns:
> + * >  1 - found _BCL brightness levels -> generic video support
> + * == 1 - could not get brightness level through _BCL, but generic video
> + *        support enforced, due to dmi or boot param, should not happen
> + * == 0 - no generic video functions found or vendor specific
> + *        support enforced via dmi or boot param.
> + */
>  int acpi_video_backlight_support(void)
>  {
>  	/*
> @@ -192,16 +242,26 @@ int acpi_video_backlight_support(void)
>  	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
>  		return 0;
>  	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
> -		return 1;
> +		goto video;
>
>  	/* Then check for DMI blacklist -> second highest prio */
>  	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
>  		return 0;
>  	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
> -		return 1;
> +		goto video;
This is not correct, it may return 0 even acpi_backlight=video is given.
It should return 1, even no brightness levels are found.
The patch in the end is better.
>
> -	/* Then go the default way */
> -	return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
> +	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT) {
> + video:
> +		if (brightness_levels < 2) {
> +			printk(KERN_WARNING "Default Video backlight interface"
> +			       " chosen, but no brightness levels found\n");
> +			return 1;
> +		}
> +		else
> +			return brightness_levels;
> +	}
> +	else
> +		return 0;
>  }
>  EXPORT_SYMBOL(acpi_video_backlight_support);

===========

ACPI: acpi_video_backlight_support return found generic video brightness levels


Signed-off-by: Thomas Renninger <trenn@suse.de>

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index baa4419..239800b 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -445,37 +445,6 @@ acpi_video_device_set_state(struct acpi_video_device *device, int state)
 }
 
 static int
-acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
-				   union acpi_object **levels)
-{
-	int status;
-	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
-	union acpi_object *obj;
-
-
-	*levels = NULL;
-
-	status = acpi_evaluate_object(device->dev->handle, "_BCL", NULL, &buffer);
-	if (!ACPI_SUCCESS(status))
-		return status;
-	obj = (union acpi_object *)buffer.pointer;
-	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
-		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
-		status = -EFAULT;
-		goto err;
-	}
-
-	*levels = obj;
-
-	return 0;
-
-      err:
-	kfree(buffer.pointer);
-
-	return status;
-}
-
-static int
 acpi_video_device_lcd_set_level(struct acpi_video_device *device, int level)
 {
 	int status = AE_OK;
@@ -625,6 +594,10 @@ acpi_video_bus_DOS(struct acpi_video_bus *video, int bios_flag, int lcd_flag)
 	return status;
 }
 
+/* from video_detect.c */
+int
+acpi_video_device_lcd_query_levels(acpi_handle handle,
+				   union acpi_object **levels);
 /*
  *  Arg:	
  *  	device	: video output device (LCD, CRT, ..)
@@ -643,7 +616,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
 	union acpi_object *o;
 	struct acpi_video_device_brightness *br = NULL;
 
-	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
+	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device->dev->
+							     handle, &obj))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
 						"LCD brightness level\n"));
 		goto out;
diff --git a/drivers/acpi/video_detect.c b/drivers/acpi/video_detect.c
index f022eb6..f8ebe9c 100644
--- a/drivers/acpi/video_detect.c
+++ b/drivers/acpi/video_detect.c
@@ -42,19 +42,62 @@ ACPI_MODULE_NAME("video");
 
 static long acpi_video_support;
 static bool acpi_video_caps_checked;
+static int brightness_levels;
+
+/* also used in video.c */
+int
+acpi_video_device_lcd_query_levels(acpi_handle handle,
+				   union acpi_object **levels)
+{
+	int status;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *obj;
+
+	*levels = NULL;
+
+	status = acpi_evaluate_object(handle, "_BCL", NULL, &buffer);
+	if (!ACPI_SUCCESS(status))
+		return status;
+	obj = (union acpi_object *)buffer.pointer;
+	if (!obj || (obj->type != ACPI_TYPE_PACKAGE)) {
+		printk(KERN_ERR PREFIX "Invalid _BCL data\n");
+		status = -EFAULT;
+		goto err;
+	}
+
+	*levels = obj;
+
+	return 0;
+
+      err:
+	kfree(buffer.pointer);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(acpi_video_device_lcd_query_levels);
 
 static acpi_status
 acpi_backlight_cap_match(acpi_handle handle, u32 level, void *context,
-			  void **retyurn_value)
+			  void **return_value)
 {
 	long *cap = context;
 	acpi_handle h_dummy;
+	union acpi_object *obj = NULL;
+	int ret;
 
 	if (ACPI_SUCCESS(acpi_get_handle(handle, "_BCM", &h_dummy)) &&
 	    ACPI_SUCCESS(acpi_get_handle(handle, "_BCL", &h_dummy))) {
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
 				  "support\n"));
-		*cap |= ACPI_VIDEO_BACKLIGHT;
+
+		ret = acpi_video_device_lcd_query_levels(handle, &obj);
+		if (!ret)
+			brightness_levels = obj->package.count;
+		kfree(obj);
+
+		if (brightness_levels > 1)
+			*cap |= ACPI_VIDEO_BACKLIGHT;
+		
 		/* We have backlight support, no need to scan further */
 		return AE_CTRL_TERMINATE;
 	}
@@ -178,7 +221,14 @@ long acpi_video_get_capabilities(acpi_handle graphics_handle)
 }
 EXPORT_SYMBOL(acpi_video_get_capabilities);
 
-/* Returns true if video.ko can do backlight switching */
+/*
+ * Returns:
+ * >  1 - found _BCL brightness levels -> generic video support
+ * == 1 - could not get brightness level through _BCL, but generic video
+ *        support enforced, due to dmi or boot param, should not happen
+ * == 0 - no generic video functions found or vendor specific
+ *        support enforced via dmi or boot param.
+ */
 int acpi_video_backlight_support(void)
 {
 	/*
@@ -192,16 +242,26 @@ int acpi_video_backlight_support(void)
 	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VENDOR)
 		return 0;
 	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_FORCE_VIDEO)
-		return 1;
+		goto video;
 
 	/* Then check for DMI blacklist -> second highest prio */
 	if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VENDOR)
 		return 0;
 	else if (acpi_video_support & ACPI_VIDEO_BACKLIGHT_DMI_VIDEO)
+		goto video;
+
+	if (!(acpi_video_support & ACPI_VIDEO_BACKLIGHT))
+		return 0;
+
+ video:
+	if (brightness_levels < 2) {
+		printk(KERN_WARNING "Default Video backlight interface"
+		       " chosen, but no brightness levels found\n");
 		return 1;
+	}
+	else
+		return brightness_levels;
 
-	/* Then go the default way */
-	return acpi_video_support & ACPI_VIDEO_BACKLIGHT;
 }
 EXPORT_SYMBOL(acpi_video_backlight_support);
 

[PATCH 2/2] Video.ko vs toshiba.ko - more brightness levels win

[Thomas Renninger]

Signed-off-by: Thomas Renninger <trenn@suse.de>
---
 drivers/acpi/toshiba_acpi.c |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/acpi/toshiba_acpi.c b/drivers/acpi/toshiba_acpi.c
index 66aac06..843009e 100644
--- a/drivers/acpi/toshiba_acpi.c
+++ b/drivers/acpi/toshiba_acpi.c
@@ -780,19 +780,23 @@ static int __init toshiba_acpi_init(void)
 		}
 	}
 
-	toshiba_backlight_device = backlight_device_register("toshiba",
+        toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
+
+	if (acpi_video_backlight_support() <
+	    toshiba_backlight_device->props.max_brightness) {
+		toshiba_backlight_device = backlight_device_register("toshiba",
 						&toshiba_acpi.p_dev->dev,
 						NULL,
 						&toshiba_backlight_data);
-        if (IS_ERR(toshiba_backlight_device)) {
-		ret = PTR_ERR(toshiba_backlight_device);
+		if (IS_ERR(toshiba_backlight_device)) {
+			ret = PTR_ERR(toshiba_backlight_device);
 
-		printk(KERN_ERR "Could not register toshiba backlight device\n");
-		toshiba_backlight_device = NULL;
-		toshiba_acpi_exit();
-		return ret;
+			printk(KERN_ERR "Could not register toshiba backlight device\n");
+			toshiba_backlight_device = NULL;
+			toshiba_acpi_exit();
+			return ret;
+		}
 	}
-        toshiba_backlight_device->props.max_brightness = HCI_LCD_BRIGHTNESS_LEVELS - 1;
 
 	/* Register rfkill switch for Bluetooth */
 	if (hci_get_bt_present(&bt_present) == HCI_SUCCESS && bt_present) {
-- 
1.6.0.2