Re: [PATCH 13/21] amd64_edac: add f10-and-later methods-p3

[Andrew Morton]

On Wed, 29 Apr 2009 21:23:26 +0200
Ingo Molnar <mingo@xxxxxxx> wrote:

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

I think I disagree.  For those identifiers which map 1:1 with the
manufacturer's document, the ugliness involved in exactly copying the
manufacturer's chosen identifiers is outweighed by the benefit of
exactly copying the manufacturer's chosen identifiers.

Of course, we don't have to use StinkyIdentifiers anywhere else.  And
the nice thing about that is that when one reads the code and comes
across a StinkyIdentifier, one immeditely knows that it's an
AMD-provided thing rather than a Linux-provided thing.

Zillions of StinkyIdentifiers get merged via this logic.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/