Re: [PATCH 1/3] Add Numachip APIC support

From: Ingo Molnar
Date: Thu Oct 27 2011 - 03:46:30 EST

* Daniel J Blueman <daniel@xxxxxxxxxxxxxxxxxx> 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 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:


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 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

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.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at