[patch] complete get and set lcd patch

David Zeuthen david at fubar.dk
Wed Aug 31 16:03:48 PDT 2005


On Wed, 2005-08-31 at 19:17 +0100, Richard Hughes wrote:
> Here's the latest complete patch as requested. Please review.
> 

Looks pretty good. Thinking about it we should call it LaptopPanel
instead of LCDPanel.. since that is exactly what it is. LCDPanel might
mean an external monitor connected via VGA or DVI.

Some other comments:

+static gboolean
+lcdpanel_refresh (HalDevice *d, ACPIDevHandler *handler)
+{

We need to test that the 'brightness' file.. or whatever it's called on
various systems actually exist.. And bail out (refuse to add the hal
device object) if it's missing. 

+	snprintf (path, sizeof (path), "%s/acpi/ibm", get_hal_proc_path ());
+	if (g_file_test (path, G_FILE_TEST_EXISTS))
+		acpi_synthesize_item (path, ACPI_TYPE_IBM_DISPLAY);

Maybe this should be "%s/acpi/ibm/brightness" instead? That would give
the check we need. Right?

+++ fdi/policy/10osvendor/16-lcdpanel-mgmt-policy.fdi	2005-08-31 18:31:15.000000000 +0100

Why 16? Why not just 10? :-)

+++ tools/hal-system-lcd-get-brightness	2005-08-31 19:00:35.000000000 +0100
+++ tools/hal-system-lcd-set-brightness	2005-08-31 18:36:11.000000000 +0100

Both of these need to check that the brightness file is actually there
and throw an exception otherwise (the .Unsupported exception is fine)

So, if you can change this and send the patch again I think we're
getting there!

Thanks,
David




More information about the hal mailing list