[PATCH 0/3] acpi: video: trivial cleanups

[PATCH 0/3] acpi: video: trivial cleanups

[Felipe Contreras]

Hi,

Here's a few patches to cleanup the code-style of acpi/video.c. Some were found
by checkpatch, and some just make sense.

Felipe Contreras (3):
  acpi: video: trivial costmetic cleanups
  acpi: video: trivial style cleanups
  acpi: video: remove unnecessary casting

 drivers/acpi/video.c | 210 +++++++++++++++++++++++++--------------------------
 1 file changed, 103 insertions(+), 107 deletions(-)

-- 
1.8.3.4

[PATCH 1/3] acpi: video: trivial costmetic cleanups

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/video.c | 114 +++++++++++++++++++++++++++------------------------
 1 file changed, 61 insertions(+), 53 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 0ec434d..f021bf4 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1,5 +1,5 @@
 /*
- *  video.c - ACPI Video Driver ($Revision:$)
+ *  video.c - ACPI Video Driver
  *
  *  Copyright (C) 2004 Luming Yu <luming.yu@intel.com>
  *  Copyright (C) 2004 Bruno Ducrot <ducrot@poupinou.org>
@@ -118,26 +118,26 @@ struct acpi_video_bus_flags {
 };
 
 struct acpi_video_bus_cap {
-	u8 _DOS:1;		/*Enable/Disable output switching */
-	u8 _DOD:1;		/*Enumerate all devices attached to display adapter */
-	u8 _ROM:1;		/*Get ROM Data */
-	u8 _GPD:1;		/*Get POST Device */
-	u8 _SPD:1;		/*Set POST Device */
-	u8 _VPO:1;		/*Video POST Options */
+	u8 _DOS:1;		/* Enable/Disable output switching */
+	u8 _DOD:1;		/* Enumerate all devices attached to display adapter */
+	u8 _ROM:1;		/* Get ROM Data */
+	u8 _GPD:1;		/* Get POST Device */
+	u8 _SPD:1;		/* Set POST Device */
+	u8 _VPO:1;		/* Video POST Options */
 	u8 reserved:2;
 };
 
 struct acpi_video_device_attrib {
 	u32 display_index:4;	/* A zero-based instance of the Display */
-	u32 display_port_attachment:4;	/*This field differentiates the display type */
-	u32 display_type:4;	/*Describe the specific type in use */
-	u32 vendor_specific:4;	/*Chipset Vendor Specific */
-	u32 bios_can_detect:1;	/*BIOS can detect the device */
-	u32 depend_on_vga:1;	/*Non-VGA output device whose power is related to 
+	u32 display_port_attachment:4;	/* This field differentiates the display type */
+	u32 display_type:4;	/* Describe the specific type in use */
+	u32 vendor_specific:4;	/* Chipset Vendor Specific */
+	u32 bios_can_detect:1;	/* BIOS can detect the device */
+	u32 depend_on_vga:1;	/* Non-VGA output device whose power is related to
 				   the VGA device. */
-	u32 pipe_id:3;		/*For VGA multiple-head devices. */
-	u32 reserved:10;	/*Must be 0 */
-	u32 device_id_scheme:1;	/*Device ID Scheme */
+	u32 pipe_id:3;		/* For VGA multiple-head devices. */
+	u32 reserved:10;	/* Must be 0 */
+	u32 device_id_scheme:1;	/* Device ID Scheme */
 };
 
 struct acpi_video_enumerated_device {
@@ -174,17 +174,17 @@ struct acpi_video_device_flags {
 };
 
 struct acpi_video_device_cap {
-	u8 _ADR:1;		/*Return the unique ID */
-	u8 _BCL:1;		/*Query list of brightness control levels supported */
-	u8 _BCM:1;		/*Set the brightness level */
+	u8 _ADR:1;		/* Return the unique ID */
+	u8 _BCL:1;		/* Query list of brightness control levels supported */
+	u8 _BCM:1;		/* Set the brightness level */
 	u8 _BQC:1;		/* Get current brightness level */
 	u8 _BCQ:1;		/* Some buggy BIOS uses _BCQ instead of _BQC */
-	u8 _DDC:1;		/*Return the EDID for this device */
+	u8 _DDC:1;		/* Return the EDID for this device */
 };
 
 struct acpi_video_brightness_flags {
 	u8 _BCL_no_ac_battery_levels:1;	/* no AC/Battery levels in _BCL */
-	u8 _BCL_reversed:1;		/* _BCL package is in a reversed order*/
+	u8 _BCL_reversed:1;		/* _BCL package is in a reversed order */
 	u8 _BCL_use_index:1;		/* levels in _BCL are index values */
 	u8 _BCM_use_index:1;		/* input of _BCM is an index value */
 	u8 _BQC_use_index:1;		/* _BQC returns an index value */
@@ -231,7 +231,7 @@ static int acpi_video_get_next_level(struct acpi_video_device *device,
 static int acpi_video_switch_brightness(struct acpi_video_device *device,
 					 int event);
 
-/*backlight device sysfs support*/
+/* backlight device sysfs support */
 static int acpi_video_get_brightness(struct backlight_device *bd)
 {
 	unsigned long long cur_level;
@@ -243,8 +243,10 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 		return -EINVAL;
 	for (i = 2; i < vd->brightness->count; i++) {
 		if (vd->brightness->levels[i] == cur_level)
-			/* The first two entries are special - see page 575
-			   of the ACPI spec 3.0 */
+			/*
+			 * The first two entries are special - see page 575
+			 * of the ACPI spec 3.0
+			 */
 			return i-2;
 	}
 	return 0;
@@ -316,9 +318,11 @@ static const struct thermal_cooling_device_ops video_cooling_ops = {
 	.set_cur_state = video_set_cur_state,
 };
 
-/* --------------------------------------------------------------------------
-                               Video Management
-   -------------------------------------------------------------------------- */
+/*
+ * --------------------------------------------------------------------------
+ *                             Video Management
+ * --------------------------------------------------------------------------
+ */
 
 static int
 acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
@@ -556,7 +560,8 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 				      buf));
 			device->cap._BQC = device->cap._BCQ = 0;
 		} else {
-			/* Fixme:
+			/*
+			 * Fixme:
 			 * should we return an error or ignore this failure?
 			 * dev->brightness->curr is a cached value which stores
 			 * the correct current backlight level in most cases.
@@ -615,8 +620,8 @@ acpi_video_device_EDID(struct acpi_video_device *device,
 
 /*
  *  Arg:
- *  	video		: video bus device pointer
- *	bios_flag	: 
+ *	video		: video bus device pointer
+ *	bios_flag	:
  *		0.	The system BIOS should NOT automatically switch(toggle)
  *			the active display output.
  *		1.	The system BIOS should automatically switch (toggle) the
@@ -628,9 +633,9 @@ acpi_video_device_EDID(struct acpi_video_device *device,
  *	lcd_flag	:
  *		0.	The system BIOS should automatically control the brightness level
  *			of the LCD when the power changes from AC to DC
- *		1. 	The system BIOS should NOT automatically control the brightness 
+ *		1.	The system BIOS should NOT automatically control the brightness
  *			level of the LCD when the power changes from AC to DC.
- * Return Value:
+ *  Return Value:
  *		-EINVAL	wrong arg.
  */
 
@@ -717,8 +722,8 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
 
 
 /*
- *  Arg:	
- *  	device	: video output device (LCD, CRT, ..)
+ *  Arg:
+ *	device	: video output device (LCD, CRT, ..)
  *
  *  Return Value:
  *	Maximum brightness level
@@ -877,7 +882,7 @@ out:
  *	device	: video output device (LCD, CRT, ..)
  *
  *  Return Value:
- *  	None
+ *	None
  *
  *  Find out all required AML methods defined under the output
  *  device.
@@ -988,11 +993,11 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 }
 
 /*
- *  Arg:	
- *  	device	: video output device (VGA)
+ *  Arg:
+ *	device	: video output device (VGA)
  *
  *  Return Value:
- *  	None
+ *	None
  *
  *  Find out all required AML methods defined under the video bus device.
  */
@@ -1039,7 +1044,8 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
 		return -ENODEV;
 	pci_dev_put(dev);
 
-	/* Since there is no HID, CID and so on for VGA driver, we have
+	/*
+	 * Since there is no HID, CID and so on for VGA driver, we have
 	 * to check well known required nodes.
 	 */
 
@@ -1069,9 +1075,11 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
 	return status;
 }
 
-/* --------------------------------------------------------------------------
-                                 Driver Interface
-   -------------------------------------------------------------------------- */
+/*
+ * --------------------------------------------------------------------------
+ *                               Driver Interface
+ * --------------------------------------------------------------------------
+ */
 
 /* device interface */
 static struct acpi_video_device_attrib*
@@ -1192,12 +1200,12 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 
 /*
  *  Arg:
- *  	video	: video bus device 
+ *	video	: video bus device
  *
  *  Return:
- *  	none
- *  
- *  Enumerate the video device list of the video bus, 
+ *	none
+ *
+ *  Enumerate the video device list of the video bus,
  *  bind the ids with the corresponding video devices
  *  under the video bus.
  */
@@ -1216,13 +1224,13 @@ static void acpi_video_device_rebind(struct acpi_video_bus *video)
 
 /*
  *  Arg:
- *  	video	: video bus device 
- *  	device	: video output device under the video 
- *  		bus
+ *	video	: video bus device
+ *	device	: video output device under the video
+ *		bus
  *
  *  Return:
- *  	none
- *  
+ *	none
+ *
  *  Bind the ids with the corresponding video devices
  *  under the video bus.
  */
@@ -1245,11 +1253,11 @@ acpi_video_device_bind(struct acpi_video_bus *video,
 
 /*
  *  Arg:
- *  	video	: video bus device 
+ *	video	: video bus device
  *
  *  Return:
- *  	< 0	: error
- *  
+ *	< 0	: error
+ *
  *  Call _DOD to enumerate all devices attached to display adapter
  *
  */
-- 
1.8.3.4

[PATCH 2/3] acpi: video: trivial style cleanups

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/video.c | 90 +++++++++++++++++++++++-----------------------------
 1 file changed, 40 insertions(+), 50 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index f021bf4..b07573f 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644);
 static bool use_bios_initial_backlight = 1;
 module_param(use_bios_initial_backlight, bool, 0644);
 
-static int register_count = 0;
+static int register_count;
 static int acpi_video_bus_add(struct acpi_device *device);
 static int acpi_video_bus_remove(struct acpi_device *device);
 static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
@@ -247,7 +247,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 			 * The first two entries are special - see page 575
 			 * of the ACPI spec 3.0
 			 */
-			return i-2;
+			return i - 2;
 	}
 	return 0;
 }
