Re: [PATCH] SPI slave driver for Intel's Moorestown platform

[jassi brar <jassisinghbrar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On Thu, Jan 21, 2010 at 12:28 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> Hi Ken,
>
> Thanks for the patch.  Some comments below.
>
> On Tue, Dec 22, 2009 at 5:36 PM, Ken Mills <ken.k.mills@intel.com> wrote:
>> diff --git a/drivers/spi/mrst_spi_slave.c b/drivers/spi/mrst_spi_slave.c
>> new file mode 100644
>> index 0000000..7180ece
>> --- /dev/null
>> +++ b/drivers/spi/mrst_spi_slave.c
>> @@ -0,0 +1,1136 @@
>> +/*
>> + *  mrst_spi_slave.c - Moorestown SPI slave controller driver
>
> Drop the file name from the comment.  File names often change.  All you
> need here is the statement of what the driver is.
>
>> + *  based on pxa2xx_spi.c
>> + *
>> + *  Copyright (C) Intel 2009
>> + *  Ken Mills <ken.k.mills@intel.com>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *
>> + *  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
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Please drop the ~~~~ horizontal rules.  And unless Intel legal insists
> on the 3 paragraph license text, I think just the first paragraph is
> necessary.  I like to see boilerplate kept to a minimum in files so it
> is easy to glean out the essential information at a glance.
>
>> + *
>> + */
>> +
>> +/*
>> + * Note:
>> + *
>> + * Supports interrupt programmed I/O, DMA and non-interrupt polled transfers.
>> + *
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/highmem.h>
>> +#include <linux/pci.h>
>> +
>> +#ifdef CONFIG_SPI_MRST_SLAVE_DMA
>> +#include <linux/dma-mapping.h>
>> +#include <linux/lnw_dma.h>
>> +#endif
>> +
>> +#include <linux/spi/spi.h>
>> +#include <linux/spi/mrst_spi_slave.h>
>> +
>> +
>> +#define DRIVER_NAME "mrst_spi_slave"
>
> DRIVER_NAME is used in exactly 1 place.  Please drop this line.
>
>> +
>> +#define SSP_NOT_SYNC 0x400000
>> +
>> +MODULE_AUTHOR("");
>
> Either fill in, or drop this line.
>
>> +MODULE_DESCRIPTION("Moorestown SPI Slave Contoller");
>> +MODULE_LICENSE("GPL");
>> +
>> +/*
>> + * For testing SSCR1 changes that require SSP restart, basically
>> + * everything except the service and interrupt enables
>> + */
>> +#define SSCR1_CHANGE_MASK (SSCR1_TTELP | SSCR1_TTE | SSCR1_EBCEI | SSCR1_SCFR \
>> +                               | SSCR1_ECRA | SSCR1_ECRB | SSCR1_SCLKDIR \
>> +                               | SSCR1_SFRMDIR \
>> +                               | SSCR1_RWOT | SSCR1_TRAIL | SSCR1_PINTE \
>> +                               | SSCR1_STRF | SSCR1_EFWR | SSCR1_RFT \
>> +                               | SSCR1_TFT | SSCR1_SPH | SSCR1_SPO)
>> +
>> +#define DEFINE_SSP_REG(reg, off) \
>> +static inline u32 read_##reg(void *p) { return __raw_readl(p + (off)); } \
>> +static inline void write_##reg(u32 v, void *p) { __raw_writel(v, p + (off)); }
>
> Even though they are restricted to the file scope, static globals still
> compete with the global namespace.  It is good practise to prefix all
> global statics with a driver prefix.  This goes for all declarations;
> variables, functions and structures.
>
> Adding a prefix also makes it easier to differentiate between driver local
> stuff and common functions when reviewing code.
>
>> +#define START_STATE ((void *)0)
>> +#define RUNNING_STATE ((void *)1)
>> +#define DONE_STATE ((void *)2)
>> +#define ERROR_STATE ((void *)-1)
>
> This looks pretty ugly, and while the state pointer gets set to these
> values, it doesn't seem to be actually used anywhere.
>
>> +
>> +struct chip_data {
>> +       u32 cr0;
>> +       u32 cr1;
>> +       u32 psp;
>> +       u32 timeout;
>> +       u8 n_bytes;
>> +       u32 threshold;
>> +       u8 enable_dma;
>> +       u8 poll_mode;     /* 1 means use poll mode */
>> +       u8 bits_per_word;
>> +       int (*write)(struct driver_data *drv_data);
>> +       int (*read)(struct driver_data *drv_data);
>> +};
>> +
>> +static void flush(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       u32 sssr;
>> +
>> +       /* If the transmit fifo is not empty, reset the interface. */
>> +       sssr = read_SSSR(reg);
>> +       if ((sssr & 0xf00) || (sssr & SSSR_TNF) == 0) {
>> +               write_SSCR0(read_SSCR0(reg) & ~SSCR0_SSE, reg);
>> +               return;
>> +       }
>> +
>> +       while (read_SSSR(reg) & SSSR_RNE)
>> +               read_SSDR(reg);
>> +
>> +       write_SSSR(SSSR_ROR, reg);
>> +       write_SSSR(SSSR_TUR, reg);
>> +
>> +       return;
>> +}
>> +
>> +static int null_writer(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       u8 n_bytes = drv_data->n_bytes;
>> +
>> +       if (((read_SSSR(reg) & 0x00000f00) == 0x00000f00)
>> +               || (drv_data->tx == drv_data->tx_end))
>> +               return 0;
>> +
>> +       write_SSDR(0, reg);
>> +       drv_data->tx += n_bytes;
>> +
>> +       return 1;
>> +}
>> +
>> +static int null_reader(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       u8 n_bytes = drv_data->n_bytes;
>> +
>> +       while ((read_SSSR(reg) & SSSR_RNE)
>> +               && (drv_data->rx < drv_data->rx_end)) {
>> +               read_SSDR(reg);
>> +               drv_data->rx += n_bytes;
>> +       }
>> +
>> +       return drv_data->rx == drv_data->rx_end;
>> +}
>> +
>> +static int u8_writer(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       if (((read_SSSR(reg) & 0x00000f00) == 0x00000f00)
>> +               || (drv_data->tx == drv_data->tx_end))
>> +               return 0;
>> +
>> +       write_SSDR(*(u8 *)(drv_data->tx), reg);
>> +       ++drv_data->tx;
>> +
>> +       return 1;
>> +}
>> +
>> +static int u8_reader(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       while ((read_SSSR(reg) & SSSR_RNE)
>> +               && (drv_data->rx < drv_data->rx_end)) {
>> +               *(u8 *)(drv_data->rx) = read_SSDR(reg);
>> +               ++drv_data->rx;
>> +       }
>> +
>> +       return drv_data->rx == drv_data->rx_end;
>> +}
>> +
>> +static int u16_writer(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       if (((read_SSSR(reg) & 0x00000f00) == 0x00000f00)
>> +               || (drv_data->tx == drv_data->tx_end))
>> +               return 0;
>> +
>> +       write_SSDR(*(u16 *)(drv_data->tx), reg);
>> +       drv_data->tx += 2;
>> +
>> +       return 1;
>> +}
>> +
>> +static int u16_reader(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       while ((read_SSSR(reg) & SSSR_RNE)
>> +               && (drv_data->rx < drv_data->rx_end)) {
>> +               *(u16 *)(drv_data->rx) = read_SSDR(reg);
>> +               drv_data->rx += 2;
>> +       }
>> +
>> +return drv_data->rx == drv_data->rx_end;
>> +}
>> +
>> +static int u32_writer(struct driver_data *drv_data)
>> +{
>> +void *reg = drv_data->ioaddr;
>> +       if (((read_SSSR(reg) & 0x00000f00) == 0x00000f00)
>> +               || (drv_data->tx == drv_data->tx_end))
>> +               return 0;
>> +
>> +       write_SSDR(*(u32 *)(drv_data->tx), reg);
>> +       drv_data->tx += 4;
>> +
>> +       return 1;
>> +}
>> +
>> +static int u32_reader(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       while ((read_SSSR(reg) & SSSR_RNE)
>> +               && (drv_data->rx < drv_data->rx_end)) {
>> +               *(u32 *)(drv_data->rx) = read_SSDR(reg);
>> +               drv_data->rx += 4;
>> +       }
>> +
>> +       return drv_data->rx == drv_data->rx_end;
>> +}
>> +
>> +
>> +
>> +/* caller already set message->status; dma and pio irqs are blocked */
>> +static void giveback(struct driver_data *drv_data)
>> +{
>> +       struct spi_message *msg;
>> +
>> +       msg = drv_data->cur_msg;
>> +       msg->state = NULL;
>> +       if (msg->complete)
>> +               msg->complete(msg->context);
>> +}
>> +
>> +#ifdef CONFIG_SPI_MRST_SLAVE_DMA
>> +
>> +static bool chan_filter(struct dma_chan *chan, void *param)
>> +{
>> +       struct driver_data *drv_data = (struct driver_data *)param;
>> +       bool ret = false;
>> +
>> +       if (!drv_data->dmac1)
>> +               return ret;
>> +
>> +       if (chan->device->dev == &drv_data->dmac1->dev)
>> +               ret = true;
>> +
>> +       return ret;
>> +}
>> +
>> +static void int_transfer_complete(struct driver_data *drv_data);
>> +static void unmap_dma_buffers(struct driver_data *drv_data);
>> +
>> +static void mrst_spi_dma_done(void *arg)
>> +{
>> +       u64 *param = arg;
>> +       struct driver_data *drv_data;
>> +       int *done;
>> +
>> +       drv_data = (struct driver_data *)(u32)(*param >> 32);
>> +       done = (int *)(u32)(*param & 0xffffffff);
>> +       *done = 1;
>> +
>> +       unmap_dma_buffers(drv_data);
>> +
>> +       if (!drv_data->txdma_done || !drv_data->rxdma_done)
>> +               return;
>> +       int_transfer_complete(drv_data);
>> +}
>> +
>> +static void mrst_spi_dma_init(struct driver_data *drv_data)
>> +{
>> +       struct lnw_dma_slave *rxs, *txs;
>> +       dma_cap_mask_t mask;
>> +
>> +       /* Use DMAC1 */
>> +       drv_data->dmac1 = pci_get_device(PCI_VENDOR_ID_INTEL, 0x0814, NULL);
>> +       if (!drv_data->dmac1) {
>> +               printk(KERN_WARNING "SPI Slave:Can't find DMAC1\n");
>> +               return;
>> +       }
>> +
>> +/* 1. init rx channel */
>> +       rxs = &drv_data->dmas_rx;
>> +
>> +       rxs->dirn = DMA_FROM_DEVICE;
>> +       rxs->hs_mode = LNW_DMA_HW_HS;
>> +       rxs->cfg_mode = LNW_DMA_PER_TO_MEM;
>> +       rxs->src_width = LNW_DMA_WIDTH_16BIT;
>> +       rxs->dst_width = LNW_DMA_WIDTH_32BIT;
>> +       rxs->src_msize = LNW_DMA_MSIZE_8;
>> +       rxs->dst_msize = LNW_DMA_MSIZE_8;
>> +
>> +
>> +       dma_cap_zero(mask);
>> +       dma_cap_set(DMA_MEMCPY, mask);
>> +       dma_cap_set(DMA_SLAVE, mask);
>> +
>> +       drv_data->rxchan = dma_request_channel(mask, chan_filter, drv_data);
>> +       if (!drv_data->rxchan)
>> +               goto err_exit;
>> +
>> +       drv_data->rxchan->private = rxs;
>> +
>> +       /* 2. init tx channel */
>> +       txs = &drv_data->dmas_tx;
>> +
>> +       txs->dirn = DMA_TO_DEVICE;
>> +       txs->hs_mode = LNW_DMA_HW_HS;
>> +       txs->cfg_mode = LNW_DMA_MEM_TO_PER;
>> +       txs->src_width = LNW_DMA_WIDTH_32BIT;
>> +       txs->dst_width = LNW_DMA_WIDTH_16BIT;
>> +       txs->src_msize = LNW_DMA_MSIZE_8;
>> +       txs->dst_msize = LNW_DMA_MSIZE_8;
>> +
>> +
>> +       dma_cap_set(DMA_SLAVE, mask);
>> +       dma_cap_set(DMA_MEMCPY, mask);
>> +
>> +       drv_data->txchan = dma_request_channel(mask, chan_filter, drv_data);
>> +       if (!drv_data->txchan)
>> +               goto free_rxchan;
>> +       else
>> +
>> +       drv_data->txchan->private = txs;
>> +
>> +       /* set the dma done bit to 1 */
>> +       drv_data->txdma_done = 1;
>> +       drv_data->rxdma_done = 1;
>> +
>> +       drv_data->tx_param = ((u64)(u32)drv_data << 32)
>> +               | (u32)(&drv_data->txdma_done);
>> +       drv_data->rx_param = ((u64)(u32)drv_data << 32)
>> +               | (u32)(&drv_data->rxdma_done);
>> +       return;
>> +
>> +free_rxchan:
>> +       printk(KERN_ERR "SPI-Slave Error : DMA Channle Not available\n");
>> +       dma_release_channel(drv_data->rxchan);
>> +err_exit:
>> +       printk(KERN_ERR "SPI-Slave Error : DMA Channel Not available\n");
>> +       pci_dev_put(drv_data->dmac1);
>> +       return;
>> +}
>> +
>> +static void mrst_spi_dma_exit(struct driver_data *drv_data)
>> +{
>> +       dma_release_channel(drv_data->txchan);
>> +       dma_release_channel(drv_data->rxchan);
>> +       pci_dev_put(drv_data->dmac1);
>> +}
>> +
>> +static void dma_transfer(struct driver_data *drv_data)
>> +{
>> +       dma_addr_t ssdr_addr;
>> +       struct dma_async_tx_descriptor *txdesc = NULL, *rxdesc = NULL;
>> +       struct dma_chan *txchan, *rxchan;
>> +       enum dma_ctrl_flags flag;
>> +
>> +       /* get Data Read/Write address */
>> +       ssdr_addr = (dma_addr_t)(u32)(drv_data->paddr + 0x10);
>> +
>> +       if (drv_data->tx_dma)
>> +               drv_data->txdma_done = 0;
>> +
>> +       if (drv_data->rx_dma)
>> +               drv_data->rxdma_done = 0;
>> +
>> +       /* 2. start the TX dma transfer */
>> +       txchan = drv_data->txchan;
>> +       rxchan = drv_data->rxchan;
>> +
>> +       flag = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>> +
>> +       if (drv_data->rx_dma) {
>> +               rxdesc = rxchan->device->device_prep_dma_memcpy
>> +               (rxchan,                /* DMA Channel */
>> +               drv_data->rx_dma,       /* DAR */
>> +               ssdr_addr,              /* SAR */
>> +               drv_data->len,          /* Data Length */
>> +               flag);                  /* Flag */
>> +
>> +               rxdesc->callback = mrst_spi_dma_done;
>> +               rxdesc->callback_param = &drv_data->rx_param;
>> +       }
>> +
>> +/* 3. start the RX dma transfer */
>> +       if (drv_data->tx_dma) {
>> +               txdesc = txchan->device->device_prep_dma_memcpy
>> +               (txchan,                /* DMA Channel */
>> +               ssdr_addr,              /* DAR */
>> +               drv_data->tx_dma,       /* SAR */
>> +               drv_data->len,          /* Data Length */
>> +               flag);                  /* Flag */
>> +
>> +               txdesc->callback = mrst_spi_dma_done;
>> +               txdesc->callback_param = &drv_data->tx_param;
>> +       }
>> +
>> +       if (rxdesc)
>> +               rxdesc->tx_submit(rxdesc);
>> +       if (txdesc)
>> +               txdesc->tx_submit(txdesc);
>> +}
>> +
>> +static int map_dma_buffers(struct driver_data *drv_data)
>> +{
>> +       struct device *dev = &drv_data->pdev->dev;
>> +
>> +       if (drv_data->dma_mapped)
>> +               return 1;
>> +
>> +       drv_data->tx_dma =
>> +               dma_map_single(dev, drv_data->tx, drv_data->len,
>> +                       PCI_DMA_TODEVICE);
>> +       if (dma_mapping_error(dev, drv_data->tx_dma))
>> +               return 0;
>> +
>> +       drv_data->rx_dma =
>> +               dma_map_single(dev, drv_data->rx, drv_data->len,
>> +                       PCI_DMA_FROMDEVICE);
>> +       if (dma_mapping_error(dev, drv_data->rx_dma)) {
>> +               dma_unmap_single(dev, drv_data->tx_dma,
>> +                       drv_data->tx_map_len, DMA_TO_DEVICE);
>> +               return 0;
>> +       }
>> +
>> +       return 1;
>> +}
>> +
>> +static void unmap_dma_buffers(struct driver_data *drv_data)
>> +{
>> +       struct device *dev = &drv_data->pdev->dev;
>> +
>> +       if (!drv_data->dma_mapped)
>> +               return;
>> +       dma_unmap_single(dev, drv_data->rx_dma, drv_data->len,
>> +               PCI_DMA_FROMDEVICE);
>> +       dma_unmap_single(dev, drv_data->tx_dma, drv_data->len,
>> +               PCI_DMA_TODEVICE);
>> +       drv_data->dma_mapped = 0;
>> +}
>> +
>> +#endif
>> +
>> +static void int_error_stop(struct driver_data *drv_data, const char* msg)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +
>> +       /* Stop and reset SSP */
>> +       write_SSSR(drv_data->clear_sr, reg);
>> +       write_SSCR1(read_SSCR1(reg) & ~drv_data->int_cr1, reg);
>> +       write_SSTO(0, reg);
>> +       flush(drv_data);
>> +
>> +       dev_err(&drv_data->pdev->dev, "%s\n", msg);
>> +
>> +       drv_data->cur_msg->state = ERROR_STATE;
>> +}
>> +
>> +static void int_transfer_complete(struct driver_data *drv_data)
>> +{
>> +void *reg = drv_data->ioaddr;
>> +
>> +       /* Clear Status Register */
>> +       write_SSSR(drv_data->clear_sr, reg);
>> +
>> +#ifdef CONFIG_SPI_MRST_SLAVE_DMA
>> +       /* Disable Triggers to DMA */
>> +       write_SSCR1(read_SSCR1(reg) & ~drv_data->dma_cr1, reg);
>> +#else
>> +       /* Disable Interrupt */
>> +       write_SSCR1(read_SSCR1(reg) & ~drv_data->int_cr1, reg);
>> +#endif
>> +       /* Stop getting Time Outs */
>> +       write_SSTO(0, reg);
>> +
>> +       /* Update total byte transfered return count actual bytes read */
>> +       drv_data->cur_msg->actual_length += drv_data->len -
>> +       (drv_data->rx_end - drv_data->rx);
>> +
>> +       drv_data->cur_msg->status = 0;
>> +       giveback(drv_data);
>> +}
>> +
>> +static void transfer_complete(struct driver_data *drv_data)
>> +{
>> +       /* Update total byte transfered return count actual bytes read */
>> +       drv_data->cur_msg->actual_length +=
>> +               drv_data->len - (drv_data->rx_end - drv_data->rx);
>> +
>> +       drv_data->cur_msg->status = 0;
>> +       giveback(drv_data);
>> +}
>> +
>> +static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
>> +{
>> +       void *reg = drv_data->ioaddr;
>> +       u32 irq_mask = (read_SSCR1(reg) & SSCR1_TIE) ?
>> +       drv_data->mask_sr : drv_data->mask_sr & ~SSSR_TFS;
>> +
>> +       u32 irq_status = read_SSSR(reg) & irq_mask;
>> +       if (irq_status & SSSR_ROR) {
>> +               int_error_stop(drv_data, "interrupt_transfer: fifo overrun");
>> +               return IRQ_HANDLED;
>> +       }
>> +
>> +       if (irq_status & SSSR_TINT) {
>> +               write_SSSR(SSSR_TINT, reg);
>> +               if (drv_data->read(drv_data)) {
>> +                       int_transfer_complete(drv_data);
>> +                       return IRQ_HANDLED;
>> +               }
>> +       }
>> +
>> +/* Drain rx fifo, Fill tx fifo and prevent overruns */
>
> Indentation
>
>> +       do {
>> +               if (drv_data->read(drv_data)) {
>> +                       int_transfer_complete(drv_data);
>> +                       return IRQ_HANDLED;
>> +               }
>> +       } while (drv_data->write(drv_data));
>> +
>> +       if (drv_data->read(drv_data)) {
>> +               int_transfer_complete(drv_data);
>> +               return IRQ_HANDLED;
>> +       }
>> +
>> +       if (drv_data->tx == drv_data->tx_end)
>> +               write_SSCR1(read_SSCR1(reg) & ~SSCR1_TIE, reg);
>> +
>> +       return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t ssp_int(int irq, void *dev_id)
>> +{
>> +       struct driver_data *drv_data = dev_id;
>> +       void *reg = drv_data->ioaddr;
>> +#ifdef CONFIG_SPI_MRST_SLAVE_DMA
>> +       u32 status = read_SSSR(reg);
>> +
>> +       if (status & SSSR_ROR || status & SSSR_TUR) {
>> +               printk(KERN_DEBUG "--- SPI ROR or TUR occurred : SSSR=%x\n",
>> +       status);
>> +               write_SSSR(SSSR_ROR, reg);              /* Clear ROR */
>> +               write_SSSR(SSSR_TUR, reg);              /* Clear TUR */
>> +               return IRQ_HANDLED;
>> +       }
>> +       return IRQ_NONE;
>> +#endif
>
> I don't see CONFIG_SPI_MRST_SLAVE_DMA defined anywhere.  Please drop these
> sections from the driver until that bit is ready.  It can be added in a
> follow-on patch.
>
>> +       /* just return if this is not our interrupt */
>> +       if (!(read_SSSR(reg) & drv_data->mask_sr))
>> +               return IRQ_NONE;
>> +
>> +       if (!drv_data->cur_msg) {
>> +               write_SSCR0(read_SSCR0(reg) & ~SSCR0_SSE, reg);
>> +               write_SSCR1(read_SSCR1(reg) & ~drv_data->int_cr1, reg);
>> +               write_SSSR(drv_data->clear_sr, reg);
>> +
>> +               /* Never fail */
>> +               return IRQ_HANDLED;
>> +       }
>> +       return drv_data->transfer_handler(drv_data);
>> +}
>> +
>> +static void poll_transfer(unsigned long data)
>> +{
>> +       struct driver_data *drv_data = (struct driver_data *)data;
>> +
>> +       if (drv_data->tx)
>> +               while (drv_data->tx != drv_data->tx_end) {
>> +                       drv_data->write(drv_data);
>> +                       drv_data->read(drv_data);
>> +               }
>> +
>> +       while (!drv_data->read(drv_data))
>> +               ;
>> +
>> +       transfer_complete(drv_data);
>> +}
>> +
>> +static int transfer(struct spi_device *spi, struct spi_message *msg)
>> +{
>> +       struct driver_data *drv_data = \
>> +       spi_slave_get_devdata(spi->slave);
>
> Backslashes are unnecessary in normal C code.  :-)
>
>> +       unsigned long flags;
>> +       struct chip_data *chip = NULL;
>> +       struct spi_transfer *transfer = NULL;
>> +       void *reg = drv_data->ioaddr;
>> +       void *i2cReg = drv_data->I2C_ioaddr;
>> +       u32 clk_div = 0;
>> +       u8 bits = 0;
>> +       u32 cr0;
>> +       u32 cr1;
>> +       u32 sssr;
>> +
>> +       spin_lock_irqsave(&drv_data->lock, flags);
>> +       msg->actual_length = 0;
>> +       msg->status = -EINPROGRESS;
>> +       drv_data->cur_msg = msg;
>> +       /* Initial message state*/
>> +       msg->state = START_STATE;
>> +
>> +       /* We handle only one transfer message since the protocol module has to
>> +          control the out of band signaling. */
>
> Try to keep lines down under 80 chars as much as possible.
>
>> +       transfer = list_entry(msg->transfers.next,
>> +       struct spi_transfer,
>> +       transfer_list);
>> +
>> +       chip = spi_get_ctldata(msg->spi);
>> +
>> +       drv_data->busy = 1;
>> +
>> +/* Check transfer length */
>
> indent
>
>> +       if (transfer->len > 8192) {
>> +               dev_warn(&drv_data->pdev->dev, "SPI-SLAVE: transfer "
>> +               "length greater than 8192\n");
>
> Indent second line so it is obvious that this is a continuation of the
> dev_warn parameters.
>
>> +               msg->status = -EINVAL;
>> +               giveback(drv_data);
>> +               spin_unlock_irqrestore(&drv_data->lock, flags);
>> +               return 0;
>> +       }
>> +
>> +       /* Setup the transfer state based on the type of transfer */
>> +       flush(drv_data);
>> +       drv_data->n_bytes = chip->n_bytes;
>> +       drv_data->tx = (void *)transfer->tx_buf;
>> +       drv_data->tx_end = drv_data->tx + transfer->len;
>> +       drv_data->rx = transfer->rx_buf;
>> +       drv_data->rx_end = drv_data->rx + transfer->len;
>> +       drv_data->rx_dma = transfer->rx_dma;
>> +       drv_data->tx_dma = transfer->tx_dma;
>> +       drv_data->len = transfer->len;
>> +       drv_data->write = drv_data->tx ? chip->write : null_writer;
>> +       drv_data->read = drv_data->rx ? chip->read : null_reader;
>> +
>> +       /* Change speed and bit per word on a per transfer */
>> +       cr0 = chip->cr0;
>> +       if (transfer->bits_per_word) {
>> +               bits = chip->bits_per_word;
>> +               clk_div = 0x0;
>> +
>> +       if (transfer->bits_per_word)
>> +               bits = transfer->bits_per_word;
>
> ick.  Must fix this indentation over whole file so a reader can follow
> the scope.
>
>> diff --git a/include/linux/spi/mrst_spi_slave.h b/include/linux/spi/mrst_spi_slave.h
>> new file mode 100644
>> index 0000000..70ea3b7
>> --- /dev/null
>> +++ b/include/linux/spi/mrst_spi_slave.h
>> @@ -0,0 +1,134 @@
>> +/*
>
> Include a one-line statement on what this file is.
This header contains mostly what is controller specific.
Am not sure if include/linux/spi is the place for this header, but only
for common SPI protocol headers and other stuff used across various
platforms(even though, unfortunately, that isn't the situation already).

>> + *  Copyright (C) Intel 2009
>> + *  Ken Mills <ken.k.mills@intel.com>
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> + *
>> + *  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
>> + *
>> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Same comment as on .c file.
>> + *
>> + */
>> +#ifndef MRST_SSP_H_
>> +#define MRST_SSP_H_
>> +
>> +
>> +/*
>> + * Langwell SSP serial port register definitions
>> + */
>> +
>> +#define SSCR0_DSS      (0x0000000f)    /* Data Size Select (mask) */
>> +#define SSCR0_DataSize(x) ((x) - 1)    /* Data Size Select [4..16] */
>> +#define SSCR0_FRF      (0x00000030)    /* FRame Format (mask) */
>> +#define SSCR0_Motorola (0x0 << 4)      /* Motorola's SPI mode */
>> +#define SSCR0_ECS      (1 << 6)        /* External clock select */
>> +#define SSCR0_SSE      (1 << 7)        /* Synchronous Serial Port Enable */
>> +
>> +
>> +#define SSCR0_SCR      (0x000fff00)    /* Serial Clock Rate (mask) */
>> +#define SSCR0_SerClkDiv(x) (((x) - 1) << 8)/* Divisor [1..4096] */
>> +#define SSCR0_EDSS     (1 << 20)       /* Extended data size select */
>> +#define SSCR0_NCS      (1 << 21)       /* Network clock select */
>> +#define SSCR0_RIM      (1 << 22)       /* Receive FIFO overrrun int mask */
>> +#define SSCR0_TUM      (1 << 23)       /* Transmit FIFO underrun int mask */
>> +#define SSCR0_FRDC     (0x07000000)    /* Frame rate divider control (mask) */
>> +#define SSCR0_SlotsPerFrm(x) (((x) - 1) << 24)/* Time slots per frame */
>> +#define SSCR0_ADC      (1 << 30)       /* Audio clock select */
>> +#define SSCR0_MOD      (1 << 31)       /* Mode (normal or network) */
>> +
>> +
>> +#define SSCR1_RIE      (1 << 0)        /* Receive FIFO Interrupt Enable */
>> +#define SSCR1_TIE      (1 << 1)        /* Transmit FIFO Interrupt Enable */
>> +#define SSCR1_LBM      (1 << 2)        /* Loop-Back Mode */
>> +#define SSCR1_SPO      (1 << 3)        /* SSPSCLK polarity setting */
>> +#define SSCR1_SPH      (1 << 4)        /* Motorola SPI SSPSCLK phase setting */
>> +#define SSCR1_MWDS     (1 << 5)        /* Microwire Transmit Data Size */
>> +#define SSCR1_TFT      (0x000003c0)    /* Transmit FIFO Threshold (mask) */
>> +#define SSCR1_TxTresh(x) (((x) - 1) << 6) /* level [1..16] */
>> +#define SSCR1_RFT      (0x00003c00)    /* Receive FIFO Threshold (mask) */
>> +#define SSCR1_RxTresh(x) (((x) - 1) << 10) /* level [1..16] */
>> +
>> +#define SSSR_TNF       (1 << 2)        /* Transmit FIFO Not Full */
>> +#define SSSR_RNE       (1 << 3)        /* Receive FIFO Not Empty */
>> +#define SSSR_BSY       (1 << 4)        /* SSP Busy */
>> +#define SSSR_TFS       (1 << 5)        /* Transmit FIFO Service Request */
>> +#define SSSR_RFS       (1 << 6)        /* Receive FIFO Service Request */
>> +#define SSSR_ROR       (1 << 7)        /* Receive FIFO Overrun */
>> +
>> +#define SSCR0_TIM      (1 << 23)       /* Transmit FIFO Under Run Int Mask */
>> +#define SSCR0_RIM      (1 << 22)       /* Receive FIFO Over Run int Mask */
>> +#define SSCR0_NCS      (1 << 21)       /* Network Clock Select */
>> +#define SSCR0_EDSS     (1 << 20)       /* Extended Data Size Select */
>> +
>> +#define SSCR0_TISSP    (1 << 4)        /* TI Sync Serial Protocol */
>> +#define SSCR0_PSP      (3 << 4)        /* PSP - Programmable Serial Protocol */
>> +#define SSCR1_TTELP    (1 << 31)       /* TXD Tristate Enable Last Phase */
>> +#define SSCR1_TTE      (1 << 30)       /* TXD Tristate Enable */
>> +#define SSCR1_EBCEI    (1 << 29)       /* Enable Bit Count Error interrupt */
>> +#define SSCR1_SCFR     (1 << 28)       /* Slave Clock free Running */
>> +#define SSCR1_ECRA     (1 << 27)       /* Enable Clock Request A */
>> +#define SSCR1_ECRB     (1 << 26)       /* Enable Clock request B */
>> +#define SSCR1_SCLKDIR  (1 << 25)       /* Serial Bit Rate Clock Direction */
>> +#define SSCR1_SFRMDIR  (1 << 24)       /* Frame Direction */
>> +#define SSCR1_RWOT     (1 << 23)       /* Receive Without Transmit */
>> +#define SSCR1_TRAIL    (1 << 22)       /* Trailing Byte */
>> +#define SSCR1_TSRE     (1 << 21)       /* Transmit Service Request Enable */
>> +#define SSCR1_RSRE     (1 << 20)       /* Receive Service Request Enable */
>> +#define SSCR1_TINTE    (1 << 19)       /* Receiver Time-out Interrupt enable */
>> +#define SSCR1_PINTE    (1 << 18)       /* Trailing Byte Interupt Enable */
>> +#define SSCR1_STRF     (1 << 15)       /* Select FIFO or EFWR */
>> +#define SSCR1_EFWR     (1 << 14)       /* Enable FIFO Write/Read */
>> +
>> +#define SSSR_BCE       (1 << 23)       /* Bit Count Error */
>> +#define SSSR_CSS       (1 << 22)       /* Clock Synchronisation Status */
>> +#define SSSR_TUR       (1 << 21)       /* Transmit FIFO Under Run */
>> +#define SSSR_EOC       (1 << 20)       /* End Of Chain */
>> +#define SSSR_TINT      (1 << 19)       /* Receiver Time-out Interrupt */
>> +#define SSSR_PINT      (1 << 18)       /* Peripheral Trailing Byte Interrupt */
>> +
>> +#define SSPSP_FSRT     (1 << 25)       /* Frame Sync Relative Timing */
>> +#define SSPSP_DMYSTOP(x) ((x) << 23)   /* Dummy Stop */
>> +#define SSPSP_SFRMWDTH(x) ((x) << 16)  /* Serial Frame Width */
>> +#define SSPSP_SFRMDLY(x) ((x) << 9)    /* Serial Frame Delay */
>> +#define SSPSP_DMYSTRT(x) ((x) << 7)    /* Dummy Start */
>> +#define SSPSP_STRTDLY(x) ((x) << 4)    /* Start Delay */
>> +#define SSPSP_ETDS(1 << 3)             /* End of Transfer data State */
>> +#define SSPSP_SFRMP  (1 << 2)          /* Serial Frame Polarity */
>> +#define SSPSP_SCMODE(x)((x) << 0)      /* Serial Bit Rate Clock Mode */
>
> Move these #defines to the .c file.  They aren't part of the interface.
>
>> +
>> +/* spi_board_info.controller_data for SPI slave devices,
>> + * copied to spi_device.platform_data ... mostly for dma tuning
>> + */
>> +struct mrst_spi_chip {
>> +       u8 tx_threshold;
>> +       u8 rx_threshold;
>> +       u8 dma_burst_size;
>> +       u32 timeout;
>> +       u8 enable_loopback;
>> +       u16 extra_data[5];
>> +};
>> +
>> +#define SPI_DIB_NAME_LEN  16
>> +#define SPI_DIB_SPEC_INFO_LEN  10
>
> Not used anywhere
>
>> +struct spi_dib_header {
>> +       u32signature;
>> +       u32length;
>> +       u8 rev;
>> +       u8 checksum;
>> +       u8 dib[0];
>> +} __attribute__((packed));
>
> spi_dib_header isn't used anywhere.
>
> Cheers,
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>
> ------------------------------------------------------------------------------
> Throughout its 18-year history, RSA Conference consistently attracts the
> world's best and brightest in the field, creating opportunities for Conference
> attendees to learn about information security's most important issues through
> interactions with peers, luminaries and emerging and established companies.
> http://p.sf.net/sfu/rsaconf-dev2dev
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general
>

------------------------------------------------------------------------------
Throughout its 18-year history, RSA Conference consistently attracts the
world's best and brightest in the field, creating opportunities for Conference
attendees to learn about information security's most important issues through
interactions with peers, luminaries and emerging and established companies.
http://p.sf.net/sfu/rsaconf-dev2dev
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general