[U-Boot-Users] [RFC/PATCH] SPI API improvements
Haavard Skinnemoen
haavard.skinnemoen at atmel.com
Tue May 13 15:50:55 CEST 2008
On Tue, 13 May 2008 13:20:22 +0200 (CEST)
Guennadi Liakhovetski <lg at denx.de> wrote:
> > static int device;
> > static int bitlen;
> > static uchar dout[MAX_SPI_BYTES];
> > static uchar din[MAX_SPI_BYTES];
> > +static struct spi_slave *slave;
>
> Don't think this is needed...
Right.
> > + /* 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.
> > 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.
What I would really like here is a rtc_init() call which sets up the
slave parameters once and for all. In the mean time, we'll have to
either do the initialization on demand and keep it around, or do the
initialization every time and take it down afterwards. I don't really
have strong feelings one way or another, but I don't think consistency
with the "spi" command is a very useful goal.
> > reg = 0x2c000000 | day | 0x80000000;
> > spi_xfer(0, 32, (uchar *)®, (uchar *)&day);
>
> You changed spi_xfer()'s prototype, but didn't change all calls.
Right, I'll fix it.
> > @@ -132,15 +125,15 @@ void spi_init(void)
> > {
> > }
> >
> > -int spi_select(unsigned int bus, unsigned int dev, unsigned long mode)
> > +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> > + unsigned int max_hz, unsigned int mode)
>
> You changed the parameter name from dev to cs
>
> > {
> > unsigned int ctrl_reg;
> > + struct spi_slave *slave;
> >
> > if (bus >= sizeof(spi_bases) / sizeof(spi_bases[0]) ||
> > dev > 3)
>
> but you didn't change it here
>
> > - return 1;
> > -
> > - spi_base = spi_bases[bus];
> > + return NULL;
> >
> > ctrl_reg = MXC_CSPICTRL_CHIPSELECT(dev) |
> > MXC_CSPICTRL_BITCOUNT(31) |
>
> and here.
Fixed.
> > -/*
> > - * 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?
While I'm not fanatical about keeping the spi_slave layout hidden from
its users, I do think this offers some potential optimizations in the
controller driver (i.e. instead of storing a bus number, you can store
a direct pointer to its register space.) I'm very concerned about
keeping this as lightweight as possible, so unless defining a common
spi_slave layout offers any actual advantages, I'd rather leave all of
it up to the controller driver.
> 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.
Sorry about pushing this untested code on everyone, but I wanted to get
it out early so I could get some feedback on the interface. I did
_attempt_ to get it right so that you could see how it all fits
together, but I did expect some breakage.
I'll do some more thorough testing during the next couple of days, but
I'd like to know whether or not you all think this whole thing looks
reasonable, given that the breakage gets sorted out.
Now, I guess I should start compile-testing powerpc...that's a bit
problematic since practically no configurations build out of the box,
and nobody seems to want the absolutely trivial fix I posted half a
year ago...
> I only reviewed the parts I'd written or changed myself.
Thanks for the feedback.
Haavard
diff --git a/common/cmd_spi.c b/common/cmd_spi.c
index b0e7db1..15b2811 100644
--- a/common/cmd_spi.c
+++ b/common/cmd_spi.c
@@ -44,7 +44,6 @@ static int device;
static int bitlen;
static uchar dout[MAX_SPI_BYTES];
static uchar din[MAX_SPI_BYTES];
-static struct spi_slave *slave;
/*
* SPI read/write
@@ -111,7 +110,8 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
debug ("spi chipsel = %08X\n", device);
spi_claim_bus(slave);
- if(spi_xfer(device, bitlen, dout, din) != 0) {
+ if(spi_xfer(slave, bitlen, dout, din,
+ SPI_XFER_BEGIN | SPI_XFER_END) != 0) {
printf("Error with the SPI transaction.\n");
rcode = 1;
} else {
diff --git a/drivers/rtc/mc13783-rtc.c b/drivers/rtc/mc13783-rtc.c
index 5a1ef4f..b6e1501 100644
--- a/drivers/rtc/mc13783-rtc.c
+++ b/drivers/rtc/mc13783-rtc.c
@@ -45,19 +45,22 @@ int rtc_get(struct rtc_time *rtc)
do {
reg = 0x2c000000;
- err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day1);
+ err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day1,
+ SPI_XFER_BEGIN | SPI_XFER_END);
if (err)
return err;
reg = 0x28000000;
- err = spi_xfer(0, 32, (uchar *)®, (uchar *)&time);
+ err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&time,
+ SPI_XFER_BEGIN | SPI_XFER_END);
if (err)
return err;
reg = 0x2c000000;
- err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day2);
+ err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day2,
+ SPI_XFER_BEGIN | SPI_XFER_END);
if (err)
return err;
@@ -95,10 +98,12 @@ void rtc_set(struct rtc_time *rtc)
return;
reg = 0x2c000000 | day | 0x80000000;
- spi_xfer(0, 32, (uchar *)®, (uchar *)&day);
+ spi_xfer(slave, 32, (uchar *)®, (uchar *)&day,
+ SPI_XFER_BEGIN | SPI_XFER_END);
reg = 0x28000000 | time | 0x80000000;
- spi_xfer(0, 32, (uchar *)®, (uchar *)&time);
+ spi_xfer(slave, 32, (uchar *)®, (uchar *)&time,
+ SPI_XFER_BEGIN | SPI_XFER_END);
spi_release_bus(slave);
}
diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c
index 8279e3e..435d64a 100644
--- a/drivers/spi/mxc_spi.c
+++ b/drivers/spi/mxc_spi.c
@@ -19,6 +19,7 @@
*/
#include <common.h>
+#include <malloc.h>
#include <spi.h>
#include <asm/io.h>
@@ -132,10 +133,10 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
struct spi_slave *slave;
if (bus >= sizeof(spi_bases) / sizeof(spi_bases[0]) ||
- dev > 3)
+ cs > 3)
return NULL;
- ctrl_reg = MXC_CSPICTRL_CHIPSELECT(dev) |
+ ctrl_reg = MXC_CSPICTRL_CHIPSELECT(cs) |
MXC_CSPICTRL_BITCOUNT(31) |
MXC_CSPICTRL_DATARATE(7) | /* FIXME: calculate data rate */
MXC_CSPICTRL_EN |
More information about the U-Boot
mailing list