Messages in this thread | | | Date | Sat, 8 Jun 2013 11:32:54 +0800 | From | Kefeng Wang <> | Subject | Re: [PATCH 0/7] irq: fix checkpatch errors and warnings |
| |
On 2013-06-08 6:24, Joe Perches wrote: > On Fri, 2013-06-07 at 22:42 +0200, Thomas Gleixner wrote: >> On Thu, 6 Jun 2013, Kefeng Wang wrote: >> >>> Fix all the checkpath errors in kernel/irq dir, and some warnings >>> also fixed. >> >> Sorry, I'm not really interested in this kind of patches. To be >> honest, it would be way more exciting if you had taught checkpatch to >> actually fix the missing space after the comma. > > Yup. Kefeng, if you want to take that up, > I'd be happy to help/guide you. > >> Aside of that your mechanical fixups are mostly making the code worse >> to read. Just a few examples: >> >> --- linux-2.6.orig/kernel/irq/chip.c > [] >> struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, >> IRQ_GET_DESC_CHECK_GLOBAL); >> >> Aligning the first arguments of the first and the second line makes it >> way simpler to read. > > or maybe use > > struct irq_desc *desc = > irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL); > > and satisfy 80 cols too. > > Existing uses that exceed the 80 column text limit shouldn't > be changed without making other useful changes at the same > time. Make 80 column changes as part of a larger patch set, > >> --- linux-2.6.orig/kernel/irq/handle.c > [] >> @@ -47,7 +47,7 @@ static void warn_no_thread(unsigned int > >> - printk(KERN_WARNING "IRQ %d device %s returned IRQ_WAKE_THREAD " >> + pr_warn("IRQ %d device %s returned IRQ_WAKE_THREAD " >> "but no thread function available.", irq, action->name); > > It'd be useful to add a terminating \n though to avoid > interleaving by other thread pr_cont/naked printks. > >> The checkpatch warning is to prevent new code to use the old style >> printk format, but it's not intended to force that on existing code. > > Yup, and checkpatch will never be a perfect tool. > >> Please sit down and read and understand the code and try to find real >> issues which cannot be detected and solved by scripts and >> tools. That's what's kernel hacking is about. You are not becoming a >> kernel developer by running tools and blindly fixing the complaints of >> the tools. You have to understand the code and you have to learn to >> judge the output of tools. > > Quite right. > > Maybe add something like it to SubmittingPatches in > Section 2 ala: > > 5) "Don't be a checkpatch monkey - How not to write patches" >
hi Thomas and Joe, thank very much for your reply and remind, I will pay more attention to study kernel, like your said, read more code and be an good kernel developer.
thanks, kefeng
> -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > . >
-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |