On Tue, Apr 18, 2006 at 09:29:52AM +0100, Matthew Garrett wrote:
> This allows Asus backlight control to be performed via
> /sys/class/backlight. It does not currently remove the legacy /proc
> interface.
Neat, though a few questions that apply to each of the three ACPI platform drivers..
> diff -urp drivers/acpi.bak/asus_acpi.c drivers/acpi/asus_acpi.c
> +static struct backlight_device *asus_backlight_device;
> +
Why is this dynamically allocated? If there is only one per driver, then the
register() function could take that as a parameter -- instead of passing various
variable to initialize it with -- and return an error.
> -static int read_brightness(void)
> +static int read_brightness(struct backlight_device *bd)
It seems that you're always passing NULL to this. And, if you're not passing NULL,
aren't you always referencing the single global instance above?
> -static void set_brightness(int value)
> +static int set_brightness(struct backlight_device *bd, int value)
> {
> acpi_status status = 0;
>
> + value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
> + /* 0 <= value <= 15 */
Is there something wrong with:
if (value < 0)
value = 0;
else if (value > 15)
value = 15;
? Or, could you just make the parameter an unsigned int and just keep the 2nd check?
> - value = (0 < value) ? ((15 < value) ? 15 : value) : 0;
> - /* 0 <= value <= 15 */
> - set_brightness(value);
> + set_brightness(NULL,value);
There should be a space between parameters.
> + asus_backlight_device = backlight_device_register ("asus_bl", NULL,
> + &asusbl_data);
> +
> + if (IS_ERR (asus_backlight_device)) {
> + printk("Unable to register backlight\n");
Could you print the name of the driver?
> + acpi_bus_unregister_driver(&asus_hotk_driver);
> + remove_proc_entry(PROC_ASUS, acpi_root_dir);
If the backlight fails to register, does it mean that these must also fail?
Thanks,
Patrick
-
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
