Patchwork [4/4] ACPI: video - cleanup video device registration

login
register
mail settings
Submitter Dmitry Torokhov
Date Aug. 10, 2009, 5:51 a.m.
Message ID <20090810062216.C9A68526EC9@mailhub.coreip.homeip.net>
Download mbox | patch
Permalink /patch/40348/
State RFC, archived
Headers show

Comments

Dmitry Torokhov - Aug. 10, 2009, 5:51 a.m.
On Mon, Aug 10, 2009 at 10:11:05AM +0800, Zhang Rui wrote:
> On Sat, 2009-08-08 at 15:26 +0800, Dmitry Torokhov wrote:
> > +       if (acpi_video_backlight_support()) {
> > +               acpi_video_init_brightness(data);
> > +               acpi_video_device_add_backlight(data);
> > +               acpi_video_device_add_cooling_dev(data);
> 
> we should check the return value of acpi_video_init_brightness first.
> No backlight device or thermal cooling device if the video device
> doesn't support backlight control well.
> 

Ok, then what about the patch below then (on top of #4)?
Zhang Rui - Aug. 10, 2009, 6:21 a.m.
On Mon, 2009-08-10 at 13:51 +0800, Dmitry Torokhov wrote:
> On Mon, Aug 10, 2009 at 10:11:05AM +0800, Zhang Rui wrote:
> > On Sat, 2009-08-08 at 15:26 +0800, Dmitry Torokhov wrote:
> > > +       if (acpi_video_backlight_support()) {
> > > +               acpi_video_init_brightness(data);
> > > +               acpi_video_device_add_backlight(data);
> > > +               acpi_video_device_add_cooling_dev(data);
> > 
> > we should check the return value of acpi_video_init_brightness first.
> > No backlight device or thermal cooling device if the video device
> > doesn't support backlight control well.
> > 
> 
> Ok, then what about the patch below then (on top of #4)?
> 
the patch looks good. Thanks, Dmitry.

Acked-by: Zhang Rui <rui.zhang@intel.com>

> -- 
> Dmitry
> 
> 
> ACPI: video - do not register backlight/cooling without brightness
> 
> Do not attempt registering backlight or cooling device when initialization
> of brightness support failed - they are not useable without it.
> Also don't use 2 kzallocs when one will suffice.
> 
> Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
> ---
> 
>  drivers/acpi/video.c |  101 +++++++++++++++++++++++---------------------------
>  1 files changed, 46 insertions(+), 55 deletions(-)
> 
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index bc5082b..51b8004 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -184,9 +184,9 @@ struct acpi_video_brightness_flags {
>  
>  struct acpi_video_device_brightness {
>  	int curr;
> -	int count;
> -	int *levels;
>  	struct acpi_video_brightness_flags flags;
> +	int count;
> +	int levels[0];
>  };
>  
>  struct acpi_video_device {
> @@ -923,32 +923,27 @@ static int acpi_video_init_brightness(struct acpi_video_device *device)
>  	int result = -EINVAL;
>  
>  	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
> -		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
> -						"LCD brightness level\n"));
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> +			"Could not query available LCD brightness levels\n"));
>  		goto out;
>  	}
>  
>  	if (obj->package.count < 2)
>  		goto out;
>  
> -	br = kzalloc(sizeof(*br), GFP_KERNEL);
> +	br = kzalloc(sizeof(*br) +
> +		     (obj->package.count + 2) * sizeof(br->levels[0]),
> +		     GFP_KERNEL);
>  	if (!br) {
> -		printk(KERN_ERR "can't allocate memory\n");
> +		printk(KERN_ERR PREFIX "can't allocate memory\n");
>  		result = -ENOMEM;
>  		goto out;
>  	}
>  
> -	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
> -				GFP_KERNEL);
> -	if (!br->levels) {
> -		result = -ENOMEM;
> -		goto out_free;
> -	}
> -
>  	for (i = 0; i < obj->package.count; i++) {
>  		o = (union acpi_object *)&obj->package.elements[i];
>  		if (o->type != ACPI_TYPE_INTEGER) {
> -			printk(KERN_ERR PREFIX "Invalid data\n");
> +			printk(KERN_ERR PREFIX "Invalid brightness data\n");
>  			continue;
>  		}
>  		br->levels[count] = (u32) o->integer.value;
> @@ -1005,60 +1000,55 @@ static int acpi_video_init_brightness(struct acpi_video_device *device)
>  	/* _BQC uses INDEX while _BCL uses VALUE in some laptops */
>  	br->curr = level_old = max_level;
>  
> -	if (!device->cap._BQC)
> -		goto set_level;
> -
> -	result = acpi_video_device_lcd_get_level_current(device, &level_old);
> -	if (result)
> -		goto out_free_levels;
> +	if (device->cap._BQC) {
> +		result = acpi_video_device_lcd_get_level_current(device,
> +								 &level_old);
> +		if (result)
> +			goto out;
>  
> -	/*
> -	 * Set the level to maximum and check if _BQC uses indexed value
> -	 */
> -	result = acpi_video_device_lcd_set_level(device, max_level);
> -	if (result)
> -		goto out_free_levels;
> -
> -	result = acpi_video_device_lcd_get_level_current(device, &level);
> -	if (result)
> -		goto out_free_levels;
> +		/*
> +		 * Set the level to maximum and check if _BQC uses
> +		 * indexed value
> +		 */
> +		result = acpi_video_device_lcd_set_level(device, max_level);
> +		if (result)
> +			goto out;
>  
> -	br->flags._BQC_use_index = (level == max_level ? 0 : 1);
> +		result = acpi_video_device_lcd_get_level_current(device,
> +								 &level);
> +		if (result)
> +			goto out;
>  
> -	if (!br->flags._BQC_use_index)
> -		goto set_level;
> +		br->flags._BQC_use_index = (level == max_level ? 0 : 1);
>  
> -	if (br->flags._BCL_reversed)
> -		level_old = (br->count - 1) - level_old;
> -	level_old = br->levels[level_old];
> +		if (br->flags._BQC_use_index) {
> +			if (br->flags._BCL_reversed)
> +				level_old = (br->count - 1) - level_old;
> +			level_old = br->levels[level_old];
> +		}
> +	}
>  
> -set_level:
>  	result = acpi_video_device_lcd_set_level(device, level_old);
>  	if (result)
> -		goto out_free_levels;
> +		goto out;
>  
>  	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			  "found %d brightness levels\n", count - 2));
> -	kfree(obj);
> -	return result;
>  
> -out_free_levels:
> -	kfree(br->levels);
> -out_free:
> -	kfree(br);
>  out:
> -	device->brightness = NULL;
> +	if (result) {
> +		kfree(br);
> +		device->brightness = NULL;
> +	}
> +
>  	kfree(obj);
>  	return result;
>  }
>  
>  static void acpi_video_free_brightness_data(struct acpi_video_device *device)
>  {
> -	if (device->brightness) {
> -		kfree(device->brightness->levels);
> -		kfree(device->brightness);
> -		device->brightness = NULL;
> -	}
> +	kfree(device->brightness);
> +	device->brightness = NULL;
>  }
>  
>  /*
> @@ -1066,7 +1056,7 @@ static void acpi_video_free_brightness_data(struct acpi_video_device *device)
>   *	device	: video output device (LCD, CRT, ..)
>   *
>   *  Return Value:
> - *  	None
> + *	None
>   *
>   *  Find out all required AML methods defined under the output
>   *  device.
> @@ -1824,9 +1814,10 @@ static int acpi_video_bus_add_device(struct acpi_device *device,
>  	acpi_video_device_find_cap(data);
>  
>  	if (acpi_video_backlight_support()) {
> -		acpi_video_init_brightness(data);
> -		acpi_video_device_add_backlight(data);
> -		acpi_video_device_add_cooling_dev(data);
> +		if (acpi_video_init_brightness(data) == 0) {
> +			acpi_video_device_add_backlight(data);
> +			acpi_video_device_add_cooling_dev(data);
> +		}
>  	}
>  
>  	if (acpi_video_display_switch_support())
> @@ -2259,7 +2250,7 @@ static int acpi_video_bus_add(struct acpi_device *device)
>  	if (!strcmp(device->pnp.bus_id, "VID")) {
>  		if (instance)
>  			device->pnp.bus_id[3] = '0' + instance;
> -		instance ++;
> +		instance++;
>  	}
>  	/* a hack to fix the duplicate name "VGA" problem on Pa 3553 */
>  	if (!strcmp(device->pnp.bus_id, "VGA")) {

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index bc5082b..51b8004 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -184,9 +184,9 @@  struct acpi_video_brightness_flags {
 
 struct acpi_video_device_brightness {
 	int curr;
-	int count;
-	int *levels;
 	struct acpi_video_brightness_flags flags;
+	int count;
+	int levels[0];
 };
 
 struct acpi_video_device {
@@ -923,32 +923,27 @@  static int acpi_video_init_brightness(struct acpi_video_device *device)
 	int result = -EINVAL;
 
 	if (!ACPI_SUCCESS(acpi_video_device_lcd_query_levels(device, &obj))) {
-		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Could not query available "
-						"LCD brightness level\n"));
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
+			"Could not query available LCD brightness levels\n"));
 		goto out;
 	}
 
 	if (obj->package.count < 2)
 		goto out;
 
