* [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller
@ 2010-01-05 4:47 Sudhakar Rajashekhara
2010-01-05 9:52 ` Nick Thompson
2010-01-05 14:19 ` Tom
0 siblings, 2 replies; 5+ messages in thread
From: Sudhakar Rajashekhara @ 2010-01-05 4:47 UTC (permalink / raw)
To: u-boot
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>
---
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller
2010-01-05 4:47 [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
@ 2010-01-05 9:52 ` Nick Thompson
2010-01-05 13:29 ` Sudhakar Rajashekhara
2010-01-05 14:19 ` Tom
1 sibling, 1 reply; 5+ messages in thread
From: Nick Thompson @ 2010-01-05 9:52 UTC (permalink / raw)
To: u-boot
On 05/01/10 04:47, 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>
> ---
> From the previous version following have been modified:
> 1. Sorted the entries in drivers/spi/Makefile alphabetically.
> 2. Implemented dummy functions for spi_cs_is_valid(),
> spi_cs_activate() and spi_cs_deactivate().
> 3. Added GPL header for drivers/spi/davinci_spi.h file.
> 4. Added protection against multiple inclusion of SPI header
> file.
> 5. Replaced the macro based register offsets in SPI header
> file with structure.
> 6. Replaced the spi_readl and spi_writel functions with
> readl and writel respectively.
>
> drivers/spi/Makefile | 1 +
> drivers/spi/davinci_spi.c | 221 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/spi/davinci_spi.h | 102 ++++++++++++++++++++
> include/configs/da830evm.h | 2 +-
No sign of this file in the patch set. Is this intentional?
> 4 files changed, 325 insertions(+), 1 deletions(-)
> create mode 100644 drivers/spi/davinci_spi.c
> create mode 100644 drivers/spi/davinci_spi.h
...
> diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
> new file mode 100644
> index 0000000..c3f1810
> --- /dev/null
> +++ b/drivers/spi/davinci_spi.c
> @@ -0,0 +1,221 @@
...
> +
> + /* CS, CLK, SIMO and SOMI are functional pins */
> + writel((SPIPC0_EN0FUN_MASK) | (SPIPC0_CLKFUN_MASK) |
> + (SPIPC0_DOFUN_MASK) | (SPIPC0_DIFUN_MASK), &ds->regs->pc0);
There seem to be numerous cases, here and elsewhere in the file where bare defines
are referenced within parenthesis for no obvious reason. If they are needed they
should be in the #define statement. I think in all cases they are, so the above
lines should be something like...
/* CS, CLK, SIMO and SOMI are functional pins */
writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK |
SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), &ds->regs->pc0);
...which also corrects the alignment of the last line.
Regards,
Nick.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller
2010-01-05 9:52 ` Nick Thompson
@ 2010-01-05 13:29 ` Sudhakar Rajashekhara
0 siblings, 0 replies; 5+ messages in thread
From: Sudhakar Rajashekhara @ 2010-01-05 13:29 UTC (permalink / raw)
To: u-boot
On Tue, Jan 05, 2010 at 15:22:39, Nick Thompson wrote:
> On 05/01/10 04:47, 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>
> > ---
> > From the previous version following have been modified:
> > 1. Sorted the entries in drivers/spi/Makefile alphabetically.
> > 2. Implemented dummy functions for spi_cs_is_valid(),
> > spi_cs_activate() and spi_cs_deactivate().
> > 3. Added GPL header for drivers/spi/davinci_spi.h file.
> > 4. Added protection against multiple inclusion of SPI header
> > file.
> > 5. Replaced the macro based register offsets in SPI header
> > file with structure.
> > 6. Replaced the spi_readl and spi_writel functions with
> > readl and writel respectively.
> >
> > drivers/spi/Makefile | 1 +
> > drivers/spi/davinci_spi.c | 221 ++++++++++++++++++++++++++++++++++++++++++++
> > drivers/spi/davinci_spi.h | 102 ++++++++++++++++++++
> > include/configs/da830evm.h | 2 +-
>
> No sign of this file in the patch set. Is this intentional?
>
My mistake. I'll correct it.
> > 4 files changed, 325 insertions(+), 1 deletions(-)
> > create mode 100644 drivers/spi/davinci_spi.c
> > create mode 100644 drivers/spi/davinci_spi.h
>
> ...
>
> > diff --git a/drivers/spi/davinci_spi.c b/drivers/spi/davinci_spi.c
> > new file mode 100644
> > index 0000000..c3f1810
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.c
> > @@ -0,0 +1,221 @@
>
> ...
>
> > +
> > + /* CS, CLK, SIMO and SOMI are functional pins */
> > + writel((SPIPC0_EN0FUN_MASK) | (SPIPC0_CLKFUN_MASK) |
> > + (SPIPC0_DOFUN_MASK) | (SPIPC0_DIFUN_MASK), &ds->regs->pc0);
>
> There seem to be numerous cases, here and elsewhere in the file where bare defines
> are referenced within parenthesis for no obvious reason. If they are needed they
> should be in the #define statement. I think in all cases they are, so the above
> lines should be something like...
>
> /* CS, CLK, SIMO and SOMI are functional pins */
> writel((SPIPC0_EN0FUN_MASK | SPIPC0_CLKFUN_MASK |
> SPIPC0_DOFUN_MASK | SPIPC0_DIFUN_MASK), &ds->regs->pc0);
>
> ...which also corrects the alignment of the last line.
>
I'll remove such things and re-submit the patch.
Regards,
Sudhakar
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller
2010-01-05 4:47 [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
2010-01-05 9:52 ` Nick Thompson
@ 2010-01-05 14:19 ` Tom
2010-01-06 11:26 ` Sudhakar Rajashekhara
1 sibling, 1 reply; 5+ messages in thread
From: Tom @ 2010-01-05 14:19 UTC (permalink / raw)
To: u-boot
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>
> ---
> From the previous version following have been modified:
> 1. Sorted the entries in drivers/spi/Makefile alphabetically.
> 2. Implemented dummy functions for spi_cs_is_valid(),
> spi_cs_activate() and spi_cs_deactivate().
> 3. Added GPL header for drivers/spi/davinci_spi.h file.
> 4. Added protection against multiple inclusion of SPI header
> file.
> 5. Replaced the macro based register offsets in SPI header
> file with structure.
> 6. Replaced the spi_readl and spi_writel functions with
> readl and writel respectively.
>
> drivers/spi/Makefile | 1 +
> drivers/spi/davinci_spi.c | 221 ++++++++++++++++++++++++++++++++++++++++++++
> drivers/spi/davinci_spi.h | 102 ++++++++++++++++++++
> include/configs/da830evm.h | 2 +-
> 4 files changed, 325 insertions(+), 1 deletions(-)
> create mode 100644 drivers/spi/davinci_spi.c
> create mode 100644 drivers/spi/davinci_spi.h
>
<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
> +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.
> +
> +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.
> + 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
> +
> + /* 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.
> +
> + 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
> + 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.
> + 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
> + return container_of(slave, struct davinci_spi_slave, slave);
> +}
> +
> +#endif /* _DAVINCI_SPI_H_ */
Tom
^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller
2010-01-05 14:19 ` Tom
@ 2010-01-06 11:26 ` Sudhakar Rajashekhara
0 siblings, 0 replies; 5+ messages in thread
From: Sudhakar Rajashekhara @ 2010-01-06 11:26 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-01-06 11:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-05 4:47 [U-Boot] [PATCH v2] TI DaVinci: Driver for the davinci SPI controller Sudhakar Rajashekhara
2010-01-05 9:52 ` Nick Thompson
2010-01-05 13:29 ` Sudhakar Rajashekhara
2010-01-05 14:19 ` Tom
2010-01-06 11:26 ` Sudhakar Rajashekhara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox