On Tue, 13 May 2008, Haavard Skinnemoen wrote:
> On Tue, 13 May 2008 13:20:22 +0200 (CEST)
> Guennadi Liakhovetski <[EMAIL PROTECTED]> 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: [EMAIL PROTECTED]
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
U-Boot-Users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/u-boot-users