@@ -304,11 +304,11 @@ video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long st
 	struct acpi_video_device *video = acpi_driver_data(device);
 	int level;
 
-	if ( state >= video->brightness->count - 2)
+	if (state >= video->brightness->count - 2)
 		return -EINVAL;
 
 	state = video->brightness->count - state;
-	level = video->brightness->levels[state -1];
+	level = video->brightness->levels[state - 1];
 	return acpi_video_device_lcd_set_level(video, level);
 }
 
@@ -349,7 +349,7 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
 
 	return 0;
 
-      err:
+err:
 	kfree(buffer.pointer);
 
 	return status;
@@ -550,7 +550,7 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
 				if (device->brightness->levels[i] == *level) {
 					device->brightness->curr = *level;
 					return 0;
-			}
+				}
 			/*
 			 * BQC returned an invalid level.
 			 * Stop using it.
@@ -892,16 +892,13 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 {
 	acpi_handle h_dummy1;
 
-	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR", &h_dummy1)))
 		device->cap._ADR = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL", &h_dummy1)))
 		device->cap._BCL = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM", &h_dummy1)))
 		device->cap._BCM = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle,"_BQC",&h_dummy1)))
+	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BQC", &h_dummy1)))
 		device->cap._BQC = 1;
 	else if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCQ",
 				&h_dummy1))) {
@@ -909,9 +906,8 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 		device->cap._BCQ = 1;
 	}
 
-	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC", &h_dummy1)))
 		device->cap._DDC = 1;
-	}
 
 	if (acpi_video_init_brightness(device))
 		return;
@@ -922,7 +918,7 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 		acpi_handle acpi_parent;
 		struct device *parent = NULL;
 		int result;
-		static int count = 0;
+		static int count;
 		char *name;
 
 		name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
@@ -1006,24 +1002,18 @@ static void acpi_video_bus_find_cap(struct acpi_video_bus *video)
 {
 	acpi_handle h_dummy1;
 
-	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS", &h_dummy1)))
 		video->cap._DOS = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOD", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOD", &h_dummy1)))
 		video->cap._DOD = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_ROM", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_ROM", &h_dummy1)))
 		video->cap._ROM = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_GPD", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_GPD", &h_dummy1)))
 		video->cap._GPD = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_SPD", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_SPD", &h_dummy1)))
 		video->cap._SPD = 1;
-	}
-	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_VPO", &h_dummy1))) {
+	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_VPO", &h_dummy1)))
 		video->cap._VPO = 1;
-	}
 }
 
 /*
@@ -1082,7 +1072,7 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
  */
 
 /* device interface */
