Re: [PATCH 1/2] spi: Add SPI driver for Mikrotik RB4xx series boards

[Andy Shevchenko]

On Fri, Mar 20, 2015 at 2:16 PM, Bert Vermeulen <bert@xxxxxxxx> wrote:

Besides what Mark told you (I mostly about absence of the commit
message) there are few more comments below.

> Signed-off-by: Bert Vermeulen <bert@xxxxxxxx>

> +++ b/drivers/spi/spi-rb4xx.c
> @@ -0,0 +1,419 @@
> +/*
> + * SPI controller driver for the Mikrotik RB4xx boards
> + *
> + * Copyright (C) 2010 Gabor Juhos <juhosg@xxxxxxxxxxx>

2010, 2015 ?

> + *
> + * This file was based on the patches for Linux 2.6.27.39 published by
> + * MikroTik for their RouterBoard 4xx series devices.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/delay.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +#include <linux/platform_device.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/mach-ath79/ar71xx_regs.h>
> +#include <asm/mach-ath79/ath79.h>
> +
> +#define DRV_NAME       "rb4xx-spi"
> +#define DRV_DESC       "Mikrotik RB4xx SPI controller driver"
> +#define DRV_VERSION    "0.1.0"
> +
> +#define SPI_CTRL_FASTEST       0x40
> +#define SPI_HZ                 33333334
> +
> +#undef RB4XX_SPI_DEBUG

Why is it here?

> +
> +struct rb4xx_spi {
> +       void __iomem            *base;
> +       struct spi_master       *master;
> +
> +       unsigned                spi_ctrl;
> +
> +       struct clk              *ahb_clk;
> +       unsigned long           ahb_freq;
> +
> +       spinlock_t              lock;
> +       struct list_head        queue;
> +       int                     busy:1;
> +       int                     cs_wait;
> +};
> +
> +static unsigned spi_clk_low = AR71XX_SPI_IOC_CS1;
> +
> +#ifdef RB4XX_SPI_DEBUG
> +static inline void do_spi_delay(void)
> +{
> +       ndelay(20000);
> +}
> +#else
> +static inline void do_spi_delay(void) { }
> +#endif
> +
> +static inline void do_spi_init(struct spi_device *spi)
> +{
> +       unsigned cs = AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1;
> +
> +       if (!(spi->mode & SPI_CS_HIGH))
> +               cs ^= (spi->chip_select == 2) ? AR71XX_SPI_IOC_CS1 :
> +                                               AR71XX_SPI_IOC_CS0;

What is the magic CS == 2?

> +
> +       spi_clk_low = cs;
> +}
> +
> +static inline void do_spi_finish(void __iomem *base)
> +{
> +       do_spi_delay();
> +       __raw_writel(AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1,
> +                    base + AR71XX_SPI_REG_IOC);

I highly recommend you to provide pair of inliners to access hardware.
It would be easy to maintainâ

> +}
> +
> +static inline void do_spi_clk(void __iomem *base, int bit)
> +{
> +       unsigned bval = spi_clk_low | ((bit & 1) ? AR71XX_SPI_IOC_DO : 0);
> +
> +       do_spi_delay();
> +       __raw_writel(bval, base + AR71XX_SPI_REG_IOC);

âand you may easily provide rb4xx_writel_delayed() as well.

> +       do_spi_delay();
> +       __raw_writel(bval | AR71XX_SPI_IOC_CLK, base + AR71XX_SPI_REG_IOC);
> +}
> +
> +static void do_spi_byte(void __iomem *base, unsigned char byte)
> +{
> +       do_spi_clk(base, byte >> 7);
> +       do_spi_clk(base, byte >> 6);
> +       do_spi_clk(base, byte >> 5);
> +       do_spi_clk(base, byte >> 4);
> +       do_spi_clk(base, byte >> 3);
> +       do_spi_clk(base, byte >> 2);
> +       do_spi_clk(base, byte >> 1);
> +       do_spi_clk(base, byte);
> +
> +       pr_debug("spi_byte sent 0x%02x got 0x%02x\n",
> +                (unsigned)byte,
> +                (unsigned char)__raw_readl(base + AR71XX_SPI_REG_RDS));
> +}
> +
> +static inline void do_spi_clk_fast(void __iomem *base, unsigned bit1,
> +                                  unsigned bit2)
> +{
> +       unsigned bval = (spi_clk_low |
> +                        ((bit1 & 1) ? AR71XX_SPI_IOC_DO : 0) |
> +                        ((bit2 & 1) ? AR71XX_SPI_IOC_CS2 : 0));

Regarding to the usage, why not to provide one variable with two bits?

> +       do_spi_delay();
> +       __raw_writel(bval, base + AR71XX_SPI_REG_IOC);
> +       do_spi_delay();
> +       __raw_writel(bval | AR71XX_SPI_IOC_CLK, base + AR71XX_SPI_REG_IOC);
> +}
> +
> +static void do_spi_byte_fast(void __iomem *base, unsigned char byte)
> +{
> +       do_spi_clk_fast(base, byte >> 7, byte >> 6);
> +       do_spi_clk_fast(base, byte >> 5, byte >> 4);
> +       do_spi_clk_fast(base, byte >> 3, byte >> 2);
> +       do_spi_clk_fast(base, byte >> 1, byte >> 0);
> +
> +       pr_debug("spi_byte_fast sent 0x%02x got 0x%02x\n",
> +                (unsigned)byte,
> +                (unsigned char) __raw_readl(base + AR71XX_SPI_REG_RDS));
> +}
> +
> +static int rb4xx_spi_txrx(void __iomem *base, struct spi_transfer *t)
> +{
> +       const unsigned char *rxv_ptr = NULL;
> +       const unsigned char *tx_ptr = t->tx_buf;
> +       unsigned char *rx_ptr = t->rx_buf;
> +       unsigned i;
> +
> +       pr_debug("spi_txrx len %u tx %u rx %u\n", t->len,
> +                (t->tx_buf ? 1 : 0),

!!t->tx_buf ?
Or personally I prefer to see %p here.

> +                (t->rx_buf ? 1 : 0));

Ditto.

> +
> +       for (i = 0; i < t->len; ++i) {
> +               unsigned char sdata = tx_ptr ? tx_ptr[i] : 0;
> +
> +               if (t->fast_write)
> +                       do_spi_byte_fast(base, sdata);
> +               else
> +                       do_spi_byte(base, sdata);
> +
> +               if (rx_ptr) {
> +                       rx_ptr[i] = __raw_readl(base + AR71XX_SPI_REG_RDS) & 0xff;
> +               } else if (rxv_ptr) {
> +                       unsigned char c = __raw_readl(base + AR71XX_SPI_REG_RDS);
> +
> +                       if (rxv_ptr[i] != c)
> +                               return i;
> +               }
> +       }
> +
> +       return i;
> +}
> +
> +static int rb4xx_spi_msg(struct rb4xx_spi *rbspi, struct spi_message *m)
> +{
> +       struct spi_transfer *t = NULL;
> +       void __iomem *base = rbspi->base;
> +
> +       m->status = 0;
> +       if (list_empty(&m->transfers))
> +               return -1;
> +
> +       __raw_writel(AR71XX_SPI_FS_GPIO, base + AR71XX_SPI_REG_FS);
> +       __raw_writel(SPI_CTRL_FASTEST, base + AR71XX_SPI_REG_CTRL);
> +       do_spi_init(m->spi);
> +
> +       list_for_each_entry(t, &m->transfers, transfer_list) {
> +               int len;
> +
> +               len = rb4xx_spi_txrx(base, t);
> +               if (len != t->len) {
> +                       m->status = -EMSGSIZE;
> +                       break;
> +               }
> +               m->actual_length += len;
> +
> +               if (t->cs_change) {
> +                       if (list_is_last(&t->transfer_list, &m->transfers)) {
> +                               /* wait for continuation */
> +                               return m->spi->chip_select;
> +                       }
> +                       do_spi_finish(base);
> +                       ndelay(100);
> +               }
> +       }
> +
> +       do_spi_finish(base);
> +       __raw_writel(rbspi->spi_ctrl, base + AR71XX_SPI_REG_CTRL);
> +       __raw_writel(0, base + AR71XX_SPI_REG_FS);
> +       return -1;
> +}
> +
> +static void rb4xx_spi_process_queue_locked(struct rb4xx_spi *rbspi,
> +                                          unsigned long *flags)
> +{
> +       int cs = rbspi->cs_wait;
> +
> +       rbspi->busy = 1;
> +       while (!list_empty(&rbspi->queue)) {
> +               struct spi_message *m;
> +
> +               list_for_each_entry(m, &rbspi->queue, queue)
> +                       if (cs < 0 || cs == m->spi->chip_select)
> +                               break;
> +
> +               if (&m->queue == &rbspi->queue)
> +                       break;
> +
> +               list_del_init(&m->queue);
> +               spin_unlock_irqrestore(&rbspi->lock, *flags);
> +
> +               cs = rb4xx_spi_msg(rbspi, m);
> +               m->complete(m->context);
> +
> +               spin_lock_irqsave(&rbspi->lock, *flags);
> +       }
> +
> +       rbspi->cs_wait = cs;
> +       rbspi->busy = 0;
> +
> +       if (cs >= 0) {
> +               /* TODO: add timer to unlock cs after 1s inactivity */
> +       }
> +}
> +
> +static int rb4xx_spi_transfer(struct spi_device *spi,
> +                             struct spi_message *m)
> +{
> +       struct rb4xx_spi *rbspi = spi_master_get_devdata(spi->master);
> +       unsigned long flags;
> +
> +       m->actual_length = 0;
> +       m->status = -EINPROGRESS;
> +
> +       spin_lock_irqsave(&rbspi->lock, flags);
> +       list_add_tail(&m->queue, &rbspi->queue);
> +       if (rbspi->busy ||
> +           (rbspi->cs_wait >= 0 && rbspi->cs_wait != m->spi->chip_select)) {
> +               /* job will be done later */
> +               spin_unlock_irqrestore(&rbspi->lock, flags);
> +               return 0;
> +       }
> +
> +       /* process job in current context */
> +       rb4xx_spi_process_queue_locked(rbspi, &flags);
> +       spin_unlock_irqrestore(&rbspi->lock, flags);
> +
> +       return 0;
> +}
> +
> +static int rb4xx_spi_setup(struct spi_device *spi)
> +{
> +       struct rb4xx_spi *rbspi = spi_master_get_devdata(spi->master);
> +       unsigned long flags;
> +
> +       if (spi->mode & ~(SPI_CS_HIGH)) {
> +               dev_err(&spi->dev, "mode %x not supported\n",
> +                       (unsigned) spi->mode);

Why explicitly casted?

> +               return -EINVAL;
> +       }
> +
> +       if (spi->bits_per_word != 8 && spi->bits_per_word != 0) {
> +               dev_err(&spi->dev, "bits_per_word %u not supported\n",
> +                       (unsigned) spi->bits_per_word);

Ditto.

> +               return -EINVAL;
> +       }
> +
> +       spin_lock_irqsave(&rbspi->lock, flags);
> +       if (rbspi->cs_wait == spi->chip_select && !rbspi->busy) {
> +               rbspi->cs_wait = -1;
> +               rb4xx_spi_process_queue_locked(rbspi, &flags);
> +       }
> +       spin_unlock_irqrestore(&rbspi->lock, flags);
> +
> +       return 0;
> +}
> +
> +static unsigned get_spi_ctrl(struct rb4xx_spi *rbspi)
> +{
> +       unsigned div;
> +
> +       div = (rbspi->ahb_freq - 1) / (2 * SPI_HZ);
> +
> +       /*
> +        * CPU has a bug at (div == 0) - first bit read is random
> +        */

Would it be one line?

> +       if (div == 0)
> +               ++div;

div++ will work as well.

> +
> +       return SPI_CTRL_FASTEST + div;
> +}
> +
> +static int rb4xx_spi_probe(struct platform_device *pdev)
> +{
> +       struct spi_master *master;
> +       struct rb4xx_spi *rbspi;
> +       struct resource *r;
> +       int err = 0;
> +
> +       master = spi_alloc_master(&pdev->dev, sizeof(*rbspi));

What if you set clock, get resources first and then allocate master?

> +       if (!master) {
> +               err = -ENOMEM;
> +               goto err_out;
> +       }
> +
> +       master->bus_num = 0;
> +       master->num_chipselect = 3;
> +       master->setup = rb4xx_spi_setup;
> +       master->transfer = rb4xx_spi_transfer;
> +
> +       rbspi = spi_master_get_devdata(master);
> +
> +       rbspi->ahb_clk = clk_get(&pdev->dev, "ahb");

What about devm_*, here devm_clk_get()?

> +       if (IS_ERR(rbspi->ahb_clk)) {
> +               err = PTR_ERR(rbspi->ahb_clk);
> +               goto err_put_master;
> +       }
> +
> +       err = clk_enable(rbspi->ahb_clk);

Shouldn't be clk_prepare_enable()?

> +       if (err)
> +               goto err_clk_put;
> +
> +       rbspi->ahb_freq = clk_get_rate(rbspi->ahb_clk);
> +       if (!rbspi->ahb_freq) {
> +               err = -EINVAL;
> +               goto err_clk_disable;
> +       }
> +
> +       platform_set_drvdata(pdev, rbspi);
> +
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!r) {
> +               err = -ENOENT;
> +               goto err_clk_disable;
> +       }
> +
> +       rbspi->base = ioremap(r->start, r->end - r->start + 1);
> +       if (!rbspi->base) {
> +               err = -ENXIO;
> +               goto err_clk_disable;
> +       }

