Re: [PATCH v2] spi: New driver for Altera SPI

On Sun, Jan 16, 2011 at 10:46 PM, Thomas Chou [off-list ref] wrote:

This patch adds a new SPI driver to support the Altera SOPC Builder
SPI component. It uses the bitbanging library.

Signed-off-by: Thomas Chou <redacted>

Hi Thomas, some minor comments below.  I've not dug any deeper, so I
have not verified locking or the way the driver uses the bitbang
library, or anything like that.

g.

Is this a flag?  If so, use type 'bool'.

+       int len;
+       int count;
+       int bytes_per_word;
+       unsigned long imr;
+
+       /* data buffers */
+       const unsigned char *tx;
+       unsigned char *rx;
+
+};

Nit: drop the blank line at the end of the structure.

+
+static inline struct altera_spi *to_hw(struct spi_device *sdev)

All file scope symbols should have a driver prefix.  In this case, it
should be "altera_spi_to_hw()".

+{
+       return spi_master_get_devdata(sdev->master);
+}
+
+static void altera_spi_chipsel(struct spi_device *spi, int value)
+{
+       struct altera_spi *hw = to_hw(spi);
+
+       if (spi->mode & SPI_CS_HIGH) {
+               switch (value) {
+               case BITBANG_CS_INACTIVE:
+                       writel(1 << spi->chip_select,
+                              hw->base + ALTERA_SPI_SLAVE_SEL);
+                       hw->imr |= ALTERA_SPI_CONTROL_SSO_MSK;
+                       writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+                       break;
+
+               case BITBANG_CS_ACTIVE:
+                       hw->imr &= ~ALTERA_SPI_CONTROL_SSO_MSK;
+                       writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+                       writel(0, hw->base + ALTERA_SPI_SLAVE_SEL);
+                       break;
+               }
+       } else {
+               switch (value) {
+               case BITBANG_CS_INACTIVE:
+                       hw->imr &= ~ALTERA_SPI_CONTROL_SSO_MSK;
+                       writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+                       break;
+
+               case BITBANG_CS_ACTIVE:
+                       writel(1 << spi->chip_select,
+                              hw->base + ALTERA_SPI_SLAVE_SEL);
+                       hw->imr |= ALTERA_SPI_CONTROL_SSO_MSK;
+                       writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+                       break;
+               }
+       }
+}
+
+static int altera_spi_setupxfer(struct spi_device *spi, struct spi_transfer *t)
+{
+       return 0;
+}
+
+static int altera_spi_setup(struct spi_device *spi)
+{
+       return 0;
+}
+
+static inline unsigned int hw_txbyte(struct altera_spi *hw, int count)
+{
+       if (hw->tx) {
+               switch (hw->bytes_per_word) {
+               case 1:
+                       return hw->tx[count];
+               case 2:
+                       return (hw->tx[count * 2]
+                               | (hw->tx[count * 2 + 1] << 8));
+               }
+       }
+
+       return 0;
+}
+
+static int altera_spi_txrx(struct spi_device *spi, struct spi_transfer *t)
+{
+       struct altera_spi *hw = to_hw(spi);
+
+       hw->tx = t->tx_buf;
+       hw->rx = t->rx_buf;
+       hw->count = 0;
+       hw->bytes_per_word = (t->bits_per_word ? : spi->bits_per_word) / 8;
+       hw->len = t->len / hw->bytes_per_word;
+
+       if (hw->irq >= 0 && hw->interrupt) {
+               init_completion(&hw->done);
+               /* enable receive interrupt */
+               hw->imr |= ALTERA_SPI_CONTROL_IRRDY_MSK;
+               writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+
+               /* send the first byte */
+               writel(hw_txbyte(hw, 0), hw->base + ALTERA_SPI_TXDATA);
+
+               wait_for_completion(&hw->done);
+               /* disable receive interrupt */
+               hw->imr &= ~ALTERA_SPI_CONTROL_IRRDY_MSK;
+               writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+       } else {
+               /* send the first byte */
+               writel(hw_txbyte(hw, 0), hw->base + ALTERA_SPI_TXDATA);
+
+               while (1) {
+                       unsigned int rxd;
+
+                       while (!(readl(hw->base + ALTERA_SPI_STATUS) &
+                                ALTERA_SPI_STATUS_RRDY_MSK))
+                               cpu_relax();
+
+                       rxd = readl(hw->base + ALTERA_SPI_RXDATA);
+                       if (hw->rx) {
+                               switch (hw->bytes_per_word) {
+                               case 1:
+                                       hw->rx[hw->count] = rxd;
+                                       break;
+                               case 2:
+                                       hw->rx[hw->count * 2] = rxd;
+                                       hw->rx[hw->count * 2 + 1] = rxd >> 8;
+                                       break;
+                               }
+                       }
+
+                       hw->count++;
+
+                       if (hw->count < hw->len)
+                               writel(hw_txbyte(hw, hw->count),
+                                      hw->base + ALTERA_SPI_TXDATA);
+                       else
+                               break;
+               }
+
+       }
+
+       return hw->count * hw->bytes_per_word;
+}
+
+static irqreturn_t altera_spi_irq(int irq, void *dev)
+{
+       struct altera_spi *hw = dev;
+       unsigned int rxd;
+
+       rxd = readl(hw->base + ALTERA_SPI_RXDATA);
+       if (hw->rx) {
+               switch (hw->bytes_per_word) {
+               case 1:
+                       hw->rx[hw->count] = rxd;
+                       break;
+               case 2:
+                       hw->rx[hw->count * 2] = rxd;
+                       hw->rx[hw->count * 2 + 1] = rxd >> 8;
+                       break;
+               }
+       }
+
+       hw->count++;
+
+       if (hw->count < hw->len)
+               writel(hw_txbyte(hw, hw->count), hw->base + ALTERA_SPI_TXDATA);
+       else
+               complete(&hw->done);
+
+       return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_OF
+static int __devinit altera_spi_of_probe(struct platform_device *pdev,
+                                      struct altera_spi *hw)
+{
+       const __be32 *val;
+
+       hw->bitbang.master->dev.of_node = pdev->dev.of_node;
+       val = of_get_property(pdev->dev.of_node, "interrupt-driven", NULL);
+       if (val)
+               hw->interrupt = be32_to_cpup(val);
+       return 0;
+}
+#else
+static int __devinit altera_spi_of_probe(struct platform_device *pdev,
+                                      struct altera_spi *hw)
+{
+       return 0;
+}
+#endif
+
+static int __devinit altera_spi_probe(struct platform_device *pdev)
+{
+       struct altera_spi_platform_data *platp = pdev->dev.platform_data;
+       struct altera_spi *hw;
+       struct spi_master *master;
+       struct resource *res;
+       int err = 0;
+
+       master = spi_alloc_master(&pdev->dev, sizeof(struct altera_spi));
+       if (master == NULL) {
+               dev_err(&pdev->dev, "No memory for spi_master\n");
+               err = -ENOMEM;
+               goto err_no_mem;
+       }
+
+       /* setup the master state. */
+       master->bus_num = pdev->id;
+       master->num_chipselect = 16;
+       master->mode_bits = SPI_CS_HIGH;
+       master->setup = altera_spi_setup;
+
+       hw = spi_master_get_devdata(master);
+       platform_set_drvdata(pdev, hw);
+
+       /* setup the state for the bitbang driver */
+       hw->bitbang.master = spi_master_get(master);
+       if (hw->bitbang.master == NULL) {
+               dev_err(&pdev->dev, "Cannot get device\n");
+               err = -ENODEV;
+               goto err_no_dev;
+       }
+       hw->bitbang.setup_transfer = altera_spi_setupxfer;
+       hw->bitbang.chipselect = altera_spi_chipsel;
+       hw->bitbang.txrx_bufs = altera_spi_txrx;
+
+       /* find and map our resources */
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       if (res == NULL) {
+               dev_err(&pdev->dev, "Cannot get IORESOURCE_MEM\n");
+               err = -ENOENT;
+               goto err_no_iores;
+       }
+       hw->base = ioremap(res->start, (res->end - res->start) + 1);
+       if (hw->base == 0) {
+               dev_err(&pdev->dev, "Cannot map IO\n");
+               err = -ENXIO;
+               goto err_no_iomap;
+       }
+       /* program defaults into the registers */
+       hw->imr = 0;            /* disable spi interrupts */
+       writel(hw->imr, hw->base + ALTERA_SPI_CONTROL);
+       writel(0, hw->base + ALTERA_SPI_STATUS);        /* clear status reg */
+       if (readl(hw->base + ALTERA_SPI_STATUS) & ALTERA_SPI_STATUS_RRDY_MSK)
+               readl(hw->base + ALTERA_SPI_RXDATA);    /* flush rxdata */
+       /* irq is optional */
+       hw->irq = platform_get_irq(pdev, 0);
+       if (hw->irq >= 0) {
+               init_completion(&hw->done);
+               err = request_irq(hw->irq, altera_spi_irq, 0, pdev->name, hw);
+               if (err) {
+                       dev_err(&pdev->dev, "Cannot claim IRQ\n");
+                       goto err_no_irq;
+               }
+       }
+       /* find platform data */
+       if (platp) {
+               hw->interrupt = platp->interrupt;
+       } else {
+               err = altera_spi_of_probe(pdev, hw);
+               if (err)
+                       goto err_no_of;
+       }
+
+       /* register our spi controller */
+       err = spi_bitbang_start(&hw->bitbang);
+       if (err) {
+               dev_err(&pdev->dev, "Failed to register SPI master\n");
+               goto err_register;
+       }
+       dev_info(&pdev->dev, "base %p, irq %d\n", hw->base, hw->irq);
+
+       return 0;
+
+err_register:
+err_no_of:
+       if (hw->irq >= 0)
+               free_irq(hw->irq, hw);
+err_no_irq:
+       iounmap((void *)hw->base);

The cast should not be necessary.

+err_no_iomap:
+err_no_iores:
+       spi_master_put(master);;
+err_no_mem:
+err_no_dev:
+       return err;
+}
+
+static int __devexit altera_spi_remove(struct platform_device *dev)
+{
+       struct altera_spi *hw = platform_get_drvdata(dev);
+       struct spi_master *master = hw->bitbang.master;
+
+       spi_bitbang_stop(&hw->bitbang);
+
+       if (hw->irq >= 0)
+               free_irq(hw->irq, hw);
+       iounmap((void *)hw->base);

Ditto here.

+
+       platform_set_drvdata(dev, NULL);
+       spi_master_put(master);
+       return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id altera_spi_match[] = {
+       {
+               .compatible = "altera,spi_altera",
+       },
+       {},
+}
+MODULE_DEVICE_TABLE(of, altera_spi_match);
+#endif
+
+static struct platform_driver altera_spidrv = {
+       .remove = __devexit_p(altera_spi_remove),
+       .driver = {
+               .name = DRV_NAME,
+               .owner = THIS_MODULE,
+               .pm = NULL,
+#ifdef CONFIG_OF
+               .of_match_table = altera_spi_match,
+#endif
+       },
+};
+
+static int __init altera_spi_init(void)
+{
+       return platform_driver_probe(&altera_spidrv, altera_spi_probe);
+}
+
+static void __exit altera_spi_exit(void)
+{
+       platform_driver_unregister(&altera_spidrv);
+}
+
+module_init(altera_spi_init);

Move module_init line to be immediately after altera_spi_init() definition.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.