-static struct acpi_video_device_attrib*
+static struct acpi_video_device_attrib *
 acpi_video_get_device_attr(struct acpi_video_bus *video, unsigned long device_id)
 {
 	struct acpi_video_enumerated_device *ids;
@@ -1120,7 +1110,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 	unsigned long long device_id;
 	int status, device_type;
 	struct acpi_video_device *data;
-	struct acpi_video_device_attrib* attribute;
+	struct acpi_video_device_attrib *attribute;
 
 	status =
 	    acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
@@ -1142,7 +1132,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 
 	attribute = acpi_video_get_device_attr(video, device_id);
 
-	if((attribute != NULL) && attribute->device_id_scheme) {
+	if (attribute && attribute->device_id_scheme) {
 		switch (attribute->display_type) {
 		case ACPI_VIDEO_DISPLAY_CRT:
 			data->flags.crt = 1;
@@ -1160,24 +1150,24 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
 			data->flags.unknown = 1;
 			break;
 		}
-		if(attribute->bios_can_detect)
+		if (attribute->bios_can_detect)
 			data->flags.bios = 1;
 	} else {
 		/* Check for legacy IDs */
 		device_type = acpi_video_get_device_type(video, device_id);
 		/* Ignore bits 16 and 18-20 */
 		switch (device_type & 0xffe2ffff) {
-			case ACPI_VIDEO_DISPLAY_LEGACY_MONITOR:
-				data->flags.crt = 1;
-				break;
-			case ACPI_VIDEO_DISPLAY_LEGACY_PANEL:
-				data->flags.lcd = 1;
-				break;
-			case ACPI_VIDEO_DISPLAY_LEGACY_TV:
-				data->flags.tvout = 1;
-				break;
-			default:
-				data->flags.unknown = 1;
+		case ACPI_VIDEO_DISPLAY_LEGACY_MONITOR:
+			data->flags.crt = 1;
+			break;
+		case ACPI_VIDEO_DISPLAY_LEGACY_PANEL:
+			data->flags.lcd = 1;
+			break;
+		case ACPI_VIDEO_DISPLAY_LEGACY_TV:
+			data->flags.tvout = 1;
+			break;
+		default:
+			data->flags.unknown = 1;
 		}
 	}
 
@@ -1318,7 +1308,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
 	video->attached_array = active_list;
 	video->attached_count = count;
 
- out:
+out:
 	kfree(buffer.pointer);
 	return status;
 }
@@ -1773,7 +1763,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")) {
@@ -1845,16 +1835,16 @@ static int acpi_video_bus_add(struct acpi_device *device)
 
 	return 0;
 
- err_unregister_pm_notifier:
+err_unregister_pm_notifier:
 	unregister_pm_notifier(&video->pm_nb);
- err_stop_video:
+err_stop_video:
 	acpi_video_bus_stop_devices(video);
- err_free_input_dev:
+err_free_input_dev:
 	input_free_device(input);
- err_put_video:
+err_put_video:
 	acpi_video_bus_put_devices(video);
 	kfree(video->attached_array);
- err_free_video:
+err_free_video:
 	kfree(video);
 	device->driver_data = NULL;
 
-- 
1.8.3.4

[PATCH 3/3] acpi: video: remove unnecessary casting

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 drivers/acpi/video.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index b07573f..11bafbe 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -236,8 +236,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 {
 	unsigned long long cur_level;
 	int i;
-	struct acpi_video_device *vd =
-		(struct acpi_video_device *)bl_get_data(bd);
+	struct acpi_video_device *vd = bl_get_data(bd);
 
 	if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
 		return -EINVAL;
@@ -255,8 +254,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
 static int acpi_video_set_brightness(struct backlight_device *bd)
 {
 	int request_level = bd->props.brightness + 2;
-	struct acpi_video_device *vd =
-		(struct acpi_video_device *)bl_get_data(bd);
+	struct acpi_video_device *vd = bl_get_data(bd);
 
 	return acpi_video_device_lcd_set_level(vd,
 				vd->brightness->levels[request_level]);
-- 
1.8.3.4

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Aaron Lu]

On 08/02/2013 07:43 AM, Felipe Contreras wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Please add change log explaining what you have changed.
It seems that the patch modify comment style only, some add a space and
some change spaces to tab, is it the case?

Thanks,
Aaron

> ---
>  drivers/acpi/video.c | 114 +++++++++++++++++++++++++++------------------------
>  1 file changed, 61 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index 0ec434d..f021bf4 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -1,5 +1,5 @@
>  /*
> - *  video.c - ACPI Video Driver ($Revision:$)
> + *  video.c - ACPI Video Driver
>   *
>   *  Copyright (C) 2004 Luming Yu <luming.yu@intel.com>
>   *  Copyright (C) 2004 Bruno Ducrot <ducrot@poupinou.org>
> @@ -118,26 +118,26 @@ struct acpi_video_bus_flags {
>  };
>  
>  struct acpi_video_bus_cap {
> -	u8 _DOS:1;		/*Enable/Disable output switching */
> -	u8 _DOD:1;		/*Enumerate all devices attached to display adapter */
> -	u8 _ROM:1;		/*Get ROM Data */
> -	u8 _GPD:1;		/*Get POST Device */
> -	u8 _SPD:1;		/*Set POST Device */
> -	u8 _VPO:1;		/*Video POST Options */
> +	u8 _DOS:1;		/* Enable/Disable output switching */
> +	u8 _DOD:1;		/* Enumerate all devices attached to display adapter */
> +	u8 _ROM:1;		/* Get ROM Data */
> +	u8 _GPD:1;		/* Get POST Device */
> +	u8 _SPD:1;		/* Set POST Device */
> +	u8 _VPO:1;		/* Video POST Options */
>  	u8 reserved:2;
>  };
>  
>  struct acpi_video_device_attrib {
>  	u32 display_index:4;	/* A zero-based instance of the Display */
> -	u32 display_port_attachment:4;	/*This field differentiates the display type */
> -	u32 display_type:4;	/*Describe the specific type in use */
> -	u32 vendor_specific:4;	/*Chipset Vendor Specific */
> -	u32 bios_can_detect:1;	/*BIOS can detect the device */
> -	u32 depend_on_vga:1;	/*Non-VGA output device whose power is related to 
> +	u32 display_port_attachment:4;	/* This field differentiates the display type */
> +	u32 display_type:4;	/* Describe the specific type in use */
> +	u32 vendor_specific:4;	/* Chipset Vendor Specific */
> +	u32 bios_can_detect:1;	/* BIOS can detect the device */
> +	u32 depend_on_vga:1;	/* Non-VGA output device whose power is related to
>  				   the VGA device. */
> -	u32 pipe_id:3;		/*For VGA multiple-head devices. */
> -	u32 reserved:10;	/*Must be 0 */
> -	u32 device_id_scheme:1;	/*Device ID Scheme */
> +	u32 pipe_id:3;		/* For VGA multiple-head devices. */
> +	u32 reserved:10;	/* Must be 0 */
> +	u32 device_id_scheme:1;	/* Device ID Scheme */
>  };
>  
>  struct acpi_video_enumerated_device {
> @@ -174,17 +174,17 @@ struct acpi_video_device_flags {
>  };
>  
>  struct acpi_video_device_cap {
> -	u8 _ADR:1;		/*Return the unique ID */
> -	u8 _BCL:1;		/*Query list of brightness control levels supported */
> -	u8 _BCM:1;		/*Set the brightness level */
> +	u8 _ADR:1;		/* Return the unique ID */
> +	u8 _BCL:1;		/* Query list of brightness control levels supported */
> +	u8 _BCM:1;		/* Set the brightness level */
>  	u8 _BQC:1;		/* Get current brightness level */
>  	u8 _BCQ:1;		/* Some buggy BIOS uses _BCQ instead of _BQC */
> -	u8 _DDC:1;		/*Return the EDID for this device */
> +	u8 _DDC:1;		/* Return the EDID for this device */
>  };
>  
>  struct acpi_video_brightness_flags {
>  	u8 _BCL_no_ac_battery_levels:1;	/* no AC/Battery levels in _BCL */
> -	u8 _BCL_reversed:1;		/* _BCL package is in a reversed order*/
> +	u8 _BCL_reversed:1;		/* _BCL package is in a reversed order */
>  	u8 _BCL_use_index:1;		/* levels in _BCL are index values */
>  	u8 _BCM_use_index:1;		/* input of _BCM is an index value */
>  	u8 _BQC_use_index:1;		/* _BQC returns an index value */
> @@ -231,7 +231,7 @@ static int acpi_video_get_next_level(struct acpi_video_device *device,
>  static int acpi_video_switch_brightness(struct acpi_video_device *device,
>  					 int event);
>  
> -/*backlight device sysfs support*/
> +/* backlight device sysfs support */
>  static int acpi_video_get_brightness(struct backlight_device *bd)
>  {
>  	unsigned long long cur_level;
> @@ -243,8 +243,10 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
>  		return -EINVAL;
>  	for (i = 2; i < vd->brightness->count; i++) {
>  		if (vd->brightness->levels[i] == cur_level)
> -			/* The first two entries are special - see page 575
> -			   of the ACPI spec 3.0 */
> +			/*
> +			 * The first two entries are special - see page 575
> +			 * of the ACPI spec 3.0
> +			 */
>  			return i-2;
>  	}
>  	return 0;
> @@ -316,9 +318,11 @@ static const struct thermal_cooling_device_ops video_cooling_ops = {
>  	.set_cur_state = video_set_cur_state,
>  };
>  
> -/* --------------------------------------------------------------------------
> -                               Video Management
> -   -------------------------------------------------------------------------- */
> +/*
> + * --------------------------------------------------------------------------
> + *                             Video Management
> + * --------------------------------------------------------------------------
> + */
>  
>  static int
>  acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
> @@ -556,7 +560,8 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>  				      buf));
>  			device->cap._BQC = device->cap._BCQ = 0;
>  		} else {
> -			/* Fixme:
> +			/*
> +			 * Fixme:
>  			 * should we return an error or ignore this failure?
>  			 * dev->brightness->curr is a cached value which stores
>  			 * the correct current backlight level in most cases.
> @@ -615,8 +620,8 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>  
>  /*
>   *  Arg:
> - *  	video		: video bus device pointer
> - *	bios_flag	: 
> + *	video		: video bus device pointer
> + *	bios_flag	:
>   *		0.	The system BIOS should NOT automatically switch(toggle)
>   *			the active display output.
>   *		1.	The system BIOS should automatically switch (toggle) the
> @@ -628,9 +633,9 @@ acpi_video_device_EDID(struct acpi_video_device *device,
>   *	lcd_flag	:
>   *		0.	The system BIOS should automatically control the brightness level
>   *			of the LCD when the power changes from AC to DC
> - *		1. 	The system BIOS should NOT automatically control the brightness 
> + *		1.	The system BIOS should NOT automatically control the brightness
>   *			level of the LCD when the power changes from AC to DC.
> - * Return Value:
> + *  Return Value:
>   *		-EINVAL	wrong arg.
>   */
>  
> @@ -717,8 +722,8 @@ static int acpi_video_bqc_quirk(struct acpi_video_device *device,
>  
>  
>  /*
> - *  Arg:	
> - *  	device	: video output device (LCD, CRT, ..)
> + *  Arg:
> + *	device	: video output device (LCD, CRT, ..)
>   *
>   *  Return Value:
>   *	Maximum brightness level
> @@ -877,7 +882,7 @@ out:
>   *	device	: video output device (LCD, CRT, ..)
>   *
>   *  Return Value:
> - *  	None
> + *	None
>   *
>   *  Find out all required AML methods defined under the output
>   *  device.
> @@ -988,11 +993,11 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
>  }
>  
>  /*
> - *  Arg:	
> - *  	device	: video output device (VGA)
> + *  Arg:
> + *	device	: video output device (VGA)
>   *
>   *  Return Value:
> - *  	None
> + *	None
>   *
>   *  Find out all required AML methods defined under the video bus device.
>   */
> @@ -1039,7 +1044,8 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
>  		return -ENODEV;
>  	pci_dev_put(dev);
>  
> -	/* Since there is no HID, CID and so on for VGA driver, we have
> +	/*
> +	 * Since there is no HID, CID and so on for VGA driver, we have
>  	 * to check well known required nodes.
>  	 */
>  
> @@ -1069,9 +1075,11 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
>  	return status;
>  }
>  
> -/* --------------------------------------------------------------------------
> -                                 Driver Interface
> -   -------------------------------------------------------------------------- */
> +/*
> + * --------------------------------------------------------------------------
> + *                               Driver Interface
> + * --------------------------------------------------------------------------
> + */
>  
>  /* device interface */
>  static struct acpi_video_device_attrib*
> @@ -1192,12 +1200,12 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
>  
>  /*
>   *  Arg:
> - *  	video	: video bus device 
> + *	video	: video bus device
>   *
>   *  Return:
> - *  	none
> - *  
> - *  Enumerate the video device list of the video bus, 
> + *	none
> + *
> + *  Enumerate the video device list of the video bus,
>   *  bind the ids with the corresponding video devices
>   *  under the video bus.
>   */
> @@ -1216,13 +1224,13 @@ static void acpi_video_device_rebind(struct acpi_video_bus *video)
>  
>  /*
>   *  Arg:
> - *  	video	: video bus device 
> - *  	device	: video output device under the video 
> - *  		bus
> + *	video	: video bus device
> + *	device	: video output device under the video
> + *		bus
>   *
>   *  Return:
> - *  	none
> - *  
> + *	none
> + *
>   *  Bind the ids with the corresponding video devices
>   *  under the video bus.
>   */
> @@ -1245,11 +1253,11 @@ acpi_video_device_bind(struct acpi_video_bus *video,
>  
>  /*
>   *  Arg:
> - *  	video	: video bus device 
> + *	video	: video bus device
>   *
>   *  Return:
> - *  	< 0	: error
> - *  
> + *	< 0	: error
> + *
>   *  Call _DOD to enumerate all devices attached to display adapter
>   *
>   */
> 

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Aaron Lu]

On 08/02/2013 07:44 AM, Felipe Contreras wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Change log please.

> ---
>  drivers/acpi/video.c | 90 +++++++++++++++++++++++-----------------------------
>  1 file changed, 40 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index f021bf4..b07573f 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644);
>  static bool use_bios_initial_backlight = 1;
>  module_param(use_bios_initial_backlight, bool, 0644);
>  
> -static int register_count = 0;
> +static int register_count;

This isn't a style clean up.

>  static int acpi_video_bus_add(struct acpi_device *device);
>  static int acpi_video_bus_remove(struct acpi_device *device);
>  static void acpi_video_bus_notify(struct acpi_device *device, u32 event);
> @@ -247,7 +247,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
>  			 * The first two entries are special - see page 575
>  			 * of the ACPI spec 3.0
>  			 */
> -			return i-2;
> +			return i - 2;
>  	}
>  	return 0;
>  }
> @@ -304,11 +304,11 @@ video_set_cur_state(struct thermal_cooling_device *cooling_dev, unsigned long st
>  	struct acpi_video_device *video = acpi_driver_data(device);
>  	int level;
>  
> -	if ( state >= video->brightness->count - 2)
> +	if (state >= video->brightness->count - 2)
>  		return -EINVAL;
>  
>  	state = video->brightness->count - state;
> -	level = video->brightness->levels[state -1];
> +	level = video->brightness->levels[state - 1];
>  	return acpi_video_device_lcd_set_level(video, level);
>  }
>  
> @@ -349,7 +349,7 @@ acpi_video_device_lcd_query_levels(struct acpi_video_device *device,
>  
>  	return 0;
>  
> -      err:
> +err:
>  	kfree(buffer.pointer);
>  
>  	return status;
> @@ -550,7 +550,7 @@ acpi_video_device_lcd_get_level_current(struct acpi_video_device *device,
>  				if (device->brightness->levels[i] == *level) {
>  					device->brightness->curr = *level;
>  					return 0;
> -			}
> +				}
>  			/*
>  			 * BQC returned an invalid level.
>  			 * Stop using it.
> @@ -892,16 +892,13 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
>  {
>  	acpi_handle h_dummy1;
>  
> -	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_ADR", &h_dummy1)))
>  		device->cap._ADR = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCL", &h_dummy1)))
>  		device->cap._BCL = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCM", &h_dummy1)))
>  		device->cap._BCM = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle,"_BQC",&h_dummy1)))
> +	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BQC", &h_dummy1)))
>  		device->cap._BQC = 1;
>  	else if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_BCQ",
>  				&h_dummy1))) {
> @@ -909,9 +906,8 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
>  		device->cap._BCQ = 1;
>  	}
>  
> -	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(device->dev->handle, "_DDC", &h_dummy1)))
>  		device->cap._DDC = 1;
> -	}
>  
>  	if (acpi_video_init_brightness(device))
>  		return;
> @@ -922,7 +918,7 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
>  		acpi_handle acpi_parent;
>  		struct device *parent = NULL;
>  		int result;
> -		static int count = 0;
> +		static int count;

Same here.

>  		char *name;
>  
>  		name = kasprintf(GFP_KERNEL, "acpi_video%d", count);
> @@ -1006,24 +1002,18 @@ static void acpi_video_bus_find_cap(struct acpi_video_bus *video)
>  {
>  	acpi_handle h_dummy1;
>  
> -	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOS", &h_dummy1)))
>  		video->cap._DOS = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOD", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_DOD", &h_dummy1)))
>  		video->cap._DOD = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_ROM", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_ROM", &h_dummy1)))
>  		video->cap._ROM = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_GPD", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_GPD", &h_dummy1)))
>  		video->cap._GPD = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_SPD", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_SPD", &h_dummy1)))
>  		video->cap._SPD = 1;
> -	}
> -	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_VPO", &h_dummy1))) {
> +	if (ACPI_SUCCESS(acpi_get_handle(video->device->handle, "_VPO", &h_dummy1)))
>  		video->cap._VPO = 1;
> -	}
>  }
>  
>  /*
> @@ -1082,7 +1072,7 @@ static int acpi_video_bus_check(struct acpi_video_bus *video)
>   */
>  
>  /* device interface */
> -static struct acpi_video_device_attrib*
> +static struct acpi_video_device_attrib *
>  acpi_video_get_device_attr(struct acpi_video_bus *video, unsigned long device_id)
>  {
>  	struct acpi_video_enumerated_device *ids;
> @@ -1120,7 +1110,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
>  	unsigned long long device_id;
>  	int status, device_type;
>  	struct acpi_video_device *data;
> -	struct acpi_video_device_attrib* attribute;
> +	struct acpi_video_device_attrib *attribute;
>  
>  	status =
>  	    acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
> @@ -1142,7 +1132,7 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
>  
>  	attribute = acpi_video_get_device_attr(video, device_id);
>  
> -	if((attribute != NULL) && attribute->device_id_scheme) {
> +	if (attribute && attribute->device_id_scheme) {
>  		switch (attribute->display_type) {
>  		case ACPI_VIDEO_DISPLAY_CRT:
>  			data->flags.crt = 1;
> @@ -1160,24 +1150,24 @@ acpi_video_bus_get_one_device(struct acpi_device *device,
>  			data->flags.unknown = 1;
>  			break;
>  		}
> -		if(attribute->bios_can_detect)
> +		if (attribute->bios_can_detect)
>  			data->flags.bios = 1;
>  	} else {
>  		/* Check for legacy IDs */
>  		device_type = acpi_video_get_device_type(video, device_id);
>  		/* Ignore bits 16 and 18-20 */
>  		switch (device_type & 0xffe2ffff) {
> -			case ACPI_VIDEO_DISPLAY_LEGACY_MONITOR:
> -				data->flags.crt = 1;
> -				break;
> -			case ACPI_VIDEO_DISPLAY_LEGACY_PANEL:
> -				data->flags.lcd = 1;
> -				break;
> -			case ACPI_VIDEO_DISPLAY_LEGACY_TV:
> -				data->flags.tvout = 1;
> -				break;
> -			default:
> -				data->flags.unknown = 1;
> +		case ACPI_VIDEO_DISPLAY_LEGACY_MONITOR:
> +			data->flags.crt = 1;
> +			break;
> +		case ACPI_VIDEO_DISPLAY_LEGACY_PANEL:
> +			data->flags.lcd = 1;
> +			break;
> +		case ACPI_VIDEO_DISPLAY_LEGACY_TV:
> +			data->flags.tvout = 1;
> +			break;
> +		default:
> +			data->flags.unknown = 1;
>  		}
>  	}
>  
> @@ -1318,7 +1308,7 @@ static int acpi_video_device_enumerate(struct acpi_video_bus *video)
>  	video->attached_array = active_list;
>  	video->attached_count = count;
>  
> - out:
> +out:
>  	kfree(buffer.pointer);
>  	return status;
>  }
> @@ -1773,7 +1763,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")) {
> @@ -1845,16 +1835,16 @@ static int acpi_video_bus_add(struct acpi_device *device)
>  
>  	return 0;
>  
> - err_unregister_pm_notifier:
> +err_unregister_pm_notifier:
>  	unregister_pm_notifier(&video->pm_nb);
> - err_stop_video:
> +err_stop_video:
>  	acpi_video_bus_stop_devices(video);
> - err_free_input_dev:
> +err_free_input_dev:
>  	input_free_device(input);
> - err_put_video:
> +err_put_video:
>  	acpi_video_bus_put_devices(video);
>  	kfree(video->attached_array);
> - err_free_video:
> +err_free_video:
>  	kfree(video);
>  	device->driver_data = NULL;
>  
> 
Thanks,
Aaron

Re: [PATCH 3/3] acpi: video: remove unnecessary casting

[Aaron Lu]

On 08/02/2013 07:44 AM, Felipe Contreras wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Looks good to me, except that no change log feels a little weird.

Reviewed-by: Aaron Lu <aaron.lu@intel.com>

Thanks,
Aaron

> ---
>  drivers/acpi/video.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index b07573f..11bafbe 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -236,8 +236,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
>  {
>  	unsigned long long cur_level;
>  	int i;
> -	struct acpi_video_device *vd =
> -		(struct acpi_video_device *)bl_get_data(bd);
> +	struct acpi_video_device *vd = bl_get_data(bd);
>  
>  	if (acpi_video_device_lcd_get_level_current(vd, &cur_level, false))
>  		return -EINVAL;
> @@ -255,8 +254,7 @@ static int acpi_video_get_brightness(struct backlight_device *bd)
>  static int acpi_video_set_brightness(struct backlight_device *bd)
>  {
>  	int request_level = bd->props.brightness + 2;
> -	struct acpi_video_device *vd =
> -		(struct acpi_video_device *)bl_get_data(bd);
> +	struct acpi_video_device *vd = bl_get_data(bd);
>  
>  	return acpi_video_device_lcd_set_level(vd,
>  				vd->brightness->levels[request_level]);
> 

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Felipe Contreras]

On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> On 08/02/2013 07:43 AM, Felipe Contreras wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> Please add change log explaining what you have changed.
> It seems that the patch modify comment style only, some add a space and
> some change spaces to tab, is it the case?

The commit message already explains what the change is; trivial
cosmetic cleanups. Cosmetic means it's completely superficial.

-- 
Felipe Contreras

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Felipe Contreras]

On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> On 08/02/2013 07:44 AM, Felipe Contreras wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> Change log please.

You mean a commit message? That's what it's called in Git lingo, and
it's right there:

acpi: video: trivial style cleanups

>> ---
>>  drivers/acpi/video.c | 90 +++++++++++++++++++++++-----------------------------
>>  1 file changed, 40 insertions(+), 50 deletions(-)
>>
>> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
>> index f021bf4..b07573f 100644
>> --- a/drivers/acpi/video.c
>> +++ b/drivers/acpi/video.c
>> @@ -88,7 +88,7 @@ module_param(allow_duplicates, bool, 0644);
>>  static bool use_bios_initial_backlight = 1;
>>  module_param(use_bios_initial_backlight, bool, 0644);
>>
>> -static int register_count = 0;
>> +static int register_count;
>
> This isn't a style clean up.

It is.

There's no difference before and after, the only difference is the style.

ERROR: do not initialise statics to 0 or NULL
#91: FILE: acpi/video.c:91:
+static int register_count = 0;

-- 
Felipe Contreras

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Rafael J. Wysocki]

On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote:
> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> > On 08/02/2013 07:43 AM, Felipe Contreras wrote:
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >
> > Please add change log explaining what you have changed.
> > It seems that the patch modify comment style only, some add a space and
> > some change spaces to tab, is it the case?
> 
> The commit message already explains what the change is; trivial
> cosmetic cleanups. Cosmetic means it's completely superficial.

And I have a rule not to apply patches without changelogs.  So either I'll
need to write it for you, or can you add one pretty please?

Rafael

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Rafael J. Wysocki]

On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >
> > Change log please.
> 
> You mean a commit message?

No.  He meant the part that goes between the subject and the signoff.
This is called a change log (or changelog).

> That's what it's called in Git lingo, and
> it's right there:
> 
> acpi: video: trivial style cleanups

And that's not sufficient, because it doesn't explain *what* cleanups are
being made.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Felipe Contreras]

