> -----Original Message-----
> From: Sudhakar Rajashekhara [mailto:[email protected]]
> Sent: Thursday, July 30, 2009 12:30 AM
> To: Paulraj, Sandeep; [email protected]
> Subject: RE: [PATCH 1/6] DaVinci: SPI Driver for DaVinci and DA8xx SOC's
> 
> Sandeep,
> 
> On Thu, Jul 16, 2009 at 02:08:02, [email protected] wrote:
> > From: Sandeep Paulraj <[email protected]>
> >
> > The patch adds support for SPI driver in DaVinci series
> > SOC's.
> >
> > Signed-off-by: Sandeep Paulraj <[email protected]>
> > ---
> >  arch/arm/mach-davinci/include/mach/spi.h |   45 ++
> >  drivers/spi/Kconfig                      |    7 +
> >  drivers/spi/Makefile                     |    1 +
> >  drivers/spi/davinci_spi.c                |  751
> ++++++++++++++++++++++++++++++
> >  drivers/spi/davinci_spi.h                |  163 +++++++
> >  5 files changed, 967 insertions(+), 0 deletions(-)
> >  create mode 100644 arch/arm/mach-davinci/include/mach/spi.h
> >  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..b1ed270
> > --- /dev/null
> > +++ b/drivers/spi/davinci_spi.c
> > @@ -0,0 +1,751 @@
> > +/*
> > + * Copyright (C) 2009 Texas Instruments.
> > + *
> > + * 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 <linux/types.h>
> > +#include <linux/io.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/io.h>
> > +#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/clk.h>
> > +#include <linux/gpio.h>
> > +
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/spi_bitbang.h>
> > +
> > +#include <mach/spi.h>
> > +
> > +#include "davinci_spi.h"
> > +
> > +struct device *sdev;
> > +
> > +static void davinci_spi_rx_buf_u8(u32 data, struct davinci_spi
> *davinci_spi)
> > +{
> > +   u8 *rx = davinci_spi->rx;
> > +
> > +   *rx++ = (u8)data;
> > +   davinci_spi->rx = rx;
> > +}
> > +
> > +static void davinci_spi_rx_buf_u16(u32 data, struct davinci_spi
> *davinci_spi)
> > +{
> > +   u16 *rx = davinci_spi->rx;
> > +
> > +   *rx++ = (u16)data;
> > +   davinci_spi->rx = rx;
> > +}
> > +
> > +static u32 davinci_spi_tx_buf_u8(struct davinci_spi *davinci_spi)
> > +{
> > +   u32 data;
> > +   const u8 *tx = davinci_spi->tx;
> > +
> > +   data = *tx++;
> > +   davinci_spi->tx = tx;
> > +   return data;
> > +}
> > +
> > +static u32 davinci_spi_tx_buf_u16(struct davinci_spi *davinci_spi)
> > +{
> > +   u32 data;
> > +   const u16 *tx = davinci_spi->tx;
> > +
> > +   data = *tx++;
> > +   davinci_spi->tx = tx;
> > +   return data;
> > +}
> > +
> > +static inline void set_bits(void __iomem *addr, u32 bits)
> > +{
> > +   u32 v = ioread32(addr);
> > +
> > +   v |= bits;
> > +   iowrite32(v, addr);
> > +}
> > +
> > +static inline void clear_bits(void __iomem *addr, u32 bits)
> > +{
> > +   u32 v = ioread32(addr);
> > +
> > +   v &= ~bits;
> > +   iowrite32(v, addr);
> > +}
> > +
> > +static inline void set_fmt_bits(void __iomem *addr, u32 bits, int
> bus_num)
> > +{
> > +   set_bits(addr + SPIFMT0 + (0x4 * bus_num), bits);
> > +}
> > +
> > +static inline void clear_fmt_bits(void __iomem *addr, u32 bits, int
> bus_num)
> > +{
> > +   clear_bits(addr + SPIFMT0 + (0x4 * bus_num), bits);
> > +}
> > +
> 
> Functions set_fmt_bits and clear_fmt_bits are wrong here.
> Which SPIFMTn register to use is decided by the DFSEL bit
> in SPIDAT1 register and this patch is using SPIFMT0 as the
> default value of DFSEL is ZERO. But the above functions are
> writing to SPIFMT1 if bus_num is 1.
> 
> [...]
> 
> > +
> > +static int davinci_spi_check_error(struct davinci_spi *davinci_spi,
> > +                              int int_status)
> > +{
> > +
> > +   if (int_status & SPIFLG_TIMEOUT_MASK) {
> > +           dev_dbg(sdev, "SPI Time-out Error\n");
> > +           return -ETIMEDOUT;
> > +   }
> > +   if (int_status & SPIFLG_DESYNC_MASK) {
> > +           dev_dbg(sdev, "SPI Desynchronization Error\n");
> > +           return -EIO;
> > +   }
> > +   if (int_status & SPIFLG_BITERR_MASK) {
> > +           dev_dbg(sdev, "SPI Bit error\n");
> > +           return -EIO;
> > +   }
> > +
> > +   if (davinci_spi->version == SPI_VERSION_2) {
> > +           if (int_status & SPIFLG_DLEN_ERR_MASK) {
> > +                   dev_dbg(sdev, "SPI Data Length Error\n");
> > +                   return -EIO;
> > +           }
> > +           if (int_status & SPIFLG_PARERR_MASK) {
> > +                   dev_dbg(sdev, "SPI Parity Error\n");
> > +                   return -EIO;
> > +           }
> > +           if (int_status & SPIFLG_OVRRUN_MASK) {
> > +                   dev_dbg(sdev, "SPI Data Overrun error\n");
> > +                   return -EIO;
> > +           }
> > +           if (int_status & SPIFLG_TX_INTR_MASK) {
> > +                   dev_dbg(sdev, "SPI TX intr bit set\n");
> > +                   return -EIO;
> > +           }
> 
> Do you really need to check for the above flag? This is not
> an error condition. This flag shows that transmit buffer is
> empty and a new data can be written to it.
[Sandeep] The doc says "an interrupt is pending to fill the transmitter"
so it is being checked.
> 
> > +           if (int_status & SPIFLG_BUF_INIT_ACTIVE_MASK) {
> > +                   dev_dbg(sdev, "SPI Buffer Init Active\n");
> > +                   return -EBUSY;
> > +           }
> 
> SPIFLG_BUF_INIT_ACTIVE_MASK is defined as BIT(24) but in SPIFLG
> register bit 24 is reserved.
[Sandeep] I agree your statement is based on the available user guides. But I 
can point you to documents which use BIT(24). I am quite certain that when I 
reviewed early version of the document and CSL code for DM365 that bit 24 was 
not reserved. Don't know if it got missed while publishing.


> 
> Regards, Sudhakar
> 
[Sandeep] You had 1 other comment. I'll get back to you on that.

_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Reply via email to