[coreboot] [patch][v2] AMD Fam10 memory controller updates

Peter Stuge peter at stuge.se
Mon Apr 7 23:53:38 CEST 2008


On Mon, Apr 07, 2008 at 03:33:20PM -0600, Marc Jones wrote:
> >> -	DQSTiming_D(pMCTstat, pDCTstatA);	/* Get Receiver Enable and DQS signal timing*/
> >> +	DQSTiming_D(pMCTstat, pDCTstatA);	/* Get Receiver Enable and DQS signal timing */
> >> +
> >> +	print_t("mctAutoInitMCT_D: UMAMemTyping_D\n");
> >> +	UMAMemTyping_D(pMCTstat, pDCTstatA);	/* Fix up for UMA sizing */
> >
> > Please keep code changes and whitespace changes apart. In particular
> > when changing things like these (that are not very well known) I
> > think that is important.
> 
> Serious?

Yes, afraid so.


> You are naking over a couple whitespace changes that get fixed
> as the code is fixed?

Right.


> Do you have concerns that the patch is confused by the changes

Yes, I think it is.


> or are you just naking on principal?

No, not really.


This is how I reason:

Ideally whitespace changes should not be needed, all commits should
always be formatted perfectly.

In practice this isn't true, and whitespace changes are a good thing.
I don't care all that much about whitespace formatting myself, but
others do and I agree it does make the code as a whole look more
tidy.

But, since the basic assumption is that commits change code and
nothing else, I think whitespace changes should be isolated commits,
since they clutter code diffs and make human diff processing more
difficult.

I think this argument is even more important when code is changed
that is not very well-known and well understood.


//Peter




More information about the coreboot mailing list