On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote:
>> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> > On 08/02/2013 07:43 AM, Felipe Contreras wrote:
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >
>> > Please add change log explaining what you have changed.
>> > It seems that the patch modify comment style only, some add a space and
>> > some change spaces to tab, is it the case?
>>
>> The commit message already explains what the change is; trivial
>> cosmetic cleanups. Cosmetic means it's completely superficial.
>
> And I have a rule not to apply patches without changelogs.  So either I'll
> need to write it for you, or can you add one pretty please?

The commit message is right there. Maybe Jiri can apply it then, if
not, then stay happy with your untidy code.

-- 
Felipe Contreras

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Felipe Contreras]

On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
>> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >
>> > Change log please.
>>
>> You mean a commit message?
>
> No.  He meant the part that goes between the subject and the signoff.
> This is called a change log (or changelog).

Not in Git lingo.

% man git commit

"Though not required, it’s a good idea to begin the commit message
with a single short (less than 50 character) line summarizing the
change, followed by a blank line and then a more thorough
description."

>> That's what it's called in Git lingo, and
>> it's right there:
>>
>> acpi: video: trivial style cleanups
>
> And that's not sufficient, because it doesn't explain *what* cleanups are
> being made.

Yes it does; *style* cleanups (coding-style).

-- 
Felipe Contreras
--
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

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Felipe Contreras]

