On Wed, 2009-03-11 at 20:56 +0800, Thomas Renninger wrote:
> On Tuesday 10 March 2009 09:03:25 Zhang Rui wrote:
> > Subject: check the return value of
> acpi_video_device_lcd_get_level_current
> > From: Zhang Rui <rui.zhang@xxxxxxxxx>
> >
> > check the return value of acpi_video_device_lcd_get_level_current
> >
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> > drivers/acpi/video.c | 69
> ++++++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 47 insertions(+), 22 deletions(-)
> >
> > Index: linux-2.6/drivers/acpi/video.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/acpi/video.c
> > +++ linux-2.6/drivers/acpi/video.c
> > @@ -502,11 +505,29 @@ static int
> > acpi_video_device_lcd_get_level_current(struct acpi_video_device
> *device,
> > unsigned long long *level)
> > {
> > - if (device->cap._BQC)
> > - return acpi_evaluate_integer(device->dev->handle, "_BQC", NULL,
> > - level);
> > + acpi_status status = AE_OK;
> > +
> > + if (device->cap._BQC) {
> > + status = acpi_evaluate_integer(device->dev->handle, "_BQC",
> > + NULL, level);
> > + if (ACPI_SUCCESS(status)) {
> > + device->brightness->curr = *level;
> > + return 0;
> > + } else {
> > + /* Fixme:
> > + * should we return an error or ignore this failure?
> > + * dev->brightness->curr is a cached value which stores
> > + * the correct current backlight level in most cases.
> > + * ACPI video backlight still works w/ buggy _BQC.
> > + * http://bugzilla.kernel.org/show_bug.cgi?id=12233
> > + */
> I wonder what should go wrong there at all.
> We should add a warning (using printk(.. FW_BUG...)) if we do ACPI video
> brightness switching without _BQC. This should be supported, but is a
> firmware bug and likely fails.
I think we should do it in video_detect.c
a simple patch like this:
---
drivers/acpi/video_detect.c | 2 ++
1 file changed, 2 insertions(+)
Index: linux-2.6/drivers/acpi/video_detect.c
===================================================================
--- linux-2.6.orig/drivers/acpi/video_detect.c
+++ linux-2.6/drivers/acpi/video_detect.c
@@ -55,6 +55,8 @@ acpi_backlight_cap_match(acpi_handle han
ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found generic backlight "
"support\n"));
*cap |= ACPI_VIDEO_BACKLIGHT;
+ if (ACPI_FAILURE(acpi_get_handle(handle, "_BQC", &h_dummy)))
+ printk(KERN_WARNING FW_BUG "No _BQC available\n");
/* We have backlight support, no need to scan further */
return AE_CTRL_TERMINATE;
}
But the ACPI warning here is still needed, because the _BQC method is
implemented but there are some errors when evaluating it.
> (BTW, I recently saw a BIOS with _BCQ function. They said they are going to
> fix it, but it may be more widespread, e.g. this also is often the case
> (missing _BQC) on Samsung). I found it by luck disassembling and
> recompiling the DSDT, a runtime warning would be nice (if it does not
> already exist).
Interesting, if this is tree, we can add the support for _BCQ in a
separate patch.
BTW: could you please attach the acpidump of that box please because I
never noticed this problem before. :)
> Anyway, if we have _BQC (cap._BQC), evaluation should not fail?
> Ok, double checking is always a good idea...
>
No, evaluating _BQC may return fail, like the one in bug 12233,
Method (_BQC, 0, NotSerialized)
{
And (\_SB.PCI0.LPCB.FJEX.GBLS, 0xFF, Local0)
Return (DerefOf (Index (BCLP, Add (Local0, 0x02))))
}
but \_SB.PCI0.LPCB.FJEX.GBLS doesn't exist at all...
> > + ACPI_WARNING((AE_INFO, "Evaluating _BQC failed"));
> > + device->cap._BQC = 0;
> > + }
> > + }
> > +
> > *level = device->brightness->curr;
> > - return AE_OK;
> > + return 0;
> > }
> >
> > static int
> > @@ -773,18 +794,8 @@ static void acpi_video_device_find_cap(s
> > device->backlight = backlight_device_register(name,
> > NULL, device, &acpi_backlight_ops);
> > device->backlight->props.max_brightness = device->brightness->count-3;
> > - /*
> > - * If there exists the _BQC object, the _BQC object will be
> > - * called to get the current backlight brightness. Otherwise
> > - * the brightness will be set to the maximum.
> > - */
> > - if (device->cap._BQC)
> > - device->backlight->props.brightness =
> > - acpi_video_get_brightness(device->backlight);
> > - else
> > - device->backlight->props.brightness =
> > - device->backlight->props.max_brightness;
> > - backlight_update_status(device->backlight);
> > + device->backlight->props.brightness =
> > + acpi_video_get_brightness(device->backlight);
> I could imagine this introduces a regression on machines without _BQC.
> IIRC the value which brightness is currently active must be initialized if
> there is no _BQC function and that seems to happen here.
>
right, this should probably be done in patch 5.
where we have set the backlight in acpi_video_init_brightness already.
thanks,
rui
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
