| Message ID | 1424824585-6405-2-git-send-email-al.stone@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2015?02?25? 08:36, al.stone@linaro.org wrote: > From: Al Stone <al.stone@linaro.org> > > In preparation for later splitting out some of the arch-dependent code from > osl.c, clean up the errors reported by checkpatch.pl. They fell into these > classes: > > -- remove the FSF address from the GPL notice > -- "foo * bar" should be "foo *bar" (and the ** variation of same) > -- a return is not a function, so parentheses are not required. > > Signed-off-by: Al Stone <al.stone@linaro.org> Reviewd-by: Hanjun Guo <hanjun.guo@linaro.org> Thanks Hanjun
On Tuesday, February 24, 2015 05:36:17 PM al.stone@linaro.org wrote: > From: Al Stone <al.stone@linaro.org> > > In preparation for later splitting out some of the arch-dependent code from > osl.c, clean up the errors reported by checkpatch.pl. They fell into these > classes: > > -- remove the FSF address from the GPL notice > -- "foo * bar" should be "foo *bar" (and the ** variation of same) > -- a return is not a function, so parentheses are not required. > > Signed-off-by: Al Stone <al.stone@linaro.org> checkpatch.pl is irrelevant here. You're trying to make the coding style be more consistent with the coding style of the rest of the kernel. The warnings from checkpatch.pl are meaningless for the existing code, so it should not be used to justify changes in that code. Of course, the same applies to patches [2-4/9].
On 03/04/2015 04:04 PM, Rafael J. Wysocki wrote: > On Tuesday, February 24, 2015 05:36:17 PM al.stone@linaro.org wrote: >> From: Al Stone <al.stone@linaro.org> >> >> In preparation for later splitting out some of the arch-dependent code from >> osl.c, clean up the errors reported by checkpatch.pl. They fell into these >> classes: >> >> -- remove the FSF address from the GPL notice >> -- "foo * bar" should be "foo *bar" (and the ** variation of same) >> -- a return is not a function, so parentheses are not required. >> >> Signed-off-by: Al Stone <al.stone@linaro.org> > > checkpatch.pl is irrelevant here. You're trying to make the coding style be > more consistent with the coding style of the rest of the kernel. > > The warnings from checkpatch.pl are meaningless for the existing code, so > it should not be used to justify changes in that code. > > Of course, the same applies to patches [2-4/9]. > > Okay, I'm puzzled. In the last version of these patches, I asked if I should clean up osl.c as long as I was creating the new osi.c file. I understood the reply to mean it would also be good to correct osl.c [0] from checkpatch's point of view. I took that to mean errors (patch [1/9]) and warnings (patches [2-4/9]) -- so that's what I did. What did I misunderstand from that reply? If these changes are objectionable, then I'll drop these from the next version of the patch set; I'm not hung up on insisting on either of the kernel's or ACPI's coding style -- I try to adapt as needed. I only did the patches because I thought it was helping out with some long-term maintenance type work. [0] https://lkml.org/lkml/2015/2/4/749
On 03/04/2015 05:25 PM, Rafael J. Wysocki wrote: > On Wednesday, March 04, 2015 04:56:12 PM Al Stone wrote: >> On 03/04/2015 04:04 PM, Rafael J. Wysocki wrote: >>> On Tuesday, February 24, 2015 05:36:17 PM al.stone@linaro.org wrote: >>>> From: Al Stone <al.stone@linaro.org> >>>> >>>> In preparation for later splitting out some of the arch-dependent code from >>>> osl.c, clean up the errors reported by checkpatch.pl. They fell into these >>>> classes: >>>> >>>> -- remove the FSF address from the GPL notice >>>> -- "foo * bar" should be "foo *bar" (and the ** variation of same) >>>> -- a return is not a function, so parentheses are not required. >>>> >>>> Signed-off-by: Al Stone <al.stone@linaro.org> >>> >>> checkpatch.pl is irrelevant here. You're trying to make the coding style be >>> more consistent with the coding style of the rest of the kernel. >>> >>> The warnings from checkpatch.pl are meaningless for the existing code, so >>> it should not be used to justify changes in that code. >>> >>> Of course, the same applies to patches [2-4/9]. >>> >>> >> >> Okay, I'm puzzled. In the last version of these patches, I asked if I >> should clean up osl.c as long as I was creating the new osi.c file. I >> understood the reply to mean it would also be good to correct osl.c [0] >> from checkpatch's point of view. I took that to mean errors (patch [1/9]) >> and warnings (patches [2-4/9]) -- so that's what I did. What did I >> misunderstand from that reply? >> >> If these changes are objectionable, then I'll drop these from the next >> version of the patch set; I'm not hung up on insisting on either of the >> kernel's or ACPI's coding style -- I try to adapt as needed. I only did >> the patches because I thought it was helping out with some long-term >> maintenance type work. > > The changes are basically OK, but the justification is bogus to me. > "I'm making the chagne, because checkpatch.pl told me so" is a pretty bad > explanation in my view. It is much better to say "This file does not > adhere to the general kernel coding style and since I'm going to split it > into pieces and I want those pieces to follow the coding style more closely, > make changes as follows." > > So this is more about the changelogs (and subjects) than the code changes > themselves. Aha. That makes much more sense to me. Sorry if I was being a bit dense; I'll rev these for the next version so it's far clearer. Thanks for being patient :).
On Wednesday, March 04, 2015 04:56:12 PM Al Stone wrote: > On 03/04/2015 04:04 PM, Rafael J. Wysocki wrote: > > On Tuesday, February 24, 2015 05:36:17 PM al.stone@linaro.org wrote: > >> From: Al Stone <al.stone@linaro.org> > >> > >> In preparation for later splitting out some of the arch-dependent code from > >> osl.c, clean up the errors reported by checkpatch.pl. They fell into these > >> classes: > >> > >> -- remove the FSF address from the GPL notice > >> -- "foo * bar" should be "foo *bar" (and the ** variation of same) > >> -- a return is not a function, so parentheses are not required. > >> > >> Signed-off-by: Al Stone <al.stone@linaro.org> > > > > checkpatch.pl is irrelevant here. You're trying to make the coding style be > > more consistent with the coding style of the rest of the kernel. > > > > The warnings from checkpatch.pl are meaningless for the existing code, so > > it should not be used to justify changes in that code. > > > > Of course, the same applies to patches [2-4/9]. > > > > > > Okay, I'm puzzled. In the last version of these patches, I asked if I > should clean up osl.c as long as I was creating the new osi.c file. I > understood the reply to mean it would also be good to correct osl.c [0] > from checkpatch's point of view. I took that to mean errors (patch [1/9]) > and warnings (patches [2-4/9]) -- so that's what I did. What did I > misunderstand from that reply? > > If these changes are objectionable, then I'll drop these from the next > version of the patch set; I'm not hung up on insisting on either of the > kernel's or ACPI's coding style -- I try to adapt as needed. I only did > the patches because I thought it was helping out with some long-term > maintenance type work. The changes are basically OK, but the justification is bogus to me. "I'm making the chagne, because checkpatch.pl told me so" is a pretty bad explanation in my view. It is much better to say "This file does not adhere to the general kernel coding style and since I'm going to split it into pieces and I want those pieces to follow the coding style more closely, make changes as follows." So this is more about the changelogs (and subjects) than the code changes themselves.
diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 39748bb..1dc3a3b 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -19,10 +19,6 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. * - * You should have received a copy of the GNU General Public License - * along with this program; if not, write to the Free Software - * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA - * * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * */ @@ -526,7 +522,7 @@ EXPORT_SYMBOL(acpi_os_unmap_generic_address); #ifdef ACPI_FUTURE_USAGE acpi_status -acpi_os_get_physical_address(void *virt, acpi_physical_address * phys) +acpi_os_get_physical_address(void *virt, acpi_physical_address *phys) { if (!phys || !virt) return AE_BAD_PARAMETER; @@ -543,7 +539,7 @@ static char acpi_os_name[ACPI_MAX_OVERRIDE_LEN]; acpi_status acpi_os_predefined_override(const struct acpi_predefined_names *init_val, - acpi_string * new_val) + acpi_string *new_val) { if (!init_val || !new_val) return AE_BAD_PARAMETER; @@ -714,8 +710,8 @@ static void acpi_table_taint(struct acpi_table_header *table) acpi_status -acpi_os_table_override(struct acpi_table_header * existing_table, - struct acpi_table_header ** new_table) +acpi_os_table_override(struct acpi_table_header *existing_table, + struct acpi_table_header **new_table) { if (!existing_table || !new_table) return AE_BAD_PARAMETER; @@ -889,7 +885,7 @@ u64 acpi_os_get_timer(void) return time_ns; } -acpi_status acpi_os_read_port(acpi_io_address port, u32 * value, u32 width) +acpi_status acpi_os_read_port(acpi_io_address port, u32 *value, u32 width) { u32 dummy; @@ -1046,7 +1042,7 @@ acpi_os_write_memory(acpi_physical_address phys_addr, u64 value, u32 width) } acpi_status -acpi_os_read_pci_configuration(struct acpi_pci_id * pci_id, u32 reg, +acpi_os_read_pci_configuration(struct acpi_pci_id *pci_id, u32 reg, u64 *value, u32 width) { int result, size; @@ -1074,11 +1070,11 @@ acpi_os_read_pci_configuration(struct acpi_pci_id * pci_id, u32 reg, reg, size, &value32); *value = value32; - return (result ? AE_ERROR : AE_OK); + return result ? AE_ERROR : AE_OK; } acpi_status -acpi_os_write_pci_configuration(struct acpi_pci_id * pci_id, u32 reg, +acpi_os_write_pci_configuration(struct acpi_pci_id *pci_id, u32 reg, u64 value, u32 width) { int result, size; @@ -1101,7 +1097,7 @@ acpi_os_write_pci_configuration(struct acpi_pci_id * pci_id, u32 reg, PCI_DEVFN(pci_id->device, pci_id->function), reg, size, value); - return (result ? AE_ERROR : AE_OK); + return result ? AE_ERROR : AE_OK; } static void acpi_os_execute_deferred(struct work_struct *work) @@ -1247,7 +1243,7 @@ bool acpi_queue_hotplug_work(struct work_struct *work) } acpi_status -acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle * handle) +acpi_os_create_semaphore(u32 max_units, u32 initial_units, acpi_handle *handle) { struct semaphore *sem = NULL; @@ -1735,7 +1731,7 @@ void acpi_os_release_lock(acpi_spinlock lockp, acpi_cpu_flags flags) ******************************************************************************/ acpi_status -acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t ** cache) +acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t **cache) { *cache = kmem_cache_create(name, size, 0, 0, NULL); if (*cache == NULL) @@ -1756,10 +1752,10 @@ acpi_os_create_cache(char *name, u16 size, u16 depth, acpi_cache_t ** cache) * ******************************************************************************/ -acpi_status acpi_os_purge_cache(acpi_cache_t * cache) +acpi_status acpi_os_purge_cache(acpi_cache_t *cache) { kmem_cache_shrink(cache); - return (AE_OK); + return AE_OK; } /******************************************************************************* @@ -1775,10 +1771,10 @@ acpi_status acpi_os_purge_cache(acpi_cache_t * cache) * ******************************************************************************/ -acpi_status acpi_os_delete_cache(acpi_cache_t * cache) +acpi_status acpi_os_delete_cache(acpi_cache_t *cache) { kmem_cache_destroy(cache); - return (AE_OK); + return AE_OK; } /******************************************************************************* @@ -1795,10 +1791,10 @@ acpi_status acpi_os_delete_cache(acpi_cache_t * cache) * ******************************************************************************/ -acpi_status acpi_os_release_object(acpi_cache_t * cache, void *object) +acpi_status acpi_os_release_object(acpi_cache_t *cache, void *object) { kmem_cache_free(cache, object); - return (AE_OK); + return AE_OK; } #endif