* [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops.
@ 2013-03-19 16:22 Danny Baumann
2013-03-19 16:22 ` [PATCH v4 2/3] ACPI video: Make logic a little easier to understand Danny Baumann
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Danny Baumann @ 2013-03-19 16:22 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-kernel, Zhang Rui, Len Brown, Danny Baumann
In particular, this fixes brightness control initialization for all
devices that return index values from _BQC and don't happen to have the
initial index set by the BIOS in their _BCL table. One example for that
is the Dell Inspiron 15R SE (model number 7520).
What happened for those devices is that acpi_init_brightness queried the
initial brightness by calling acpi_video_device_lcd_get_level_current.
This called _BQC, which returned e.g. 13. As _BQC_use_index isn't
determined at this point (and thus has its initial value of 0), the
index isn't converted into the actual level. As '13' isn't present in
the _BCL list, *level is later overwritten with brightness->curr, which
was initialized to max_level (100) before. Later in
acpi_init_brightness, level_old (with the value 100) is used as an index
into the _BCL table, which causes a value outside of the actual table to
be used as input into acpi_video_device_lcd_set_level(). Depending on
the (undefined) value of that location, this call will fail, causing the
brightness control for the device in question not to be enabled.
Fix that by returning the raw value returned by the _BQC call in the
initialization case.
Signed-off-by: Danny Baumann <dannybaumann@web.de>
---
drivers/acpi/video.c | 43 ++++++++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 313f959..09e4722 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
int level);
static int acpi_video_device_lcd_get_level_current(
struct acpi_video_device *device,
- unsigned long long *level, int init);
+ unsigned long long *level, bool raw);
static int acpi_video_get_next_level(struct acpi_video_device *device,
u32 level_current, u32 event);
static int acpi_video_switch_brightness(struct acpi_video_device *device,
@@ -236,7 +236,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
struct acpi_video_device *vd =
(struct acpi_video_device *)bl_get_data(bd);
- if (acpi_video_device_lcd_get_level_current(vd, &cur_level, 0))
+ if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
return -EINVAL;
for (i = 2; i < vd->brightness->count; i++) {
if (vd->brightness->levels[i] == cur_level)
@@ -281,7 +281,7 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
unsigned long long level;
int offset;
- if (acpi_video_device_lcd_get_level_current(video, &level, 0))
+ if (acpi_video_device_lcd_get_level_current(video, &level, false))
return -EINVAL;
for (offset = 2; offset < video->brightness->count; offset++)
if (level == video->brightness->levels[offset]) {
@@ -452,7 +452,7 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
static int
acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
- unsigned long long *level, int init)
+ unsigned long long *level, bool raw)
{
acpi_status status = AE_OK;
int i;
@@ -463,6 +463,15 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
status = acpi_evaluate_integer(device->dev->handle, buf,
NULL, level);
if (ACPI_SUCCESS(status)) {
+ if (raw) {
+ /*
+ * Caller has indicated he wants the raw
+ * value returned by _BQC, so don't furtherly
+ * mess with the value.
+ */
+ return 0;
+ }
+
if (device->brightness->flags._BQC_use_index) {
if (device->brightness->flags._BCL_reversed)
*level = device->brightness->count
@@ -476,16 +485,14 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
device->brightness->curr = *level;
return 0;
}
- if (!init) {
- /*
- * BQC returned an invalid level.
- * Stop using it.
- */
- ACPI_WARNING((AE_INFO,
- "%s returned an invalid level",
- buf));
- device->cap._BQC = device->cap._BCQ = 0;
- }
+ /*
+ * BQC returned an invalid level.
+ * Stop using it.
+ */
+ ACPI_WARNING((AE_INFO,
+ "%s returned an invalid level",
+ buf));
+ device->cap._BQC = device->cap._BCQ = 0;
} else {
/* Fixme:
* should we return an error or ignore this failure?
@@ -703,7 +710,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
if (!device->cap._BQC)
goto set_level;
- result = acpi_video_device_lcd_get_level_current(device, &level_old, 1);
+ result = acpi_video_device_lcd_get_level_current(device,
+ &level_old, true);
if (result)
goto out_free_levels;
@@ -714,7 +722,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
if (result)
goto out_free_levels;
- result = acpi_video_device_lcd_get_level_current(device, &level, 0);
+ result = acpi_video_device_lcd_get_level_current(device, &level, true);
if (result)
goto out_free_levels;
@@ -1268,7 +1276,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
goto out;
result = acpi_video_device_lcd_get_level_current(device,
- &level_current, 0);
+ &level_current,
+ false);
if (result)
goto out;
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 2/3] ACPI video: Make logic a little easier to understand.
2013-03-19 16:22 [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
@ 2013-03-19 16:22 ` Danny Baumann
2013-03-19 16:22 ` [PATCH v4 3/3] ACPI video: Fix applying indexed initial brightness value Danny Baumann
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Danny Baumann @ 2013-03-19 16:22 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-kernel, Zhang Rui, Len Brown, Danny Baumann
Make code paths a little easier to follow, and don't needlessly continue
list iteration.
Signed-off-by: Danny Baumann <dannybaumann@web.de>
---
drivers/acpi/video.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 09e4722..89bf61e 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -737,16 +737,17 @@ acpi_video_init_brightness(struct acpi_video_device *device)
*/
if (use_bios_initial_backlight) {
for (i = 2; i < br->count; i++)
- if (level_old == br->levels[i])
+ if (level_old == br->levels[i]) {
level = level_old;
+ break;
+ }
}
- goto set_level;
+ } else {
+ if (br->flags._BCL_reversed)
+ level_old = (br->count - 1) - level_old;
+ level = br->levels[level_old];
}
- if (br->flags._BCL_reversed)
- level_old = (br->count - 1) - level_old;
- level = br->levels[level_old];
-
set_level:
result = acpi_video_device_lcd_set_level(device, level);
if (result)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v4 3/3] ACPI video: Fix applying indexed initial brightness value.
2013-03-19 16:22 [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
2013-03-19 16:22 ` [PATCH v4 2/3] ACPI video: Make logic a little easier to understand Danny Baumann
@ 2013-03-19 16:22 ` Danny Baumann
2013-03-19 16:24 ` [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
2013-03-26 13:35 ` Rafael J. Wysocki
3 siblings, 0 replies; 6+ messages in thread
From: Danny Baumann @ 2013-03-19 16:22 UTC (permalink / raw)
To: linux-acpi; +Cc: linux-kernel, Zhang Rui, Len Brown, Danny Baumann
The value initially read via _BQC also needs to be offset by 2 to
compensate for the first 2 special items in _BCL. Introduce a helper
function that does the BQC-value-to-level conversion in order to not
needlessly duplicate code.
Signed-off-by: Danny Baumann <dannybaumann@web.de>
---
drivers/acpi/video.c | 60 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 37 insertions(+), 23 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 89bf61e..86beaa8 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -450,6 +450,31 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
{}
};
+static unsigned long long
+acpi_video_bqc_value_to_level(struct acpi_video_device *device,
+ unsigned long long bqc_value)
+{
+ unsigned long long level;
+
+ if (device->brightness->flags._BQC_use_index) {
+ /*
+ * _BQC returns an index that doesn't account for
+ * the first 2 items with special meaning, so we need
+ * to compensate for that by offsetting ourselves
+ */
+ if (device->brightness->flags._BCL_reversed)
+ bqc_value = device->brightness->count - 3 - bqc_value;
+
+ level = device->brightness->levels[bqc_value + 2];
+ } else {
+ level = bqc_value;
+ }
+
+ level += bqc_offset_aml_bug_workaround;
+
+ return level;
+}
+
static int
acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
unsigned long long *level, bool raw)
@@ -472,14 +497,8 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
return 0;
}
- if (device->brightness->flags._BQC_use_index) {
- if (device->brightness->flags._BCL_reversed)
- *level = device->brightness->count
- - 3 - (*level);
- *level = device->brightness->levels[*level + 2];
+ *level = acpi_video_bqc_value_to_level(device, *level);
- }
- *level += bqc_offset_aml_bug_workaround;
for (i = 2; i < device->brightness->count; i++)
if (device->brightness->levels[i] == *level) {
device->brightness->curr = *level;
@@ -728,24 +747,19 @@ acpi_video_init_brightness(struct acpi_video_device *device)
br->flags._BQC_use_index = (level == max_level ? 0 : 1);
- if (!br->flags._BQC_use_index) {
+ if (use_bios_initial_backlight) {
+ level = acpi_video_bqc_value_to_level(device, level_old);
/*
- * Set the backlight to the initial state.
- * On some buggy laptops, _BQC returns an uninitialized value
- * when invoked for the first time, i.e. level_old is invalid.
- * set the backlight to max_level in this case
+ * On some buggy laptops, _BQC returns an uninitialized
+ * value when invoked for the first time, i.e.
+ * level_old is invalid (no matter whether it's a level
+ * or an index). Set the backlight to max_level in this case.
*/
- if (use_bios_initial_backlight) {
- for (i = 2; i < br->count; i++)
- if (level_old == br->levels[i]) {
- level = level_old;
- break;
- }
- }
- } else {
- if (br->flags._BCL_reversed)
- level_old = (br->count - 1) - level_old;
- level = br->levels[level_old];
+ for (i = 2; i < br->count; i++)
+ if (level_old == br->levels[i])
+ break;
+ if (i == br->count)
+ level = max_level;
}
set_level:
--
1.8.1.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops.
2013-03-19 16:22 [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
2013-03-19 16:22 ` [PATCH v4 2/3] ACPI video: Make logic a little easier to understand Danny Baumann
2013-03-19 16:22 ` [PATCH v4 3/3] ACPI video: Fix applying indexed initial brightness value Danny Baumann
@ 2013-03-19 16:24 ` Danny Baumann
2013-03-20 7:32 ` Aaron Lu
2013-03-26 13:35 ` Rafael J. Wysocki
3 siblings, 1 reply; 6+ messages in thread
From: Danny Baumann @ 2013-03-19 16:24 UTC (permalink / raw)
To: Danny Baumann; +Cc: linux-acpi, linux-kernel, Zhang Rui, Len Brown
v3 incorporates Aaron's feedback, v4 fixes my inability to use git
send-email properly. Sorry for that.
Regards,
Danny
Am 19.03.2013 17:22, schrieb Danny Baumann:
> In particular, this fixes brightness control initialization for all
> devices that return index values from _BQC and don't happen to have the
> initial index set by the BIOS in their _BCL table. One example for that
> is the Dell Inspiron 15R SE (model number 7520).
>
> What happened for those devices is that acpi_init_brightness queried the
> initial brightness by calling acpi_video_device_lcd_get_level_current.
> This called _BQC, which returned e.g. 13. As _BQC_use_index isn't
> determined at this point (and thus has its initial value of 0), the
> index isn't converted into the actual level. As '13' isn't present in
> the _BCL list, *level is later overwritten with brightness->curr, which
> was initialized to max_level (100) before. Later in
> acpi_init_brightness, level_old (with the value 100) is used as an index
> into the _BCL table, which causes a value outside of the actual table to
> be used as input into acpi_video_device_lcd_set_level(). Depending on
> the (undefined) value of that location, this call will fail, causing the
> brightness control for the device in question not to be enabled.
>
> Fix that by returning the raw value returned by the _BQC call in the
> initialization case.
>
> Signed-off-by: Danny Baumann <dannybaumann@web.de>
> ---
> drivers/acpi/video.c | 43 ++++++++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 313f959..09e4722 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
> int level);
> static int acpi_video_device_lcd_get_level_current(
> struct acpi_video_device *device,
> - unsigned long long *level, int init);
> + unsigned long long *level, bool raw);
> static int acpi_video_get_next_level(struct acpi_video_device *device,
> u32 level_current, u32 event);
> static int acpi_video_switch_brightness(struct acpi_video_device *device,
> @@ -236,7 +236,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
> struct acpi_video_device *vd =
> (struct acpi_video_device *)bl_get_data(bd);
>
> - if (acpi_video_device_lcd_get_level_current(vd, &cur_level, 0))
> + if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
> return -EINVAL;
> for (i = 2; i < vd->brightness->count; i++) {
> if (vd->brightness->levels[i] == cur_level)
> @@ -281,7 +281,7 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
> unsigned long long level;
> int offset;
>
> - if (acpi_video_device_lcd_get_level_current(video, &level, 0))
> + if (acpi_video_device_lcd_get_level_current(video, &level, false))
> return -EINVAL;
> for (offset = 2; offset < video->brightness->count; offset++)
> if (level == video->brightness->levels[offset]) {
> @@ -452,7 +452,7 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>
> static int
> acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> - unsigned long long *level, int init)
> + unsigned long long *level, bool raw)
> {
> acpi_status status = AE_OK;
> int i;
> @@ -463,6 +463,15 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> status = acpi_evaluate_integer(device->dev->handle, buf,
> NULL, level);
> if (ACPI_SUCCESS(status)) {
> + if (raw) {
> + /*
> + * Caller has indicated he wants the raw
> + * value returned by _BQC, so don't furtherly
> + * mess with the value.
> + */
> + return 0;
> + }
> +
> if (device->brightness->flags._BQC_use_index) {
> if (device->brightness->flags._BCL_reversed)
> *level = device->brightness->count
> @@ -476,16 +485,14 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> device->brightness->curr = *level;
> return 0;
> }
> - if (!init) {
> - /*
> - * BQC returned an invalid level.
> - * Stop using it.
> - */
> - ACPI_WARNING((AE_INFO,
> - "%s returned an invalid level",
> - buf));
> - device->cap._BQC = device->cap._BCQ = 0;
> - }
> + /*
> + * BQC returned an invalid level.
> + * Stop using it.
> + */
> + ACPI_WARNING((AE_INFO,
> + "%s returned an invalid level",
> + buf));
> + device->cap._BQC = device->cap._BCQ = 0;
> } else {
> /* Fixme:
> * should we return an error or ignore this failure?
> @@ -703,7 +710,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
> if (!device->cap._BQC)
> goto set_level;
>
> - result = acpi_video_device_lcd_get_level_current(device, &level_old, 1);
> + result = acpi_video_device_lcd_get_level_current(device,
> + &level_old, true);
> if (result)
> goto out_free_levels;
>
> @@ -714,7 +722,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
> if (result)
> goto out_free_levels;
>
> - result = acpi_video_device_lcd_get_level_current(device, &level, 0);
> + result = acpi_video_device_lcd_get_level_current(device, &level, true);
> if (result)
> goto out_free_levels;
>
> @@ -1268,7 +1276,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
> goto out;
>
> result = acpi_video_device_lcd_get_level_current(device,
> - &level_current, 0);
> + &level_current,
> + false);
> if (result)
> goto out;
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops.
2013-03-19 16:24 ` [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
@ 2013-03-20 7:32 ` Aaron Lu
0 siblings, 0 replies; 6+ messages in thread
From: Aaron Lu @ 2013-03-20 7:32 UTC (permalink / raw)
To: Danny Baumann; +Cc: linux-acpi, linux-kernel, Zhang Rui, Len Brown
On 03/20/2013 12:24 AM, Danny Baumann wrote:
> v3 incorporates Aaron's feedback, v4 fixes my inability to use git
> send-email properly. Sorry for that.
For the 3 patches, Reviewed-by: Aaron Lu <aaron.lu@intel.com>
BTW, you can put things like what has been changed in a new series in
patch 0.
Thanks,
Aaron
>
> Regards,
>
> Danny
>
> Am 19.03.2013 17:22, schrieb Danny Baumann:
>> In particular, this fixes brightness control initialization for all
>> devices that return index values from _BQC and don't happen to have the
>> initial index set by the BIOS in their _BCL table. One example for that
>> is the Dell Inspiron 15R SE (model number 7520).
>>
>> What happened for those devices is that acpi_init_brightness queried the
>> initial brightness by calling acpi_video_device_lcd_get_level_current.
>> This called _BQC, which returned e.g. 13. As _BQC_use_index isn't
>> determined at this point (and thus has its initial value of 0), the
>> index isn't converted into the actual level. As '13' isn't present in
>> the _BCL list, *level is later overwritten with brightness->curr, which
>> was initialized to max_level (100) before. Later in
>> acpi_init_brightness, level_old (with the value 100) is used as an index
>> into the _BCL table, which causes a value outside of the actual table to
>> be used as input into acpi_video_device_lcd_set_level(). Depending on
>> the (undefined) value of that location, this call will fail, causing the
>> brightness control for the device in question not to be enabled.
>>
>> Fix that by returning the raw value returned by the _BQC call in the
>> initialization case.
>>
>> Signed-off-by: Danny Baumann <dannybaumann@web.de>
>> ---
>> drivers/acpi/video.c | 43 ++++++++++++++++++++++++++-----------------
>> 1 file changed, 26 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index 313f959..09e4722 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
>> int level);
>> static int acpi_video_device_lcd_get_level_current(
>> struct acpi_video_device *device,
>> - unsigned long long *level, int init);
>> + unsigned long long *level, bool raw);
>> static int acpi_video_get_next_level(struct acpi_video_device *device,
>> u32 level_current, u32 event);
>> static int acpi_video_switch_brightness(struct acpi_video_device *device,
>> @@ -236,7 +236,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
>> struct acpi_video_device *vd =
>> (struct acpi_video_device *)bl_get_data(bd);
>>
>> - if (acpi_video_device_lcd_get_level_current(vd, &cur_level, 0))
>> + if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
>> return -EINVAL;
>> for (i = 2; i < vd->brightness->count; i++) {
>> if (vd->brightness->levels[i] == cur_level)
>> @@ -281,7 +281,7 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
>> unsigned long long level;
>> int offset;
>>
>> - if (acpi_video_device_lcd_get_level_current(video, &level, 0))
>> + if (acpi_video_device_lcd_get_level_current(video, &level, false))
>> return -EINVAL;
>> for (offset = 2; offset < video->brightness->count; offset++)
>> if (level == video->brightness->levels[offset]) {
>> @@ -452,7 +452,7 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>>
>> static int
>> acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>> - unsigned long long *level, int init)
>> + unsigned long long *level, bool raw)
>> {
>> acpi_status status = AE_OK;
>> int i;
>> @@ -463,6 +463,15 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>> status = acpi_evaluate_integer(device->dev->handle, buf,
>> NULL, level);
>> if (ACPI_SUCCESS(status)) {
>> + if (raw) {
>> + /*
>> + * Caller has indicated he wants the raw
>> + * value returned by _BQC, so don't furtherly
>> + * mess with the value.
>> + */
>> + return 0;
>> + }
>> +
>> if (device->brightness->flags._BQC_use_index) {
>> if (device->brightness->flags._BCL_reversed)
>> *level = device->brightness->count
>> @@ -476,16 +485,14 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>> device->brightness->curr = *level;
>> return 0;
>> }
>> - if (!init) {
>> - /*
>> - * BQC returned an invalid level.
>> - * Stop using it.
>> - */
>> - ACPI_WARNING((AE_INFO,
>> - "%s returned an invalid level",
>> - buf));
>> - device->cap._BQC = device->cap._BCQ = 0;
>> - }
>> + /*
>> + * BQC returned an invalid level.
>> + * Stop using it.
>> + */
>> + ACPI_WARNING((AE_INFO,
>> + "%s returned an invalid level",
>> + buf));
>> + device->cap._BQC = device->cap._BCQ = 0;
>> } else {
>> /* Fixme:
>> * should we return an error or ignore this failure?
>> @@ -703,7 +710,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>> if (!device->cap._BQC)
>> goto set_level;
>>
>> - result = acpi_video_device_lcd_get_level_current(device, &level_old, 1);
>> + result = acpi_video_device_lcd_get_level_current(device,
>> + &level_old, true);
>> if (result)
>> goto out_free_levels;
>>
>> @@ -714,7 +722,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
>> if (result)
>> goto out_free_levels;
>>
>> - result = acpi_video_device_lcd_get_level_current(device, &level, 0);
>> + result = acpi_video_device_lcd_get_level_current(device, &level, true);
>> if (result)
>> goto out_free_levels;
>>
>> @@ -1268,7 +1276,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
>> goto out;
>>
>> result = acpi_video_device_lcd_get_level_current(device,
>> - &level_current, 0);
>> + &level_current,
>> + false);
>> if (result)
>> goto out;
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops.
2013-03-19 16:22 [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
` (2 preceding siblings ...)
2013-03-19 16:24 ` [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
@ 2013-03-26 13:35 ` Rafael J. Wysocki
3 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2013-03-26 13:35 UTC (permalink / raw)
To: Danny Baumann; +Cc: linux-acpi, linux-kernel, Zhang Rui, Len Brown
On Tuesday, March 19, 2013 05:22:50 PM Danny Baumann wrote:
> In particular, this fixes brightness control initialization for all
> devices that return index values from _BQC and don't happen to have the
> initial index set by the BIOS in their _BCL table. One example for that
> is the Dell Inspiron 15R SE (model number 7520).
>
> What happened for those devices is that acpi_init_brightness queried the
> initial brightness by calling acpi_video_device_lcd_get_level_current.
> This called _BQC, which returned e.g. 13. As _BQC_use_index isn't
> determined at this point (and thus has its initial value of 0), the
> index isn't converted into the actual level. As '13' isn't present in
> the _BCL list, *level is later overwritten with brightness->curr, which
> was initialized to max_level (100) before. Later in
> acpi_init_brightness, level_old (with the value 100) is used as an index
> into the _BCL table, which causes a value outside of the actual table to
> be used as input into acpi_video_device_lcd_set_level(). Depending on
> the (undefined) value of that location, this call will fail, causing the
> brightness control for the device in question not to be enabled.
>
> Fix that by returning the raw value returned by the _BQC call in the
> initialization case.
>
> Signed-off-by: Danny Baumann <dannybaumann@web.de>
All patches in the series applied to linux-pm.git/linux-next.
Thanks,
Rafael
> ---
> drivers/acpi/video.c | 43 ++++++++++++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 313f959..09e4722 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -222,7 +222,7 @@ static int acpi_video_device_lcd_set_level(struct acpi_video_device *device,
> int level);
> static int acpi_video_device_lcd_get_level_current(
> struct acpi_video_device *device,
> - unsigned long long *level, int init);
> + unsigned long long *level, bool raw);
> static int acpi_video_get_next_level(struct acpi_video_device *device,
> u32 level_current, u32 event);
> static int acpi_video_switch_brightness(struct acpi_video_device *device,
> @@ -236,7 +236,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
> struct acpi_video_device *vd =
> (struct acpi_video_device *)bl_get_data(bd);
>
> - if (acpi_video_device_lcd_get_level_current(vd, &cur_level, 0))
> + if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
> return -EINVAL;
> for (i = 2; i < vd->brightness->count; i++) {
> if (vd->brightness->levels[i] == cur_level)
> @@ -281,7 +281,7 @@ static int video_get_cur_state(struct thermal_cooling_device *cooling_dev, unsig
> unsigned long long level;
> int offset;
>
> - if (acpi_video_device_lcd_get_level_current(video, &level, 0))
> + if (acpi_video_device_lcd_get_level_current(video, &level, false))
> return -EINVAL;
> for (offset = 2; offset < video->brightness->count; offset++)
> if (level == video->brightness->levels[offset]) {
> @@ -452,7 +452,7 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
>
> static int
> acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> - unsigned long long *level, int init)
> + unsigned long long *level, bool raw)
> {
> acpi_status status = AE_OK;
> int i;
> @@ -463,6 +463,15 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> status = acpi_evaluate_integer(device->dev->handle, buf,
> NULL, level);
> if (ACPI_SUCCESS(status)) {
> + if (raw) {
> + /*
> + * Caller has indicated he wants the raw
> + * value returned by _BQC, so don't furtherly
> + * mess with the value.
> + */
> + return 0;
> + }
> +
> if (device->brightness->flags._BQC_use_index) {
> if (device->brightness->flags._BCL_reversed)
> *level = device->brightness->count
> @@ -476,16 +485,14 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
> device->brightness->curr = *level;
> return 0;
> }
> - if (!init) {
> - /*
> - * BQC returned an invalid level.
> - * Stop using it.
> - */
> - ACPI_WARNING((AE_INFO,
> - "%s returned an invalid level",
> - buf));
> - device->cap._BQC = device->cap._BCQ = 0;
> - }
> + /*
> + * BQC returned an invalid level.
> + * Stop using it.
> + */
> + ACPI_WARNING((AE_INFO,
> + "%s returned an invalid level",
> + buf));
> + device->cap._BQC = device->cap._BCQ = 0;
> } else {
> /* Fixme:
> * should we return an error or ignore this failure?
> @@ -703,7 +710,8 @@ acpi_video_init_brightness(struct acpi_video_device *device)
> if (!device->cap._BQC)
> goto set_level;
>
> - result = acpi_video_device_lcd_get_level_current(device, &level_old, 1);
> + result = acpi_video_device_lcd_get_level_current(device,
> + &level_old, true);
> if (result)
> goto out_free_levels;
>
> @@ -714,7 +722,7 @@ acpi_video_init_brightness(struct acpi_video_device *device)
> if (result)
> goto out_free_levels;
>
> - result = acpi_video_device_lcd_get_level_current(device, &level, 0);
> + result = acpi_video_device_lcd_get_level_current(device, &level, true);
> if (result)
> goto out_free_levels;
>
> @@ -1268,7 +1276,8 @@ acpi_video_switch_brightness(struct acpi_video_device *device, int event)
> goto out;
>
> result = acpi_video_device_lcd_get_level_current(device,
> - &level_current, 0);
> + &level_current,
> + false);
> if (result)
> goto out;
>
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-03-26 13:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-19 16:22 [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
2013-03-19 16:22 ` [PATCH v4 2/3] ACPI video: Make logic a little easier to understand Danny Baumann
2013-03-19 16:22 ` [PATCH v4 3/3] ACPI video: Fix applying indexed initial brightness value Danny Baumann
2013-03-19 16:24 ` [PATCH v4 1/3] ACPI video: Fix brightness control initialization for some laptops Danny Baumann
2013-03-20 7:32 ` Aaron Lu
2013-03-26 13:35 ` Rafael J. Wysocki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.