Hi,
On Tue, Jan 05, 2010 at 19:49:19, Tom wrote:
Sudhakar Rajashekhara wrote:
From: Sekhar Nori nsekhar@ti.com
This adds a driver for the SPI controller found on davinci based SoCs from Texas Instruments.
Signed-off-by: Sekhar Nori nsekhar@ti.com Signed-off-by: Sudhakar Rajashekhara sudhakar.raj@ti.com
<snip>
new file mode 100644 index 0000000..c3f1810 --- /dev/null +++ b/drivers/spi/davinci_spi.c @@ -0,0 +1,221 @@ +/*
- Copyright (C) 2009 Sekhar Nori, Texas Instruments, Inc <www.ti.com>
- Driver for SPI controller on DaVinci. Based on atmel_spi.c
- by Atmel Corporation
- Copyright (C) 2007 Atmel Corporation
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#include <common.h> +#include <spi.h> +#include <malloc.h>
+#include <asm/io.h>
+#include <asm/arch/hardware.h>
+#include "davinci_spi.h"
Please remove the extra spaces
I'll remove extra lines between the header file inclusions.
+static unsigned int data1_reg_val;
Why is this static value used instead of reading ds->regs->dat1 ? Depending on the order of the function calling, this value may not mirror what is in the register.
I'll remove the static variable.
+void spi_init() +{
- /* do nothing */
+}
+struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
unsigned int max_hz, unsigned int mode)+{
- struct davinci_spi_slave *ds;
- ds = malloc(sizeof(struct davinci_spi_slave));
- if (!ds)
return NULL;- ds->slave.bus = bus;
- ds->slave.cs = cs;
- ds->regs = (struct davinci_spi_regs *)CONFIG_SYS_SPI_BASE;
- ds->freq = max_hz;
- return &ds->slave;
+}
+void spi_free_slave(struct spi_slave *slave) +{
- struct davinci_spi_slave *ds = to_davinci_spi(slave);
Check before you free. It would be nice if you could poison the pointer.
OK.
- free(ds);
+}
+int spi_claim_bus(struct spi_slave *slave) +{
- struct davinci_spi_slave *ds = to_davinci_spi(slave);
- unsigned int scalar;
- /* Enable the SPI hardware */
- writel(SPIGCR0_SPIRST_MASK, &ds->regs->gcr0);
- udelay(1000);
- writel(SPIGCR0_SPIENA_MASK, &ds->regs->gcr0);
- /* Set master mode, powered up and not activated */
- writel(SPIGCR1_MASTER_MASK | SPIGCR1_CLKMOD_MASK, &ds->regs->gcr1);
- /* CS, CLK, SIMO and SOMI are functional pins */
- writel((SPIPC0_EN0FUN_MASK) | (SPIPC0_CLKFUN_MASK) |
(SPIPC0_DOFUN_MASK) | (SPIPC0_DIFUN_MASK), &ds->regs->pc0);- /* setup format */
- scalar = ((CONFIG_SYS_SPI_CLK / ds->freq) - 1) & 0xFF;
- writel(8 | /* character length */
(scalar << SPIFMT_PRESCALE_SHIFT) |/* clock signal delayed by half clk cycle */(1 << SPIFMT_PHASE_SHIFT) |/* clock low in idle state - Mode 0 */(0 << SPIFMT_POLARITY_SHIFT) |/* MSB shifted out first */(0 << SPIFMT_SHIFTDIR_SHIFT), &ds->regs->fmt0);Shifting 0's.. This could be improved
OK.
- /* hold cs active at end of transfer until explicitly de-asserted */
- data1_reg_val = (1 << SPIDAT1_CSHOLD_SHIFT) |
(slave->cs << SPIDAT1_CSNR_SHIFT);- writel(data1_reg_val, &ds->regs->dat1);
- /*
* Including a minor delay. No science here. Should be good even with* no delay*/- writel((50 << SPI_C2TDELAY_SHIFT) |
(50 << SPI_T2CDELAY_SHIFT), &ds->regs->delay);- /* default chip select register */
- writel(SPIDEF_CSDEF0_MASK, &ds->regs->def);
- /* no interrupts */
- writel(0, &ds->regs->int0);
- writel(0, &ds->regs->lvl);
- /* enable SPI */
- writel(readl(&ds->regs->gcr1) | (SPIGCR1_SPIENA_MASK), &ds->regs->gcr1);
- return 0;
+}
+void spi_release_bus(struct spi_slave *slave) +{
- struct davinci_spi_slave *ds = to_davinci_spi(slave);
- /* Disable the SPI hardware */
- writel(SPIGCR0_SPIRST_MASK, &ds->regs->gcr0);
+}
+int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
const void *dout, void *din, unsigned long flags)+{
- struct davinci_spi_slave *ds = to_davinci_spi(slave);
- unsigned int len;
- int ret, i;
- const u8 *txp = dout;
- u8 *rxp = din;
It is unclear if passing in NULL values for din and dout is normal behaviour. Please add a comment if it is. Also break transfer loop into a tx / rx parts.
NULL values for din and dout is normal and I'll document it. SPI peripheral is a transceiver kind of a peripheral on DaVinci. It requires TX for RX to work. Hence they are coupled like this.
- ret = 0;
- if (bitlen == 0)
/* Finish any previously submitted transfers */goto out;- /*
* It's not clear how non-8-bit-aligned transfers are supposed to be* represented as a stream of bytes...this is a limitation of* the current SPI interface - here we terminate on receiving such a* transfer request.*/- if (bitlen % 8) {
/* Errors always terminate an ongoing transfer */flags |= SPI_XFER_END;It is unclear if you are forcing a flag state that may also be passed in. Please add a comment
SPI_XFER_END flag is being set if bitlen is not 8 bit aligned. This has been documented above.
goto out;- }
- len = bitlen / 8;
- /* do an empty read to clear the current contents */
- readl(&ds->regs->buf);
- /* keep writing and reading 1 byte until done */
- for (i = 0; i < len; i++) {
/* wait till TXFULL is asserted */while (readl(&ds->regs->buf) & (SPIBUF_TXFULL_MASK));/* write the data */data1_reg_val &= ~0xFFFF;if (txp) {Checking for a valid pointer should happen earlier. If an error happens here, a bogus value of the old data1_reg_val will be used. Move the check higher
data1_reg_val |= *txp & 0xFF;Adding 0xff should not be necessary for a u8.
Agree, I'll remove it.
txp++;}<snip>
diff --git a/drivers/spi/davinci_spi.h b/drivers/spi/davinci_spi.h new file mode 100644 index 0000000..5b9a832 --- /dev/null +++ b/drivers/spi/davinci_spi.h @@ -0,0 +1,102 @@ +/*
<snip> > + * Copyright (C) 2009 Sekhar Nori, Texas Instruments, Inc <www.ti.com>
+static inline struct davinci_spi_slave *to_davinci_spi(struct spi_slave *slave) +{
Check before calling
OK.
Thanks for your comments. I'll submit the updated patch soon.
Regards, Sudhakar