On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
>> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >
>> > Change log please.
>>
>> You mean a commit message?
>
> No.  He meant the part that goes between the subject and the signoff.
> This is called a change log (or changelog).

Not in Git lingo.

% man git commit

"Though not required, it’s a good idea to begin the commit message
with a single short (less than 50 character) line summarizing the
change, followed by a blank line and then a more thorough
description."

>> That's what it's called in Git lingo, and
>> it's right there:
>>
>> acpi: video: trivial style cleanups
>
> And that's not sufficient, because it doesn't explain *what* cleanups are
> being made.

Yes it does; *style* cleanups (coding-style).

-- 
Felipe Contreras

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Rafael J. Wysocki]

On Friday, August 02, 2013 12:56:09 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
> >> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> >> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> >
> >> > Change log please.
> >>
> >> You mean a commit message?
> >
> > No.  He meant the part that goes between the subject and the signoff.
> > This is called a change log (or changelog).
> 
> Not in Git lingo.
> 
> % man git commit
> 
> "Though not required, it’s a good idea to begin the commit message
> with a single short (less than 50 character) line summarizing the
> change, followed by a blank line and then a more thorough
> description."

Please go and read this: https://lwn.net/Articles/560392/

Now, you may still argue that your patches fall into the "add missing include
of foo.h" category, but it does several different things:
- fixes some whitespace,
- fixes a couple of static variable initializations,
- removes some braces,
- changes the placement of some lables (some of them unnecessarily).

