From: Ingo Molnar <mingo@elte.hu>
To: Daniel J Blueman <daniel@numascale-asia.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>, H Peter Anvin <hpa@zytor.com>,
Steffen Persvold <sp@numascale.com>,
linux-kernel@vger.kernel.org, x86@kernel.org
Subject: Re: [PATCH 1/3] Add Numachip APIC support
Date: Thu, 27 Oct 2011 09:44:49 +0200 [thread overview]
Message-ID: <20111027074449.GF15923@elte.hu> (raw)
In-Reply-To: <1319609230-18897-1-git-send-email-daniel@numascale-asia.com>
* Daniel J Blueman <daniel@numascale-asia.com> wrote:
> + numachip_write_local_csr(NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN,
> + int_gen.v);
> + numachip_write_local_csr(NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN,
> + int_gen.v);
> + numachip_write_local_csr(NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN,
> + int_gen.v);
Please don't break the line in an ugly way like that just to placate
checkpatch.pl.
checkpatch.pl is right to complain about the too long line here - but
your fix is wrong: the proper approach is to make the code *shorter*.
So for example instead of numachip_write_local_csr() you could
abbreviate it to something obvious such as:
write_lcsr()
it's not like these will be used outside of the Numachip code, so
there's no confusion from the missing numachip_ prefix.
write_gcsr() can be used for the global registers.
Likewise, NUMACHIP_CSR_G3_EXT_INTERRUPT_GEN is way too long as well -
you can easily rename it to CSR_G3_EXT_IRQ_GEN or such, without
losing expressive value. Then it could be written in the much shorter
form of:
write_lcsr(CSR_G3_EXT_IRQ_GEN, irq_gen.v);
... and magically checkpatch.pl won't complain anymore.
Likewise there's ton's of other way too long names in the patch as
well - please try to abbreviate them wherever that can be done
without losing expressive value.
Note, we don't want to over-do it: we obviously don't want to shorten
names to CSRG3EIG kind of gibberish, so *some* length is of course
acceptable.
When a line becomes too long then leave it too long instead of
breaking it - line length up to say 100 cols in width are still OK,
as long as the surrounding code does not have obvious deficiencies
like too much complexity.
Note, while the merge window is in progress already, i think we can
still add these patches slightly outside the merge window as well, as
they add new hardware support and don't risk regressions elsewhere -
if all the structural and fine-detail problems i pointed out during
review are fixed.
Thanks,
Ingo
next prev parent reply other threads:[~2011-10-27 7:46 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-10-26 6:07 [PATCH 1/3] Add Numachip APIC support Daniel J Blueman
2011-10-26 6:07 ` [PATCH 2/3] Add multi-node boot support Daniel J Blueman
2011-10-27 7:30 ` Ingo Molnar
2011-10-27 10:46 ` Steffen Persvold
2011-10-27 11:38 ` Ingo Molnar
2011-10-27 11:42 ` Steffen Persvold
2011-12-02 18:31 ` Daniel J Blueman
2011-12-05 8:20 ` [PATCH 1/3] Make flat_init_apic_ldr available Daniel J Blueman
2011-12-05 8:20 ` [PATCH 2/3] v2: Add x86_init platform override to fix up core numbering Daniel J Blueman
2011-12-05 17:58 ` [tip:x86/apic] x86: Add x86_init platform override to fix up NUMA " tip-bot for Daniel J Blueman
2011-12-06 6:30 ` [tip:x86/apic] x86: Fix the !CONFIG_NUMA build of the new CPU ID fixup code support tip-bot for Steffen Persvold
2011-12-05 8:20 ` [PATCH 3/3] v4: Add support for Numascale's NumaChip Daniel J Blueman
2011-12-05 9:10 ` Ingo Molnar
2011-12-05 9:49 ` Steffen Persvold
2011-12-05 16:07 ` [PATCH 3/3] v5: Add NumaChip support Daniel J Blueman
2011-12-05 17:59 ` [tip:x86/apic] x86: " tip-bot for Steffen Persvold
2011-12-05 20:31 ` Ingo Molnar
2011-12-06 0:10 ` Steffen Persvold
2011-12-06 5:50 ` Ingo Molnar
2011-12-06 6:09 ` Steffen Persvold
2011-12-08 22:35 ` Kevin Winchester
2011-12-09 1:24 ` Steffen Persvold
2011-12-09 7:22 ` Ingo Molnar
2011-12-10 1:52 ` Kevin Winchester
2011-12-10 2:28 ` Kevin Winchester
2011-12-18 9:44 ` Ingo Molnar
2011-12-19 0:12 ` [PATCH] x86: Simplify code by allowing more of struct cpuinfo_x86 for the !SMP case Kevin Winchester
2011-12-19 14:07 ` [tip:x86/apic] " tip-bot for Kevin Winchester
2011-12-19 18:47 ` Ingo Molnar
2011-12-21 0:52 ` [PATCH] " Kevin Winchester
2011-12-21 8:48 ` [tip:x86/apic] x86: Simplify code by removing a !SMP #ifdefs from 'struct cpuinfo_x86' tip-bot for Kevin Winchester
2011-12-05 17:57 ` [tip:x86/apic] x86: Make flat_init_apic_ldr() available tip-bot for Daniel J Blueman
2011-10-26 6:07 ` [PATCH 3/3] Add NumaChip quirk Daniel J Blueman
2011-10-27 7:26 ` [PATCH 1/3] Add Numachip APIC support Ingo Molnar
2011-10-27 7:35 ` Ingo Molnar
2011-10-27 7:44 ` Ingo Molnar [this message]
2011-10-27 10:31 ` Steffen Persvold
-- strict thread matches above, loose matches on Subject: below --
2011-10-18 8:22 Daniel J Blueman
2011-07-22 10:44 Daniel J Blueman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20111027074449.GF15923@elte.hu \
--to=mingo@elte.hu \
--cc=daniel@numascale-asia.com \
--cc=hpa@zytor.com \
--cc=jbarnes@virtuousgeek.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@redhat.com \
--cc=sp@numascale.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.