[U-Boot-Users] [RFC/PATCH 2/6] SPI API improvements

[Guennadi Liakhovetski]

On Fri, 16 May 2008, Haavard Skinnemoen wrote:

> From: Haavard Skinnemoen <hskinnemoen at atmel.com>
> 
> This patch gets rid of the spi_chipsel table and adds a handful of new
> functions that makes the SPI layer cleaner and more flexible.

Ok, this looks good to me now. And it works too. Just one question:

> diff --git a/drivers/rtc/mc13783-rtc.c b/drivers/rtc/mc13783-rtc.c
> index 35b1b8b..b6e1501 100644
> --- a/drivers/rtc/mc13783-rtc.c
> +++ b/drivers/rtc/mc13783-rtc.c
> @@ -24,34 +24,50 @@
>  #include <rtc.h>
>  #include <spi.h>
>  
> +static struct spi_slave *slave;
> +
>  int rtc_get(struct rtc_time *rtc)
>  {
>  	u32 day1, day2, time;
>  	u32 reg;
>  	int err, tim, i = 0;
>  
> -	spi_select(1, 0, SPI_MODE_2 | SPI_CS_HIGH);
> +	if (!slave) {
> +		/* FIXME: Verify the max SCK rate */
> +		slave = spi_setup_slave(1, 0, 1000000,
> +				SPI_MODE_2 | SPI_CS_HIGH);
> +		if (!slave)
> +			return -1;
> +	}
> +
> +	if (spi_claim_bus(slave))
> +		return -1;
>  
>  	do {
>  		reg = 0x2c000000;
> -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day1);
> +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day1,
> +				SPI_XFER_BEGIN | SPI_XFER_END);
>  
>  		if (err)
>  			return err;
>  
>  		reg = 0x28000000;
> -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&time);
> +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&time,
> +				SPI_XFER_BEGIN | SPI_XFER_END);
>  
>  		if (err)
>  			return err;
>  
>  		reg = 0x2c000000;
> -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day2);
> +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day2,
> +				SPI_XFER_BEGIN | SPI_XFER_END);

Here... We perform 3 transfers on SPI one after another, and every time we 
do "SPI_XFER_BEGIN | SPI_XFER_EN"... Doesn't this defeat the whole purpose 
of these flags? Would it be bad, if we did

>  		reg = 0x2c000000;
> -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day1);
> +		err = spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day1,
> +				SPI_XFER_BEGIN);
> - 
> - 		if (err)
> - 			return err;
>  
>  		reg = 0x28000000;
> -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&time);
> +		err |= spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&time, 0);
> - 
> - 		if (err)
> - 			return err;
>  
>  		reg = 0x2c000000;
> -		err = spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day2);
> +		err |= spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day2,
> +				SPI_XFER_END);
>  
>  		if (err)
>  			return err;

? The worst that can happen with this, is that the first or the second 
transfer return an error, and we go on with one or two more transfers 
instead of aborting immediately. Can this have any negative effects?

> @@ -65,16 +81,31 @@ void rtc_set(struct rtc_time *rtc)
>  {
>  	u32 time, day, reg;
>  
> +	if (!slave) {
> +		/* FIXME: Verify the max SCK rate */
> +		slave = spi_setup_slave(1, 0, 1000000,
> +				SPI_MODE_2 | SPI_CS_HIGH);
> +		if (!slave)
> +			return;
> +	}
> +
>  	time = mktime(rtc->tm_year, rtc->tm_mon, rtc->tm_mday,
>  		      rtc->tm_hour, rtc->tm_min, rtc->tm_sec);
>  	day = time / 86400;
>  	time %= 86400;
>  
> +	if (spi_claim_bus(slave))
> +		return;
> +
>  	reg = 0x2c000000 | day | 0x80000000;
> -	spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day);
> +	spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&day,
> +			SPI_XFER_BEGIN | SPI_XFER_END);
>  
>  	reg = 0x28000000 | time | 0x80000000;
> -	spi_xfer(0, 32, (uchar *)&reg, (uchar *)&time);
> +	spi_xfer(slave, 32, (uchar *)&reg, (uchar *)&time,
> +			SPI_XFER_BEGIN | SPI_XFER_END);
> +
> +	spi_release_bus(slave);
>  }
>  
>  void rtc_reset(void)

Here error is not checked at all... So, it should be no problem doing only 
SPI_XFER_BEGIN in the first xfer and only SPI_XFER_END in the second one.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de