Messages in this thread | | | Date | Wed, 29 Apr 2009 21:23:26 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3 |
| |
* Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 29 Apr 2009 20:22:55 +0200 > Ingo Molnar <mingo@elte.hu> wrote: > > > > + InputAddr = ChannelAddrLong >> 8; > > > + > > > + debugf1(" (ChannelAddrLong=0x%llx) >> 8 becomes " > > > + "InputAddr=0x%x\n", ChannelAddrLong, InputAddr); > > > + > > > + /* Iterate over the DRAM DCTs looking for a > > > + * match for InputAddr on the selected NodeID > > > + */ > > > + CSFound = f10_lookup_addr_in_dct(InputAddr, > > > + NodeID, ChannelSelect); > > > + > > > + if (CSFound >= 0) { > > > + *node_id = NodeID; > > > + *channel_select = ChannelSelect; > > > + } > > > + } > > > + > > > + return CSFound; > > > +} > > > > this function is probably too large, and also it uses some weird > > hungarian notation coding style. Please dont do that! It's > > completely unacceptable. > > These identifers (or at least, DctSelBaseOffsetLong, which is the > only one I googled for) come straight out of the AMD "BIOS and > Kernel Developer's Guide". > > Sucky though they are, there's value in making the kernel code > match up with the documentation.
I'm generally resisting patches that hungarinize arch/x86/ (and heck there's been many attempts ...) but there's some conflicting advice here. I've Cc:-ed Linus, maybe he has an opinion about this.
My gut reaction would be 'hell no'. There's other, structural problems with this code too, and doing some saner naming would mostly be a sed job and would take minimal amount of time. The naming can still be intuitive. The symbols from the documentation can perhaps be mentioned in a couple of comments to establish a mapping.
Ingo
| |