Re: PATCH [1/3]: Provide generic backlight support in Asus ACPI driver

Patrick Mochel
2006-04-18T16:14:35Z

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