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

[Patrick Mochel <mochel@linux.intel.com>]

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