It would be simply *nice* to write what it does in the changelog so that
people reading the git log don't have to look deeper to see what changes the
author meant as "trivial style cleanups".

That's just a matter of making it easier to work with you for other people,
but maybe you just want to be difficult to work with in the first place?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Rafael J. Wysocki]

On Friday, August 02, 2013 12:56:09 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
> >> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> >> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> >
> >> > Change log please.
> >>
> >> You mean a commit message?
> >
> > No.  He meant the part that goes between the subject and the signoff.
> > This is called a change log (or changelog).
> 
> Not in Git lingo.
> 
> % man git commit
> 
> "Though not required, it’s a good idea to begin the commit message
> with a single short (less than 50 character) line summarizing the
> change, followed by a blank line and then a more thorough
> description."

Please go and read this: https://lwn.net/Articles/560392/

Now, you may still argue that your patches fall into the "add missing include
of foo.h" category, but it does several different things:
- fixes some whitespace,
- fixes a couple of static variable initializations,
- removes some braces,
- changes the placement of some lables (some of them unnecessarily).

It would be simply *nice* to write what it does in the changelog so that
people reading the git log don't have to look deeper to see what changes the
author meant as "trivial style cleanups".

That's just a matter of making it easier to work with you for other people,
but maybe you just want to be difficult to work with in the first place?

Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Rafael J. Wysocki]

