From mboxrd@z Thu Jan 1 00:00:00 1970 From: Haavard Skinnemoen Date: Tue, 13 May 2008 18:00:05 +0200 Subject: [U-Boot-Users] [RFC/PATCH] SPI API improvements In-Reply-To: References: <1210346484-1313-1-git-send-email-haavard.skinnemoen@atmel.com> <20080513155055.51cc2619@siona.local> Message-ID: <20080513180005.2ba7de75@siona.local> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Tue, 13 May 2008 17:06:35 +0200 (CEST) Guennadi Liakhovetski wrote: > 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. Ok, so how about we introduce CONFIG_DEFAULT_SPI_IFACE CONFIG_DEFAULT_SPI_MODE so that other platforms can do the same thing too? As for run-time configurability...here's one suggestion: * Allow optional prefix ":" before the chip select number. Use board-specific default if not specified * Add optional parameter specifying the mode at the end. This can be parsed as a 32-bit hexadecimal number so you can specify pretty much anything, but normally you'd just specify 0, 1, 2 or 3, indicating one of the standard SPI modes. Or perhaps we should use environment variables instead...? > > > 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:-) We already have things like spi_xfer() which is implemented differently depending on the SPI driver being used. However, I think we might want a common spi_slave struct for a different reason as well: so that we can pass it to the board hooks -- spi_activate_cs(), spi_deactivate_cs() and spi_cs_is_valid(). We probably need to add a "bus" parameter to the former two anyway, and by passing them a struct, we can easily add new parameters later without having to change prototypes all over the tree... So I guess I'm in favour of your second suggestion -- define a common struct spi_slave which can be embedded into a controller-specific struct. > > > 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 */ Great. I have a few other fixes that I had to make in order for other boards to compile. I've also been doing some testing with DataFlash, and it seems to work fine on my hardware too, at least so far... Haavard