devm_ioremap_resource()

> +
> +       rbspi->master = master;
> +       rbspi->spi_ctrl = get_spi_ctrl(rbspi);
> +       rbspi->cs_wait = -1;
> +
> +       spin_lock_init(&rbspi->lock);
> +       INIT_LIST_HEAD(&rbspi->queue);
> +
> +       err = spi_register_master(master);
> +       if (err) {
> +               dev_err(&pdev->dev, "failed to register SPI master\n");
> +               goto err_iounmap;
> +       }
> +
> +       return 0;
> +
> +err_iounmap:
> +       iounmap(rbspi->base);

Gone with devm_*

> +err_clk_disable:
> +       clk_disable(rbspi->ahb_clk);

clk_disable_unprepare

> +err_clk_put:
> +       clk_put(rbspi->ahb_clk);

Ditto.

> +err_put_master:
> +       platform_set_drvdata(pdev, NULL);

Not needed.

> +       spi_master_put(master);
> +err_out:
> +       return err;
> +}
> +
> +static int rb4xx_spi_remove(struct platform_device *pdev)
> +{
> +       struct rb4xx_spi *rbspi = platform_get_drvdata(pdev);
> +
> +       iounmap(rbspi->base);

Will gone with devm_*.

> +       clk_disable(rbspi->ahb_clk);

clk_disable_unprepare()

> +       clk_put(rbspi->ahb_clk);

Ditto.

> +       platform_set_drvdata(pdev, NULL);

Not needed.

> +       spi_master_put(rbspi->master);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver rb4xx_spi_drv = {
> +       .probe          = rb4xx_spi_probe,
> +       .remove         = rb4xx_spi_remove,
> +       .driver         = {
> +               .name   = DRV_NAME,
> +               .owner  = THIS_MODULE,

Not needed.

> +       },
> +};
> +
> +static int __init rb4xx_spi_init(void)
> +{
> +       return platform_driver_register(&rb4xx_spi_drv);
> +}
> +subsys_initcall(rb4xx_spi_init);
> +
> +static void __exit rb4xx_spi_exit(void)
> +{
> +       platform_driver_unregister(&rb4xx_spi_drv);
> +}
> +
> +module_exit(rb4xx_spi_exit);
> +
> +MODULE_DESCRIPTION(DRV_DESC);
> +MODULE_VERSION(DRV_VERSION);
> +MODULE_AUTHOR("Gabor Juhos <juhosg@xxxxxxxxxxx>");
> +MODULE_LICENSE("GPL v2");

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/