* [PATCH 0/3] acpi: video: trivial cleanups
@ 2013-08-01 23:43 Felipe Contreras
2013-08-01 23:43 ` [PATCH 1/3] acpi: video: trivial costmetic cleanups Felipe Contreras
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Felipe Contreras @ 2013-08-01 23:43 UTC (permalink / raw)
To: linux-kernel, linux-acpi
Cc: Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina, 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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-01 23:43 [PATCH 0/3] acpi: video: trivial cleanups Felipe Contreras @ 2013-08-01 23:43 ` Felipe Contreras 2013-08-02 1:50 ` Aaron Lu 2013-08-01 23:44 ` [PATCH 2/3] acpi: video: trivial style cleanups Felipe Contreras 2013-08-01 23:44 ` [PATCH 3/3] acpi: video: remove unnecessary casting Felipe Contreras 2 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-01 23:43 UTC (permalink / raw) To: linux-kernel, linux-acpi Cc: Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-01 23:43 ` [PATCH 1/3] acpi: video: trivial costmetic cleanups Felipe Contreras @ 2013-08-02 1:50 ` Aaron Lu 2013-08-02 4:15 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Aaron Lu @ 2013-08-02 1:50 UTC (permalink / raw) To: Felipe Contreras Cc: linux-kernel, linux-acpi, Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina 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 > * > */ > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-02 1:50 ` Aaron Lu @ 2013-08-02 4:15 ` Felipe Contreras 2013-08-02 14:05 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-02 4:15 UTC (permalink / raw) To: Aaron Lu Cc: linux-kernel, linux-acpi, Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-02 4:15 ` Felipe Contreras @ 2013-08-02 14:05 ` Rafael J. Wysocki 2013-08-02 17:52 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2013-08-02 14:05 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-02 14:05 ` Rafael J. Wysocki @ 2013-08-02 17:52 ` Felipe Contreras 2013-08-03 0:07 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-02 17:52 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-02 17:52 ` Felipe Contreras @ 2013-08-03 0:07 ` Rafael J. Wysocki 2013-08-03 1:34 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 0:07 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-03 0:07 ` Rafael J. Wysocki @ 2013-08-03 1:34 ` Felipe Contreras 2013-08-03 11:38 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-03 1:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-03 1:34 ` Felipe Contreras @ 2013-08-03 11:38 ` Rafael J. Wysocki 2013-08-03 20:28 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 11:38 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] acpi: video: trivial costmetic cleanups 2013-08-03 11:38 ` Rafael J. Wysocki @ 2013-08-03 20:28 ` Felipe Contreras 0 siblings, 0 replies; 19+ messages in thread From: Felipe Contreras @ 2013-08-03 20:28 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-01 23:43 [PATCH 0/3] acpi: video: trivial cleanups Felipe Contreras 2013-08-01 23:43 ` [PATCH 1/3] acpi: video: trivial costmetic cleanups Felipe Contreras @ 2013-08-01 23:44 ` Felipe Contreras 2013-08-02 1:55 ` Aaron Lu 2013-08-01 23:44 ` [PATCH 3/3] acpi: video: remove unnecessary casting Felipe Contreras 2 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-01 23:44 UTC (permalink / raw) To: linux-kernel, linux-acpi Cc: Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-01 23:44 ` [PATCH 2/3] acpi: video: trivial style cleanups Felipe Contreras @ 2013-08-02 1:55 ` Aaron Lu 2013-08-02 4:18 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Aaron Lu @ 2013-08-02 1:55 UTC (permalink / raw) To: Felipe Contreras Cc: linux-kernel, linux-acpi, Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-02 1:55 ` Aaron Lu @ 2013-08-02 4:18 ` Felipe Contreras 2013-08-02 14:09 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-02 4:18 UTC (permalink / raw) To: Aaron Lu Cc: linux-kernel, linux-acpi, Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-02 4:18 ` Felipe Contreras @ 2013-08-02 14:09 ` Rafael J. Wysocki 2013-08-02 17:56 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2013-08-02 14:09 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-02 14:09 ` Rafael J. Wysocki @ 2013-08-02 17:56 ` Felipe Contreras 2013-08-03 0:01 ` Rafael J. Wysocki 0 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-02 17:56 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-02 17:56 ` Felipe Contreras @ 2013-08-03 0:01 ` Rafael J. Wysocki 2013-08-03 1:28 ` Felipe Contreras 0 siblings, 1 reply; 19+ messages in thread From: Rafael J. Wysocki @ 2013-08-03 0:01 UTC (permalink / raw) To: Felipe Contreras Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/3] acpi: video: trivial style cleanups 2013-08-03 0:01 ` Rafael J. Wysocki @ 2013-08-03 1:28 ` Felipe Contreras 0 siblings, 0 replies; 19+ messages in thread From: Felipe Contreras @ 2013-08-03 1:28 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Aaron Lu, linux-kernel, linux-acpi, Len Brown, Zhang Rui, Jiri Kosina 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] acpi: video: remove unnecessary casting 2013-08-01 23:43 [PATCH 0/3] acpi: video: trivial cleanups Felipe Contreras 2013-08-01 23:43 ` [PATCH 1/3] acpi: video: trivial costmetic cleanups Felipe Contreras 2013-08-01 23:44 ` [PATCH 2/3] acpi: video: trivial style cleanups Felipe Contreras @ 2013-08-01 23:44 ` Felipe Contreras 2013-08-02 1:58 ` Aaron Lu 2 siblings, 1 reply; 19+ messages in thread From: Felipe Contreras @ 2013-08-01 23:44 UTC (permalink / raw) To: linux-kernel, linux-acpi Cc: Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/3] acpi: video: remove unnecessary casting 2013-08-01 23:44 ` [PATCH 3/3] acpi: video: remove unnecessary casting Felipe Contreras @ 2013-08-02 1:58 ` Aaron Lu 0 siblings, 0 replies; 19+ messages in thread From: Aaron Lu @ 2013-08-02 1:58 UTC (permalink / raw) To: Felipe Contreras Cc: linux-kernel, linux-acpi, Rafael J. Wysocki, Len Brown, Zhang Rui, Jiri Kosina 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]); > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2013-08-03 20:29 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-01 23:43 [PATCH 0/3] acpi: video: trivial cleanups Felipe Contreras 2013-08-01 23:43 ` [PATCH 1/3] acpi: video: trivial costmetic cleanups Felipe Contreras 2013-08-02 1:50 ` Aaron Lu 2013-08-02 4:15 ` Felipe Contreras 2013-08-02 14:05 ` Rafael J. Wysocki 2013-08-02 17:52 ` Felipe Contreras 2013-08-03 0:07 ` Rafael J. Wysocki 2013-08-03 1:34 ` Felipe Contreras 2013-08-03 11:38 ` Rafael J. Wysocki 2013-08-03 20:28 ` Felipe Contreras 2013-08-01 23:44 ` [PATCH 2/3] acpi: video: trivial style cleanups Felipe Contreras 2013-08-02 1:55 ` Aaron Lu 2013-08-02 4:18 ` Felipe Contreras 2013-08-02 14:09 ` Rafael J. Wysocki 2013-08-02 17:56 ` Felipe Contreras 2013-08-03 0:01 ` Rafael J. Wysocki 2013-08-03 1:28 ` Felipe Contreras 2013-08-01 23:44 ` [PATCH 3/3] acpi: video: remove unnecessary casting Felipe Contreras 2013-08-02 1:58 ` Aaron Lu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).