[U-Boot] [PATCH] Improve DaVinci SPI speed

[Delio Brignoli]

Hello Wolfgang,

On 21/05/2010, at 15:13, Wolfgang Denk wrote:
>> +				*rxp = buf_reg_val & 0xFF;	
>> +				rxp++;
>> +			}
> 
> Please change into:
> 
> 			if (rxp)
> 				*rxp++ = buf_reg_val & 0xFF;

Are you sure? Is folding that 3 line block into a one liner really worth the loss of readability? I know what that means, so do you and many others on this ML, but that's bound to raise a few eyebrows. A quick google search reveals quite a few articles devoted to clearing up the confusion about the semantics of that pointer dereference and post-increment. Just a few years ago I would have preferred your suggestion, but now I find that readability trumps cleverness, at least in a case like this. In the end it's your decision: if you like to be kinky, it's your prerogative ;-)

> 
>> +		if ((o_cnt < len) && ((buf_reg_val & SPIBUF_TXFULL_MASK) == 0)) {
>> +			/* write the data */
>> +			data1_reg_val &= ~0xFFFF;
>> +			if(txp) {
>> +				data1_reg_val |= *txp;
>> +				txp++;
>> +			}
> 
> Same here:
> 
> 			if (txp)
> 				data1_reg_val |= *txp++;

See my comment above.

> Please fix all other occurrences of "if(" into "if (" as well.

OK

> 
>> +			/* write to DAT1 is required to keep the serial transfer going */
>> +			/* we just terminate when we reach the end */

will fix

>> +				writel(data1_reg_val &
>> +					~(1 << SPIDAT1_CSHOLD_SHIFT), &ds->regs->dat1);
> 
> Line too long.

ditto

Thanks for the feedback
--
Delio

> 
> 
> 
> 
> Best regards,
> 
> Wolfgang Denk
> 
> -- 
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> The software required `Windows 95 or better', so I installed Linux.