| Submitter | Felipe Contreras |
|---|---|
| Date | 2009-10-18 22:54:32 |
| Message ID | <1255906474-25091-6-git-send-email-felipe.contreras@gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/54658/ |
| State | New |
| Headers | show |
Comments
On Mon, 19 Oct 2009, Felipe Contreras wrote: > Many still remain. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> I have never been in favor of merging whitespace-only patches (in a sense that the sole purpose of them being to change whitespaces, but no else value added). And after today's discussion on kernel summit on this topic, I wouldn't expect any maintainer to merge it, sorry :)
On Mon, Oct 19, 2009 at 5:20 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Mon, 19 Oct 2009, Felipe Contreras wrote: > >> Many still remain. >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > I have never been in favor of merging whitespace-only patches (in a sense > that the sole purpose of them being to change whitespaces, but no else > value added). If somebody tries to send a patch for that file that doesn't fix the white-space, checkpatch will complain, and people will complain that checkpatch complains, which is precisely what happened, and I was requested to write this patch by Daniel Walker (final mail wasn't on the ml): http://lkml.org/lkml/2009/9/14/183 > And after today's discussion on kernel summit on this topic, I wouldn't > expect any maintainer to merge it, sorry :) What are you talking about?
On Mon, 19 Oct 2009, Felipe Contreras wrote: > > I have never been in favor of merging whitespace-only patches (in a > > sense that the sole purpose of them being to change whitespaces, but > > no else value added). > If somebody tries to send a patch for that file that doesn't fix the > white-space, checkpatch will complain, and people will complain that > checkpatch complains, which is precisely what happened, Oh, well ... checkpatch warning about this is somewhat controversial. My preferred way would be that it warns about whitespace only if there are also some other (non-whitespace) changes. > and I was requested to write this patch by Daniel Walker (final mail > wasn't on the ml): > > http://lkml.org/lkml/2009/9/14/183 This is something slightly different -- he asks you to fixup whitespace issue in the code you are newly introducing, right? > > And after today's discussion on kernel summit on this topic, I wouldn't > > expect any maintainer to merge it, sorry :) > What are you talking about? Seems like many kernel maintainers are just tired of 'whitespace-cleanup-only' patches that bring no real added value otherwise. Thanks,
On Mon, Oct 19, 2009 at 6:03 PM, Jiri Kosina <jkosina@suse.cz> wrote: > On Mon, 19 Oct 2009, Felipe Contreras wrote: > >> > I have never been in favor of merging whitespace-only patches (in a >> > sense that the sole purpose of them being to change whitespaces, but >> > no else value added). >> If somebody tries to send a patch for that file that doesn't fix the >> white-space, checkpatch will complain, and people will complain that >> checkpatch complains, which is precisely what happened, > > Oh, well ... checkpatch warning about this is somewhat controversial. My > preferred way would be that it warns about whitespace only if there are > also some other (non-whitespace) changes. Huh? I think we are talking about different things. See the next comment. >> and I was requested to write this patch by Daniel Walker (final mail >> wasn't on the ml): >> >> http://lkml.org/lkml/2009/9/14/183 > > This is something slightly different -- he asks you to fixup whitespace > issue in the code you are newly introducing, right? No, did you read the thread? This was my patch: -#define ACPI_MIN(a,b) (((a)<(b))?(a):(b)) -#define ACPI_MAX(a,b) (((a)>(b))?(a):(b)) +#define ACPI_MIN(a,b) min(a, b) +#define ACPI_MAX(a,b) max(a, b) Checkpatch complains, even though my changes are ok. So that's what I mean, if somebody wants to do a similar patch in the future so that checkpatch doesn't complain; they would have to fix the white-spaces again. Or checkpatch should be fixed. >> > And after today's discussion on kernel summit on this topic, I wouldn't >> > expect any maintainer to merge it, sorry :) >> What are you talking about? > > Seems like many kernel maintainers are just tired of > 'whitespace-cleanup-only' patches that bring no real added value > otherwise. Hm, I wonder what would happen to the current badly formatted code. Stay there forever?
On Mon, 2009-10-19 at 17:03 +0200, Jiri Kosina wrote: > On Mon, 19 Oct 2009, Felipe Contreras wrote: > > > > I have never been in favor of merging whitespace-only patches (in a > > > sense that the sole purpose of them being to change whitespaces, but > > > no else value added). > > If somebody tries to send a patch for that file that doesn't fix the > > white-space, checkpatch will complain, and people will complain that > > checkpatch complains, which is precisely what happened, > > Oh, well ... checkpatch warning about this is somewhat controversial. My > preferred way would be that it warns about whitespace only if there are > also some other (non-whitespace) changes. > > > and I was requested to write this patch by Daniel Walker (final mail > > wasn't on the ml): > > > > http://lkml.org/lkml/2009/9/14/183 > > This is something slightly different -- he asks you to fixup whitespace > issue in the code you are newly introducing, right? > > > > And after today's discussion on kernel summit on this topic, I wouldn't > > > expect any maintainer to merge it, sorry :) > > What are you talking about? > > Seems like many kernel maintainers are just tired of > 'whitespace-cleanup-only' patches that bring no real added value > otherwise. May be some are tired, but others just say thanks and apply them, because it is easier to apply than complain, and because they do not mind if their subsystem becomes a tiny bit cleaner. Sometimes it may cause troubles, but hey, development is not easy and we are accustomed to fix conflict and amend patches. But the include/acpi/actypes.h does not seem to be changing very often anyway.
On Mon, 2009-10-19 at 16:20 +0200, Jiri Kosina wrote: > On Mon, 19 Oct 2009, Felipe Contreras wrote: > > > Many still remain. > > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > I have never been in favor of merging whitespace-only patches (in a sense > that the sole purpose of them being to change whitespaces, but no else > value added). > > And after today's discussion on kernel summit on this topic, I wouldn't > expect any maintainer to merge it, sorry :) could you be more specific about what "discussion" you are talking about. Daniel -- 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
>-----Original Message----- >From: Daniel Walker [mailto:dwalker@fifo99.com] >Sent: Friday, November 13, 2009 12:59 PM >To: Jiri Kosina >Cc: Felipe Contreras; linux-kernel@vger.kernel.org; Len Brown; Moore, >Robert; Lin, Ming M; linux-acpi@vger.kernel.org >Subject: Re: [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h' > >On Mon, 2009-10-19 at 16:20 +0200, Jiri Kosina wrote: >> On Mon, 19 Oct 2009, Felipe Contreras wrote: >> >> > Many still remain. >> > >> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> >> >> I have never been in favor of merging whitespace-only patches (in a sense >> that the sole purpose of them being to change whitespaces, but no else >> value added). >> >> And after today's discussion on kernel summit on this topic, I wouldn't >> expect any maintainer to merge it, sorry :) > >could you be more specific about what "discussion" you are talking >about. > Yes, do tell. >Daniel -- 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
On Fri, 13 Nov 2009, Moore, Robert wrote: > >> And after today's discussion on kernel summit on this topic, I > >> wouldn't expect any maintainer to merge it, sorry :) > >could you be more specific about what "discussion" you are talking > >about. > Yes, do tell. I have probably lost the track of the context here already, sorry. But it probably had something to do with the fact that whitespace-only patches (which [PATCH 5/7] acpi: fix a bunch of style issues on 'actypes.h' has been, as far as I remember), are unnecessary noise.
On Fri, 2009-11-13 at 22:41 +0100, Jiri Kosina wrote: > On Fri, 13 Nov 2009, Moore, Robert wrote: > > > >> And after today's discussion on kernel summit on this topic, I > > >> wouldn't expect any maintainer to merge it, sorry :) > > >could you be more specific about what "discussion" you are talking > > >about. > > Yes, do tell. > > I have probably lost the track of the context here already, sorry. > > But it probably had something to do with the fact that whitespace-only > patches (which [PATCH 5/7] acpi: fix a bunch of style issues on > 'actypes.h' has been, as far as I remember), are unnecessary noise. I don't think it was clear from the description , but I think this is actually a checkpatch cleanup. So it's not just random whitespace changes. AFAIK, maintainers do accept those. Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Patch
diff --git a/include/acpi/actypes.h b/include/acpi/actypes.h index 153f12d..c1a0eb4 100644 --- a/include/acpi/actypes.h +++ b/include/acpi/actypes.h @@ -167,7 +167,7 @@ typedef u64 acpi_physical_address; * Note: Em64_t and other X86-64 processors support misaligned transfers, * so there is no need to define this flag. */ -#if defined (__IA64__) || defined (__ia64__) +#if defined(__IA64__) || defined(__ia64__) #define ACPI_MISALIGNMENT_NOT_SUPPORTED #endif @@ -242,10 +242,10 @@ typedef u32 acpi_physical_address; * Map the OSL Mutex interfaces to binary semaphores. */ #define acpi_mutex acpi_semaphore -#define acpi_os_create_mutex(out_handle) acpi_os_create_semaphore (1, 1, out_handle) -#define acpi_os_delete_mutex(handle) (void) acpi_os_delete_semaphore (handle) -#define acpi_os_acquire_mutex(handle,time) acpi_os_wait_semaphore (handle, 1, time) -#define acpi_os_release_mutex(handle) (void) acpi_os_signal_semaphore (handle, 1) +#define acpi_os_create_mutex(out_handle) acpi_os_create_semaphore(1, 1, out_handle) +#define acpi_os_delete_mutex(handle) (void) acpi_os_delete_semaphore(handle) +#define acpi_os_acquire_mutex(handle, time) acpi_os_wait_semaphore(handle, 1, time) +#define acpi_os_release_mutex(handle) (void) acpi_os_signal_semaphore(handle, 1) #endif /* Configurable types for synchronization objects */ @@ -417,7 +417,7 @@ typedef unsigned long long acpi_integer; /* * Constants with special meanings */ -#define ACPI_ROOT_OBJECT ACPI_ADD_PTR (acpi_handle, NULL, ACPI_MAX_PTR) +#define ACPI_ROOT_OBJECT ACPI_ADD_PTR(acpi_handle, NULL, ACPI_MAX_PTR) #define ACPI_WAIT_FOREVER 0xFFFF /* u16, as per ACPI spec */ #define ACPI_DO_NOT_WAIT 0 @@ -438,8 +438,8 @@ typedef unsigned long long acpi_integer; #define ACPI_SET_BIT(target,bit) ((target) |= (bit)) #define ACPI_CLEAR_BIT(target,bit) ((target) &= ~(bit)) -#define ACPI_MIN(a,b) (((a)<(b))?(a):(b)) -#define ACPI_MAX(a,b) (((a)>(b))?(a):(b)) +#define ACPI_MIN(a, b) (((a)<(b))?(a):(b)) +#define ACPI_MAX(a, b) (((a)>(b))?(a):(b)) /* Size calculation */ @@ -449,21 +449,21 @@ typedef unsigned long long acpi_integer; #define ACPI_CAST_PTR(t, p) ((t *) (acpi_uintptr_t) (p)) #define ACPI_CAST_INDIRECT_PTR(t, p) ((t **) (acpi_uintptr_t) (p)) -#define ACPI_ADD_PTR(t, a, b) ACPI_CAST_PTR (t, (ACPI_CAST_PTR (u8, (a)) + (acpi_size)(b))) -#define ACPI_PTR_DIFF(a, b) (acpi_size) (ACPI_CAST_PTR (u8, (a)) - ACPI_CAST_PTR (u8, (b))) +#define ACPI_ADD_PTR(t, a, b) ACPI_CAST_PTR(t, (ACPI_CAST_PTR(u8, (a)) + (acpi_size)(b))) +#define ACPI_PTR_DIFF(a, b) (acpi_size) (ACPI_CAST_PTR(u8, (a)) - ACPI_CAST_PTR(u8, (b))) /* Pointer/Integer type conversions */ -#define ACPI_TO_POINTER(i) ACPI_ADD_PTR (void, (void *) NULL,(acpi_size) i) -#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF (p, (void *) NULL) -#define ACPI_OFFSET(d, f) (acpi_size) ACPI_PTR_DIFF (&(((d *)0)->f), (void *) NULL) +#define ACPI_TO_POINTER(i) ACPI_ADD_PTR(void, (void *) NULL, (acpi_size) i) +#define ACPI_TO_INTEGER(p) ACPI_PTR_DIFF(p, (void *) NULL) +#define ACPI_OFFSET(d, f) (acpi_size) ACPI_PTR_DIFF(&(((d *)0)->f), (void *) NULL) #define ACPI_PHYSADDR_TO_PTR(i) ACPI_TO_POINTER(i) #define ACPI_PTR_TO_PHYSADDR(i) ACPI_TO_INTEGER(i) #ifndef ACPI_MISALIGNMENT_NOT_SUPPORTED -#define ACPI_COMPARE_NAME(a,b) (*ACPI_CAST_PTR (u32, (a)) == *ACPI_CAST_PTR (u32, (b))) +#define ACPI_COMPARE_NAME(a, b) (*ACPI_CAST_PTR(u32, (a)) == *ACPI_CAST_PTR(u32, (b))) #else -#define ACPI_COMPARE_NAME(a,b) (!ACPI_STRNCMP (ACPI_CAST_PTR (char, (a)), ACPI_CAST_PTR (char, (b)), ACPI_NAME_SIZE)) +#define ACPI_COMPARE_NAME(a, b) (!ACPI_STRNCMP(ACPI_CAST_PTR(char, (a)), ACPI_CAST_PTR(char, (b)), ACPI_NAME_SIZE)) #endif /******************************************************************************* @@ -631,7 +631,7 @@ typedef u32 acpi_event_type; #define ACPI_EVENT_SLEEP_BUTTON 3 #define ACPI_EVENT_RTC 4 #define ACPI_EVENT_MAX 4 -#define ACPI_NUM_FIXED_EVENTS ACPI_EVENT_MAX + 1 +#define ACPI_NUM_FIXED_EVENTS (ACPI_EVENT_MAX + 1) /* * Event Status - Per event @@ -778,7 +778,7 @@ typedef u8 acpi_adr_space_type; #define ACPI_BITREG_ARB_DISABLE 0x13 #define ACPI_BITREG_MAX 0x13 -#define ACPI_NUM_BITREG ACPI_BITREG_MAX + 1 +#define ACPI_NUM_BITREG (ACPI_BITREG_MAX + 1) /* Status register values. A 1 clears a status bit. 0 = no effect */ @@ -945,7 +945,7 @@ typedef acpi_status(*acpi_adr_space_handler) (u32 function, acpi_physical_address address, u32 bit_width, - acpi_integer * value, + acpi_integer *value, void *handler_context, void *region_context);
Many still remain. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- include/acpi/actypes.h | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-)