-	br = kzalloc(sizeof(*br), GFP_KERNEL);
+	br = kzalloc(sizeof(*br) +
+		     (obj->package.count + 2) * sizeof(br->levels[0]),
+		     GFP_KERNEL);
 	if (!br) {
-		printk(KERN_ERR "can't allocate memory\n");
+		printk(KERN_ERR PREFIX "can't allocate memory\n");
 		result = -ENOMEM;
 		goto out;
 	}
 
-	br->levels = kmalloc((obj->package.count + 2) * sizeof *(br->levels),
-				GFP_KERNEL);
-	if (!br->levels) {
-		result = -ENOMEM;
-		goto out_free;
-	}
-
 	for (i = 0; i < obj->package.count; i++) {
 		o = (union acpi_object *)&obj->package.elements[i];
 		if (o->type != ACPI_TYPE_INTEGER) {
-			printk(KERN_ERR PREFIX "Invalid data\n");
+			printk(KERN_ERR PREFIX "Invalid brightness data\n");
 			continue;
 		}
 		br->levels[count] = (u32) o->integer.value;
@@ -1005,60 +1000,55 @@  static int acpi_video_init_brightness(struct acpi_video_device *device)
 	/* _BQC uses INDEX while _BCL uses VALUE in some laptops */
 	br->curr = level_old = max_level;
 
-	if (!device->cap._BQC)
-		goto set_level;
-
-	result = acpi_video_device_lcd_get_level_current(device, &level_old);
-	if (result)
-		goto out_free_levels;
+	if (device->cap._BQC) {
+		result = acpi_video_device_lcd_get_level_current(device,
+								 &level_old);
+		if (result)
+			goto out;
 
-	/*
-	 * Set the level to maximum and check if _BQC uses indexed value
-	 */
-	result = acpi_video_device_lcd_set_level(device, max_level);
-	if (result)
-		goto out_free_levels;
-
-	result = acpi_video_device_lcd_get_level_current(device, &level);
-	if (result)
-		goto out_free_levels;
+		/*
+		 * Set the level to maximum and check if _BQC uses
+		 * indexed value
+		 */
+		result = acpi_video_device_lcd_set_level(device, max_level);
+		if (result)
+			goto out;
 
-	br->flags._BQC_use_index = (level == max_level ? 0 : 1);
+		result = acpi_video_device_lcd_get_level_current(device,
+								 &level);
+		if (result)
+			goto out;
 
-	if (!br->flags._BQC_use_index)
-		goto set_level;
+		br->flags._BQC_use_index = (level == max_level ? 0 : 1);
 
-	if (br->flags._BCL_reversed)
-		level_old = (br->count - 1) - level_old;
-	level_old = br->levels[level_old];
+		if (br->flags._BQC_use_index) {
+			if (br->flags._BCL_reversed)
+				level_old = (br->count - 1) - level_old;
+			level_old = br->levels[level_old];
+		}
+	}
 
-set_level:
 	result = acpi_video_device_lcd_set_level(device, level_old);
 	if (result)
-		goto out_free_levels;
+		goto out;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			  "found %d brightness levels\n", count - 2));
-	kfree(obj);
-	return result;
 