On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote:
> >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote:
> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> >
> >> > Please add change log explaining what you have changed.
> >> > It seems that the patch modify comment style only, some add a space and
> >> > some change spaces to tab, is it the case?
> >>
> >> The commit message already explains what the change is; trivial
> >> cosmetic cleanups. Cosmetic means it's completely superficial.
> >
> > And I have a rule not to apply patches without changelogs.  So either I'll
> > need to write it for you, or can you add one pretty please?
> 
> The commit message is right there. Maybe Jiri can apply it then, if
> not, then stay happy with your untidy code.

First of all, I didn't say I wouldn't apply the patch, did I?

Second, I asked you *nicely* to add a changelog so that I don't need to write
it for you.

I don't know what made it difficult to understand.

Anyway, I ask everyone to write changelogs and nobody has had any problems with
that so far.  I don't see why I should avoid asking you to follow the rules
that everybody else is asked to follow.  If those rules are too difficult for
you to follow, I'm sorry.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Felipe Contreras]

On Fri, Aug 2, 2013 at 7:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, August 02, 2013 12:56:09 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
>> >> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> >> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> >
>> >> > Change log please.
>> >>
>> >> You mean a commit message?
>> >
>> > No.  He meant the part that goes between the subject and the signoff.
>> > This is called a change log (or changelog).
>>
>> Not in Git lingo.
>>
>> % man git commit
>>
>> "Though not required, it’s a good idea to begin the commit message
>> with a single short (less than 50 character) line summarizing the
>> change, followed by a blank line and then a more thorough
>> description."
>
> Please go and read this: https://lwn.net/Articles/560392/

OK, I've read it, and he doesn't seem to have a problem with trivial patches:

"For the most trivial patches, the one-line summary might suffice"

This patch falls into that category.

> Now, you may still argue that your patches fall into the "add missing include
> of foo.h" category, but it does several different things:
> - fixes some whitespace,
> - fixes a couple of static variable initializations,
> - removes some braces,
> - changes the placement of some lables (some of them unnecessarily).

All of these fall under the category of coding style fixes.

Jonathan Colbert lists the reasons for a detailed commit message description:

* the original motivation for the work is quickly forgotten

The motivation is clear; cleanup the code.

* Andrew Morton also famously pushes developers to document the
reasons explaining why a patch was written, including the user-visible
effects of any bugs fixed

It's clear; the reason for the cleanup is that the code is not clean.
There's no user-visible effects of code cleanups.

* Kernel developers do not like having to reverse engineer the intent
of a patch years after the fact.

The intent is clear; cleanup.

> It would be simply *nice* to write what it does in the changelog so that
> people reading the git log don't have to look deeper to see what changes the
> author meant as "trivial style cleanups".

Each time I've looked into a cleanup patch, I look at the code, never
the description. Mistakes can be made in the cleanup, the description
won't change that.

I have landed these one-line-commit-message trivial cleanups in the
kernel before. Never have I seen questions of; "what did this cleanup
patch tried to do?", or; "the commit message says you fixed code-style
A, but you also did whitespace changes, was that intended?"

Be honest; if you apply this patch, nobody is going to see it ever again.

> That's just a matter of making it easier to work with you for other people,
> but maybe you just want to be difficult to work with in the first place?

I add a proper description to the patches that deserve them. This
patch is not one of them.

I update the description according to the feedback to the patches that
deserve that. This patch is not one of them.

No user is ever going to be affected by this patch, I don't care that
much if you apply it or not, it's not important.

It's not worth my time to add a description that nobody is ever going
to read, so if you are going to drop it, go ahead.

If every time somebody provides a trivial cleanup patch you are going
to ask people to describe what each little obvious change it does,
it's no wonder the code doesn't get cleaned up.

Cheers.

-- 
Felipe Contreras
--
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

Re: [PATCH 2/3] acpi: video: trivial style cleanups

[Felipe Contreras]

On Fri, Aug 2, 2013 at 7:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, August 02, 2013 12:56:09 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 9:09 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Thursday, August 01, 2013 11:18:34 PM Felipe Contreras wrote:
>> >> On Thu, Aug 1, 2013 at 8:55 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> >> > On 08/02/2013 07:44 AM, Felipe Contreras wrote:
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> >
>> >> > Change log please.
>> >>
>> >> You mean a commit message?
>> >
>> > No.  He meant the part that goes between the subject and the signoff.
>> > This is called a change log (or changelog).
>>
>> Not in Git lingo.
>>
>> % man git commit
>>
>> "Though not required, it’s a good idea to begin the commit message
>> with a single short (less than 50 character) line summarizing the
>> change, followed by a blank line and then a more thorough
>> description."
>
> Please go and read this: https://lwn.net/Articles/560392/

OK, I've read it, and he doesn't seem to have a problem with trivial patches:

"For the most trivial patches, the one-line summary might suffice"

This patch falls into that category.

> Now, you may still argue that your patches fall into the "add missing include
> of foo.h" category, but it does several different things:
> - fixes some whitespace,
> - fixes a couple of static variable initializations,
> - removes some braces,
> - changes the placement of some lables (some of them unnecessarily).

All of these fall under the category of coding style fixes.

Jonathan Colbert lists the reasons for a detailed commit message description:

* the original motivation for the work is quickly forgotten

The motivation is clear; cleanup the code.

* Andrew Morton also famously pushes developers to document the
reasons explaining why a patch was written, including the user-visible
effects of any bugs fixed

It's clear; the reason for the cleanup is that the code is not clean.
There's no user-visible effects of code cleanups.

* Kernel developers do not like having to reverse engineer the intent
of a patch years after the fact.

The intent is clear; cleanup.

> It would be simply *nice* to write what it does in the changelog so that
> people reading the git log don't have to look deeper to see what changes the
> author meant as "trivial style cleanups".

Each time I've looked into a cleanup patch, I look at the code, never
the description. Mistakes can be made in the cleanup, the description
won't change that.

I have landed these one-line-commit-message trivial cleanups in the
kernel before. Never have I seen questions of; "what did this cleanup
patch tried to do?", or; "the commit message says you fixed code-style
A, but you also did whitespace changes, was that intended?"

Be honest; if you apply this patch, nobody is going to see it ever again.

