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

[Guennadi Liakhovetski]

On Tue, 13 May 2008, Haavard Skinnemoen wrote:

> On Tue, 13 May 2008 13:20:22 +0200 (CEST)
> Guennadi Liakhovetski <lg at denx.de> wrote:
> 
> > > +	/* FIXME: Make these parameters configurable */
> > > +	slave = spi_setup_slave(0, device, 1000000, SPI_MODE_0);
> > 
> > Until it is configurable (I think, you mean at runtime), please use the 
> > CONFIG_MXC_SPI_IFACE macro, otherwise it will be useless on mx31ads.
> 
> Do you really think platform-specific macros are appropriate here?
> 
> But yeah, I did mean at runtime. If we're going to support multiple
> busses, we need to expose that at the user interface level too.

Appropriate or not from the esthetic PoV, I don't see another chance to 
make it useful - either make it run-time configurable either via command 
parameters, or environment varables, ot at least compile-time, so 
platforms could specify something meaningful there. BTW, same holds for 
the flags. So, I'd do

#ifndef CONFIG_MXC_SPI_IFACE
#define CONFIG_MXC_SPI_IFACE 0
#endif
#ifndef CONFIG_MXC_SPI_MODE
#define CONFIG_MXC_SPI_MODE SPI_MODE_0
#endif

	slave = spi_setup_slave(CONFIG_MXC_SPI_IFACE, device, 1000000, CONFIG_MXC_SPI_MODE);

Without these sspi is useless on mx31ads.

> > > diff --git a/drivers/rtc/mc13783-rtc.c b/drivers/rtc/mc13783-rtc.c
> > > index 35b1b8b..5a1ef4f 100644
> > > --- a/drivers/rtc/mc13783-rtc.c
> > > +++ b/drivers/rtc/mc13783-rtc.c
> > > @@ -24,13 +24,24 @@
> > >  #include <rtc.h>
> > >  #include <spi.h>
> > >  
> > > +static struct spi_slave *slave;
> > > +
> > 
> > In do_spi() you use a local variable, allocate a slave, claim the bus, 
> > xfer data, release the bus and free the slave on each call, which is also 
> > nice. Whereas, for example, in this driver you use a static variable, 
> > allocate a slave for it once, and, naturally, never free it. This is at 
> > least inconsistent, IMHO.
> 
> I don't think it's inconsistent...they're very different users. While
> this RTC driver will normally only ever see one slave, do_spi() needs
> to be able to communicate with whatever device the user tells it to,
> which may be different from one call to the next.

Ok, sorry, you're right.

> > > -/*
> > > - * The function call pointer type used to drive the chip select.
> > > - */
> > > -typedef void (*spi_chipsel_type)(int cs);
> > > +/* SPI transfer flags */
> > > +#define SPI_XFER_BEGIN	0x01			/* Assert CS before transfer */
> > > +#define SPI_XFER_END	0x02			/* Deassert CS after transfer */
> > >  
> > > +/* Driver-specific SPI slave data */
> > > +struct spi_slave;
> > 
> > Well, I am a bit upset you decided to make struct spi_slave 
> > hardware-specific. I hoped it would be standard, and contain a void * to 
> > hardware-specific part. Or, better yet, be embeddable in hardware-specific 
> > object, so drivers then would use container_of to get to their data and 
> > wouldn't need to malloc 2 structs. But, as you say, it is not an operating 
> > system, so, let's see what others say.
> 
> Instead of being upset, could you tell me what kind of information such
> a hardware-independent spi_slave struct should have, and why it might be
> useful outside the controller driver?

Well, I just don't like different things being called with the same 
name:-)

> > After all above are fixed, and I can at least compile it again:-), I'll 
> > test it on hardware.
> 
> With the below patch, it compiles on imx31_litekit at least.

and it works too - rtc works, sspi works with above modifications and 
setting

#define CONFIG_MXC_SPI_MODE	(SPI_MODE_2 | SPI_CS_HIGH)	/* Default SPI mode */

in mx31ads.h

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