-out_free_levels:
-	kfree(br->levels);
-out_free:
-	kfree(br);
 out:
-	device->brightness = NULL;
+	if (result) {
+		kfree(br);
+		device->brightness = NULL;
+	}
+
 	kfree(obj);
 	return result;
 }
 
 static void acpi_video_free_brightness_data(struct acpi_video_device *device)
 {
-	if (device->brightness) {
-		kfree(device->brightness->levels);
-		kfree(device->brightness);
-		device->brightness = NULL;
-	}
+	kfree(device->brightness);
+	device->brightness = NULL;
 }
 
 /*
@@ -1066,7 +1056,7 @@  static void acpi_video_free_brightness_data(struct acpi_video_device *device)
  *	device	: video output device (LCD, CRT, ..)
  *
  *  Return Value:
- *  	None
+ *	None
  *
  *  Find out all required AML methods defined under the output
  *  device.
@@ -1824,9 +1814,10 @@  static int acpi_video_bus_add_device(struct acpi_device *device,
 	acpi_video_device_find_cap(data);
 
 	if (acpi_video_backlight_support()) {
-		acpi_video_init_brightness(data);
-		acpi_video_device_add_backlight(data);
-		acpi_video_device_add_cooling_dev(data);
+		if (acpi_video_init_brightness(data) == 0) {
+			acpi_video_device_add_backlight(data);
+			acpi_video_device_add_cooling_dev(data);
+		}
 	}
 
 	if (acpi_video_display_switch_support())
@@ -2259,7 +2250,7 @@  static int acpi_video_bus_add(struct acpi_device *device)
 	if (!strcmp(device->pnp.bus_id, "VID")) {
 		if (instance)
 			device->pnp.bus_id[3] = '0' + instance;
-		instance ++;
+		instance++;
 	}
 	/* a hack to fix the duplicate name "VGA" problem on Pa 3553 */
 	if (!strcmp(device->pnp.bus_id, "VGA")) {