> That's just a matter of making it easier to work with you for other people,
> but maybe you just want to be difficult to work with in the first place?

I add a proper description to the patches that deserve them. This
patch is not one of them.

I update the description according to the feedback to the patches that
deserve that. This patch is not one of them.

No user is ever going to be affected by this patch, I don't care that
much if you apply it or not, it's not important.

It's not worth my time to add a description that nobody is ever going
to read, so if you are going to drop it, go ahead.

If every time somebody provides a trivial cleanup patch you are going
to ask people to describe what each little obvious change it does,
it's no wonder the code doesn't get cleaned up.

Cheers.

-- 
Felipe Contreras

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Felipe Contreras]

On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote:
>> >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote:
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> >
>> >> > Please add change log explaining what you have changed.
>> >> > It seems that the patch modify comment style only, some add a space and
>> >> > some change spaces to tab, is it the case?
>> >>
>> >> The commit message already explains what the change is; trivial
>> >> cosmetic cleanups. Cosmetic means it's completely superficial.
>> >
>> > And I have a rule not to apply patches without changelogs.  So either I'll
>> > need to write it for you, or can you add one pretty please?
>>
>> The commit message is right there. Maybe Jiri can apply it then, if
>> not, then stay happy with your untidy code.
>
> First of all, I didn't say I wouldn't apply the patch, did I?
>
> Second, I asked you *nicely* to add a changelog so that I don't need to write
> it for you.
>
> I don't know what made it difficult to understand.
>
> Anyway, I ask everyone to write changelogs and nobody has had any problems with
> that so far.  I don't see why I should avoid asking you to follow the rules
> that everybody else is asked to follow.  If those rules are too difficult for
> you to follow, I'm sorry.

The patch has a commit message that describes exactly what it does.
Unless there is valid feedback I will not send another version.

To me, a valid criticism to the commit message would be: "I read X,
but I thought it would do Y". For example; "I didn't expect the patch
to do white-space cleanups", but I think that's exactly what people
expect when they read "trivial costmetic cleanups'.

-- 
Felipe Contreras

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Rafael J. Wysocki]

On Friday, August 02, 2013 08:34:29 PM Felipe Contreras wrote:
> On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote:
> >> On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote:
> >> >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
> >> >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote:
> >> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> >> >
> >> >> > Please add change log explaining what you have changed.
> >> >> > It seems that the patch modify comment style only, some add a space and
> >> >> > some change spaces to tab, is it the case?
> >> >>
> >> >> The commit message already explains what the change is; trivial
> >> >> cosmetic cleanups. Cosmetic means it's completely superficial.
> >> >
> >> > And I have a rule not to apply patches without changelogs.  So either I'll
> >> > need to write it for you, or can you add one pretty please?
> >>
> >> The commit message is right there. Maybe Jiri can apply it then, if
> >> not, then stay happy with your untidy code.
> >
> > First of all, I didn't say I wouldn't apply the patch, did I?
> >
> > Second, I asked you *nicely* to add a changelog so that I don't need to write
> > it for you.
> >
> > I don't know what made it difficult to understand.
> >
> > Anyway, I ask everyone to write changelogs and nobody has had any problems with
> > that so far.  I don't see why I should avoid asking you to follow the rules
> > that everybody else is asked to follow.  If those rules are too difficult for
> > you to follow, I'm sorry.
> 
> The patch has a commit message that describes exactly what it does.

No, it doesn't describe it exactly.  You're contradicting facts.

> Unless there is valid feedback I will not send another version.
> 
> To me, a valid criticism to the commit message would be: "I read X,
> but I thought it would do Y". For example; "I didn't expect the patch
> to do white-space cleanups", but I think that's exactly what people
> expect when they read "trivial costmetic cleanups'.

If what you're saying was correct, then it would be sufficient to use a
"this patch fixes a bug" commit message for every bug fix, but quite evidently
that is not the case.

Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups

[Felipe Contreras]

On Sat, Aug 3, 2013 at 6:38 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, August 02, 2013 08:34:29 PM Felipe Contreras wrote:
>> On Fri, Aug 2, 2013 at 7:07 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Friday, August 02, 2013 12:52:18 PM Felipe Contreras wrote:
>> >> On Fri, Aug 2, 2013 at 9:05 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> > On Thursday, August 01, 2013 11:15:38 PM Felipe Contreras wrote:
>> >> >> On Thu, Aug 1, 2013 at 8:50 PM, Aaron Lu <aaron.lwe@gmail.com> wrote:
>> >> >> > On 08/02/2013 07:43 AM, Felipe Contreras wrote:
>> >> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> >> >
>> >> >> > Please add change log explaining what you have changed.
>> >> >> > It seems that the patch modify comment style only, some add a space and
>> >> >> > some change spaces to tab, is it the case?
>> >> >>
>> >> >> The commit message already explains what the change is; trivial
>> >> >> cosmetic cleanups. Cosmetic means it's completely superficial.
>> >> >
>> >> > And I have a rule not to apply patches without changelogs.  So either I'll
>> >> > need to write it for you, or can you add one pretty please?
>> >>
>> >> The commit message is right there. Maybe Jiri can apply it then, if
>> >> not, then stay happy with your untidy code.
>> >
>> > First of all, I didn't say I wouldn't apply the patch, did I?
>> >
>> > Second, I asked you *nicely* to add a changelog so that I don't need to write
>> > it for you.
>> >
>> > I don't know what made it difficult to understand.
>> >
>> > Anyway, I ask everyone to write changelogs and nobody has had any problems with
>> > that so far.  I don't see why I should avoid asking you to follow the rules
>> > that everybody else is asked to follow.  If those rules are too difficult for
>> > you to follow, I'm sorry.
>>
>> The patch has a commit message that describes exactly what it does.
>
> No, it doesn't describe it exactly.  You're contradicting facts.
>
>> Unless there is valid feedback I will not send another version.
>>
>> To me, a valid criticism to the commit message would be: "I read X,
>> but I thought it would do Y". For example; "I didn't expect the patch
>> to do white-space cleanups", but I think that's exactly what people
>> expect when they read "trivial costmetic cleanups'.
>
> If what you're saying was correct, then it would be sufficient to use a
> "this patch fixes a bug" commit message for every bug fix, but quite evidently
> that is not the case.

No, it wouldn't be sufficient, take a look a the Corbert's list you
yourself mentioned:

* the original motivation for the work is quickly forgotten

"this patch fixes a bug" doesn't describe the motivation.

* Andrew Morton also famously pushes developers to document the
reasons explaining why a patch was written, including the user-visible
effects of any bugs fixed

The reason for the patch is not documented, nor the user-visible effects.

* Kernel developers do not like having to reverse engineer the intent
of a patch years after the fact.

The intent of the patch is not mentioned.

That is completely different with my patch.

Personally I like to answer these questions: What is the patch is
doing (motivation)? What is the current problem? What is the change?
What are the side-effects?

All those are clear with "trivial costmetic cleanups", they are not
with "this patch fixes a bug".

I think you are committing a hasty generalization fallacy. Not all
patches are the same.

-- 
Felipe Contreras