linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards
@ 2015-03-30 18:24 Bert Vermeulen
  2015-03-30 18:24 ` [PATCH v2 1/1] spi: Add SPI driver for Mikrotik RB4xx series boards Bert Vermeulen
  2015-03-31  8:44 ` [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Mark Brown
  0 siblings, 2 replies; 5+ messages in thread
From: Bert Vermeulen @ 2015-03-30 18:24 UTC (permalink / raw)
  To: ralf, broonie, linux-mips, linux-kernel, linux-spi, andy.shevchenko
  Cc: Bert Vermeulen

Changes in v2:
This is a near complete rewrite of the original OpenWrt driver. All comments
were taken into account, and the spi_transfer.fast_write flag is gone.
Instead, the cs_change flag is used. It's not too bad a hack, as it really
does use CS.

The CPLD module will be submitted as an MFD driver, as suggested.

Bert Vermeulen (1):
  spi: Add SPI driver for Mikrotik RB4xx series boards

 drivers/spi/Kconfig     |   6 ++
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-rb4xx.c | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 248 insertions(+)
 create mode 100644 drivers/spi/spi-rb4xx.c

-- 
1.9.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v2 1/1] spi: Add SPI driver for Mikrotik RB4xx series boards
  2015-03-30 18:24 [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Bert Vermeulen
@ 2015-03-30 18:24 ` Bert Vermeulen
  2015-03-30 19:14   ` Andy Shevchenko
  2015-03-31  8:44 ` [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Mark Brown
  1 sibling, 1 reply; 5+ messages in thread
From: Bert Vermeulen @ 2015-03-30 18:24 UTC (permalink / raw)
  To: ralf, broonie, linux-mips, linux-kernel, linux-spi, andy.shevchenko
  Cc: Bert Vermeulen

This driver mediates access between the connected CPLD and other devices
on the bus.

The m25p80-compatible boot flash and (some models) MMC use regular SPI,
bitbanged as required by the SoC. However the SPI-connected CPLD has
a "fast write" mode, in which two bits are transferred by SPI clock
cycle. The second bit is transmitted with the SoC's CS2 pin.

Protocol drivers using this fast write facility signal this by setting
the cs_change flag on transfers.

Signed-off-by: Bert Vermeulen <bert@biot.com>
---
 drivers/spi/Kconfig     |   6 ++
 drivers/spi/Makefile    |   1 +
 drivers/spi/spi-rb4xx.c | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 248 insertions(+)
 create mode 100644 drivers/spi/spi-rb4xx.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ab8dfbe..aa76ce5 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -429,6 +429,12 @@ config SPI_ROCKCHIP
 	  The main usecase of this controller is to use spi flash as boot
 	  device.
 
+config SPI_RB4XX
+	tristate "Mikrotik RB4XX SPI master"
+	depends on SPI_MASTER && ATH79_MACH_RB4XX
+	help
+	  SPI controller driver for the Mikrotik RB4xx series boards.
+
 config SPI_RSPI
 	tristate "Renesas RSPI/QSPI controller"
 	depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index d8cbf65..0218f39 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_SPI_PXA2XX)		+= spi-pxa2xx-platform.o
 obj-$(CONFIG_SPI_PXA2XX_PCI)		+= spi-pxa2xx-pci.o
 obj-$(CONFIG_SPI_QUP)			+= spi-qup.o
 obj-$(CONFIG_SPI_ROCKCHIP)		+= spi-rockchip.o
+obj-$(CONFIG_SPI_RB4XX)			+= spi-rb4xx.o
 obj-$(CONFIG_SPI_RSPI)			+= spi-rspi.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi-s3c24xx-hw.o
 spi-s3c24xx-hw-y			:= spi-s3c24xx.o
diff --git a/drivers/spi/spi-rb4xx.c b/drivers/spi/spi-rb4xx.c
new file mode 100644
index 0000000..9bbec9c
--- /dev/null
+++ b/drivers/spi/spi-rb4xx.c
@@ -0,0 +1,241 @@
+/*
+ * SPI controller driver for the Mikrotik RB4xx boards
+ *
+ * Copyright (C) 2010 Gabor Juhos <juhosg@openwrt.org>
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+#include <linux/spi/spi.h>
+
+#include <asm/mach-ath79/ar71xx_regs.h>
+
+#define SPI_CLOCK_FASTEST	0x40
+#define SPI_HZ			33333334
+#define CPLD_CMD_WRITE_NAND	0x08 /* bulk write mode */
+
+struct rb4xx_spi {
+	void __iomem		*base;
+	struct spi_master	*master;
+};
+
+static inline void do_spi_clk(void __iomem *base, u32 spi_ioc, int value)
+{
+	u32 regval;
+
+	regval = spi_ioc;
+	if (value & BIT(0))
+		regval |= AR71XX_SPI_IOC_DO;
+
+	__raw_writel(regval, base + AR71XX_SPI_REG_IOC);
+	__raw_writel(regval | AR71XX_SPI_IOC_CLK, base + AR71XX_SPI_REG_IOC);
+}
+
+static void do_spi_byte(void __iomem *base, u32 spi_ioc, u8 byte)
+{
+	int i;
+
+	for (i = 7; i >= 0; i--)
+		do_spi_clk(base, spi_ioc, byte >> i);
+}
+
+static inline void do_spi_clk_fast(void __iomem *base, u32 spi_ioc, u8 value)
+{
+	u32 regval;
+
+	regval = spi_ioc;
+	if (value & BIT(1))
+		regval |= AR71XX_SPI_IOC_DO;
+	if (value & BIT(0))
+		regval |= AR71XX_SPI_IOC_CS2;
+
+	__raw_writel(regval, base + AR71XX_SPI_REG_IOC);
+	__raw_writel(regval | AR71XX_SPI_IOC_CLK, base + AR71XX_SPI_REG_IOC);
+}
+
+/* Two bits at a time, msb first */
+static void do_spi_byte_fast(void __iomem *base, u32 spi_ioc, u8 byte)
+{
+	do_spi_clk_fast(base, spi_ioc, byte >> 6);
+	do_spi_clk_fast(base, spi_ioc, byte >> 4);
+	do_spi_clk_fast(base, spi_ioc, byte >> 2);
+	do_spi_clk_fast(base, spi_ioc, byte);
+}
+
+static void set_cs(struct spi_device *spi, bool enable)
+{
+	struct rb4xx_spi *rbspi = spi_master_get_devdata(spi->master);
+
+	/*
+	 * Setting CS is done along with bitbanging the actual values,
+	 * since it's all on the same hardware register. However the
+	 * CPLD needs CS deselected after every command.
+	 */
+	if (!enable)
+		__raw_writel(AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1,
+			     rbspi->base + AR71XX_SPI_REG_IOC);
+}
+
+/* Deselect CS0 and CS1. */
+static int unprep(struct spi_master *master)
+{
+	struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
+
+	__raw_writel(AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1,
+		     rbspi->base + AR71XX_SPI_REG_IOC);
+
+	return 0;
+}
+
+static int rb4xx_transfer_one(struct spi_master *master,
+			      struct spi_device *spi, struct spi_transfer *t)
+{
+	struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
+	int i;
+	u32 spi_ioc;
+	u8 *rx_buf;
+	const u8 *tx_buf;
+	unsigned char out;
+
+	/*
+	 * Prime the SPI register with the SPI device selected. The m25p80 boot
+	 * flash and CPLD share the CS0 pin. This works because the CPLD's
+	 * command set was designed to almost not clash with that of the
+	 * boot flash.
+	 */
+	if (spi->chip_select == 2)
+		spi_ioc = AR71XX_SPI_IOC_CS0;
+	else
+		spi_ioc = AR71XX_SPI_IOC_CS1;
+
+	tx_buf = t->tx_buf;
+	rx_buf = t->rx_buf;
+	for (i = 0; i < t->len; ++i) {
+		out = tx_buf ? tx_buf[i] : 0x00;
+		if (spi->chip_select == 1 && t->cs_change) {
+			/* CPLD in bulk write mode gets two bits per clock */
+			do_spi_byte_fast(rbspi->base, spi_ioc, out);
+			/* Don't want the real CS toggled */
+			t->cs_change = 0;
+		} else {
+			do_spi_byte(rbspi->base, spi_ioc, out);
+		}
+		if (!rx_buf)
+			continue;
+		rx_buf[i] = __raw_readl(rbspi->base + AR71XX_SPI_REG_RDS) & 0xff;
+	}
+	spi_finalize_current_transfer(master);
+
+	return 0;
+}
+
+static int rb4xx_spi_setup(struct spi_device *spi)
+{
+	if (spi->mode & ~(SPI_CS_HIGH)) {
+		dev_err(&spi->dev, "mode %x not supported\n", spi->mode);
+		return -EINVAL;
+	}
+
+	if (spi->bits_per_word != 8 && spi->bits_per_word != 0) {
+		dev_err(&spi->dev, "bits_per_word %u not supported\n",
+			spi->bits_per_word);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int rb4xx_spi_probe(struct platform_device *pdev)
+{
+	struct spi_master *master;
+	struct clk *ahb_clk;
+	struct rb4xx_spi *rbspi;
+	struct resource *r;
+	int err = 0;
+
+	ahb_clk = devm_clk_get(&pdev->dev, "ahb");
+	if (IS_ERR(ahb_clk))
+		return PTR_ERR(ahb_clk);
+
+	err = clk_prepare_enable(ahb_clk);
+	if (err)
+		return err;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!r)
+		return -ENOENT;
+
+	master = spi_alloc_master(&pdev->dev, sizeof(*rbspi));
+	if (!master)
+		return -ENOMEM;
+
+	master->bus_num = 0;
+	master->num_chipselect = 3;
+	master->setup = rb4xx_spi_setup;
+	master->transfer_one = rb4xx_transfer_one;
+	master->unprepare_transfer_hardware = unprep;
+	master->set_cs = set_cs;
+
+	rbspi = spi_master_get_devdata(master);
+
+	platform_set_drvdata(pdev, rbspi);
+
+	rbspi->base = devm_ioremap_resource(&pdev->dev, r);
+	if (!rbspi->base) {
+		err = -ENXIO;
+		goto err_put_master;
+	}
+
+	rbspi->master = master;
+
+	err = spi_register_master(master);
+	if (err) {
+		dev_err(&pdev->dev, "failed to register SPI master\n");
+		goto err_put_master;
+	}
+
+	/* Enable SPI */
+	__raw_writel(AR71XX_SPI_FS_GPIO, rbspi->base + AR71XX_SPI_REG_FS);
+
+	return 0;
+
+err_put_master:
+	spi_master_put(master);
+
+	return err;
+}
+
+static int rb4xx_spi_remove(struct platform_device *pdev)
+{
+	struct rb4xx_spi *rbspi = platform_get_drvdata(pdev);
+
+	platform_set_drvdata(pdev, NULL);
+	spi_master_put(rbspi->master);
+
+	return 0;
+}
+
+static struct platform_driver rb4xx_spi_drv = {
+	.probe		= rb4xx_spi_probe,
+	.remove		= rb4xx_spi_remove,
+	.driver		= {
+		.name	= "rb4xx-spi",
+	},
+};
+
+module_platform_driver(rb4xx_spi_drv);
+
+MODULE_DESCRIPTION("Mikrotik RB4xx SPI controller driver");
+MODULE_AUTHOR("Gabor Juhos <juhosg@openwrt.org>");
+MODULE_AUTHOR("Bert Vermeulen <bert@biot.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 1/1] spi: Add SPI driver for Mikrotik RB4xx series boards
  2015-03-30 18:24 ` [PATCH v2 1/1] spi: Add SPI driver for Mikrotik RB4xx series boards Bert Vermeulen
@ 2015-03-30 19:14   ` Andy Shevchenko
  0 siblings, 0 replies; 5+ messages in thread
From: Andy Shevchenko @ 2015-03-30 19:14 UTC (permalink / raw)
  To: Bert Vermeulen
  Cc: Ralf Baechle, Mark Brown, linux-mips, linux-kernel, linux-spi

On Mon, Mar 30, 2015 at 9:24 PM, Bert Vermeulen <bert@biot.com> wrote:
> This driver mediates access between the connected CPLD and other devices
> on the bus.
>
> The m25p80-compatible boot flash and (some models) MMC use regular SPI,
> bitbanged as required by the SoC. However the SPI-connected CPLD has
> a "fast write" mode, in which two bits are transferred by SPI clock
> cycle. The second bit is transmitted with the SoC's CS2 pin.
>
> Protocol drivers using this fast write facility signal this by setting
> the cs_change flag on transfers.

Looks nicer, still few comments below.

>
> Signed-off-by: Bert Vermeulen <bert@biot.com>
> ---
>  drivers/spi/Kconfig     |   6 ++
>  drivers/spi/Makefile    |   1 +
>  drivers/spi/spi-rb4xx.c | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 248 insertions(+)
>  create mode 100644 drivers/spi/spi-rb4xx.c
>
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index ab8dfbe..aa76ce5 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -429,6 +429,12 @@ config SPI_ROCKCHIP
>           The main usecase of this controller is to use spi flash as boot
>           device.
>
> +config SPI_RB4XX
> +       tristate "Mikrotik RB4XX SPI master"
> +       depends on SPI_MASTER && ATH79_MACH_RB4XX
> +       help
> +         SPI controller driver for the Mikrotik RB4xx series boards.
> +
>  config SPI_RSPI
>         tristate "Renesas RSPI/QSPI controller"
>         depends on SUPERH || ARCH_SHMOBILE || COMPILE_TEST
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index d8cbf65..0218f39 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -66,6 +66,7 @@ obj-$(CONFIG_SPI_PXA2XX)              += spi-pxa2xx-platform.o
>  obj-$(CONFIG_SPI_PXA2XX_PCI)           += spi-pxa2xx-pci.o
>  obj-$(CONFIG_SPI_QUP)                  += spi-qup.o
>  obj-$(CONFIG_SPI_ROCKCHIP)             += spi-rockchip.o
> +obj-$(CONFIG_SPI_RB4XX)                        += spi-rb4xx.o
>  obj-$(CONFIG_SPI_RSPI)                 += spi-rspi.o
>  obj-$(CONFIG_SPI_S3C24XX)              += spi-s3c24xx-hw.o
>  spi-s3c24xx-hw-y                       := spi-s3c24xx.o
> diff --git a/drivers/spi/spi-rb4xx.c b/drivers/spi/spi-rb4xx.c
> new file mode 100644
> index 0000000..9bbec9c
> --- /dev/null
> +++ b/drivers/spi/spi-rb4xx.c
> @@ -0,0 +1,241 @@
> +/*
> + * SPI controller driver for the Mikrotik RB4xx boards
> + *
> + * Copyright (C) 2010 Gabor Juhos <juhosg@openwrt.org>
> + *

There are two authors below but only one old copyright. I guess there
is should be another one dated 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/mach-ath79/ar71xx_regs.h>
> +
> +#define SPI_CLOCK_FASTEST      0x40
> +#define SPI_HZ                 33333334
> +#define CPLD_CMD_WRITE_NAND    0x08 /* bulk write mode */
> +
> +struct rb4xx_spi {
> +       void __iomem            *base;
> +       struct spi_master       *master;
> +};
> +
> +static inline void do_spi_clk(void __iomem *base, u32 spi_ioc, int value)
> +{
> +       u32 regval;
> +
> +       regval = spi_ioc;
> +       if (value & BIT(0))
> +               regval |= AR71XX_SPI_IOC_DO;
> +
> +       __raw_writel(regval, base + AR71XX_SPI_REG_IOC);
> +       __raw_writel(regval | AR71XX_SPI_IOC_CLK, base + AR71XX_SPI_REG_IOC);

So, why to avoid IO accessor helpers?

> +}
> +
> +static void do_spi_byte(void __iomem *base, u32 spi_ioc, u8 byte)
> +{
> +       int i;
> +
> +       for (i = 7; i >= 0; i--)

Just a matter of style, but you can use while-loop here as well.

> +               do_spi_clk(base, spi_ioc, byte >> i);
> +}
> +
> +static inline void do_spi_clk_fast(void __iomem *base, u32 spi_ioc, u8 value)
> +{
> +       u32 regval;
> +
> +       regval = spi_ioc;
> +       if (value & BIT(1))
> +               regval |= AR71XX_SPI_IOC_DO;
> +       if (value & BIT(0))
> +               regval |= AR71XX_SPI_IOC_CS2;
> +
> +       __raw_writel(regval, base + AR71XX_SPI_REG_IOC);
> +       __raw_writel(regval | AR71XX_SPI_IOC_CLK, base + AR71XX_SPI_REG_IOC);
> +}
> +
> +/* Two bits at a time, msb first */
> +static void do_spi_byte_fast(void __iomem *base, u32 spi_ioc, u8 byte)
> +{
> +       do_spi_clk_fast(base, spi_ioc, byte >> 6);
> +       do_spi_clk_fast(base, spi_ioc, byte >> 4);
> +       do_spi_clk_fast(base, spi_ioc, byte >> 2);
> +       do_spi_clk_fast(base, spi_ioc, byte);

Just a nitpick, what if explicitly show >> 0?

> +}
> +
> +static void set_cs(struct spi_device *spi, bool enable)
> +{
> +       struct rb4xx_spi *rbspi = spi_master_get_devdata(spi->master);
> +
> +       /*
> +        * Setting CS is done along with bitbanging the actual values,
> +        * since it's all on the same hardware register. However the
> +        * CPLD needs CS deselected after every command.
> +        */
> +       if (!enable)
> +               __raw_writel(AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1,
> +                            rbspi->base + AR71XX_SPI_REG_IOC);
> +}
> +
> +/* Deselect CS0 and CS1. */
> +static int unprep(struct spi_master *master)
> +{
> +       struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
> +
> +       __raw_writel(AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1,
> +                    rbspi->base + AR71XX_SPI_REG_IOC);
> +
> +       return 0;
> +}
> +
> +static int rb4xx_transfer_one(struct spi_master *master,
> +                             struct spi_device *spi, struct spi_transfer *t)
> +{
> +       struct rb4xx_spi *rbspi = spi_master_get_devdata(master);
> +       int i;
> +       u32 spi_ioc;
> +       u8 *rx_buf;
> +       const u8 *tx_buf;
> +       unsigned char out;
> +
> +       /*
> +        * Prime the SPI register with the SPI device selected. The m25p80 boot
> +        * flash and CPLD share the CS0 pin. This works because the CPLD's
> +        * command set was designed to almost not clash with that of the
> +        * boot flash.
> +        */
> +       if (spi->chip_select == 2)
> +               spi_ioc = AR71XX_SPI_IOC_CS0;
> +       else
> +               spi_ioc = AR71XX_SPI_IOC_CS1;
> +
> +       tx_buf = t->tx_buf;
> +       rx_buf = t->rx_buf;
> +       for (i = 0; i < t->len; ++i) {
> +               out = tx_buf ? tx_buf[i] : 0x00;
> +               if (spi->chip_select == 1 && t->cs_change) {
> +                       /* CPLD in bulk write mode gets two bits per clock */
> +                       do_spi_byte_fast(rbspi->base, spi_ioc, out);
> +                       /* Don't want the real CS toggled */
> +                       t->cs_change = 0;
> +               } else {
> +                       do_spi_byte(rbspi->base, spi_ioc, out);
> +               }
> +               if (!rx_buf)
> +                       continue;
> +               rx_buf[i] = __raw_readl(rbspi->base + AR71XX_SPI_REG_RDS) & 0xff;

Since you have u8 type you don need to & 0xff.

> +       }
> +       spi_finalize_current_transfer(master);
> +
> +       return 0;
> +}
> +
> +static int rb4xx_spi_setup(struct spi_device *spi)
> +{
> +       if (spi->mode & ~(SPI_CS_HIGH)) {

Redundant parens.

> +               dev_err(&spi->dev, "mode %x not supported\n", spi->mode);
> +               return -EINVAL;
> +       }
> +
> +       if (spi->bits_per_word != 8 && spi->bits_per_word != 0) {
> +               dev_err(&spi->dev, "bits_per_word %u not supported\n",
> +                       spi->bits_per_word);
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int rb4xx_spi_probe(struct platform_device *pdev)
> +{
> +       struct spi_master *master;
> +       struct clk *ahb_clk;
> +       struct rb4xx_spi *rbspi;
> +       struct resource *r;
> +       int err = 0;
> +
> +       ahb_clk = devm_clk_get(&pdev->dev, "ahb");
> +       if (IS_ERR(ahb_clk))
> +               return PTR_ERR(ahb_clk);
> +
> +       err = clk_prepare_enable(ahb_clk);
> +       if (err)
> +               return err;
> +
> +       r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!r)
> +               return -ENOENT;

(a)

> +
> +       master = spi_alloc_master(&pdev->dev, sizeof(*rbspi));
> +       if (!master)
> +               return -ENOMEM;
> +
> +       master->bus_num = 0;
> +       master->num_chipselect = 3;
> +       master->setup = rb4xx_spi_setup;
> +       master->transfer_one = rb4xx_transfer_one;
> +       master->unprepare_transfer_hardware = unprep;
> +       master->set_cs = set_cs;
> +
> +       rbspi = spi_master_get_devdata(master);
> +
> +       platform_set_drvdata(pdev, rbspi);
> +
> +       rbspi->base = devm_ioremap_resource(&pdev->dev, r);
> +       if (!rbspi->base) {
> +               err = -ENXIO;
> +               goto err_put_master;
> +       }

(b)

First of all you don't need to check for return code in (a) since (b)
will do this for you. Second, you have to propagate the actual error
code in (b). And third, since you are using devm_* approach you may
move this before master allocation and save few lines.

> +
> +       rbspi->master = master;
> +
> +       err = spi_register_master(master);
> +       if (err) {
> +               dev_err(&pdev->dev, "failed to register SPI master\n");
> +               goto err_put_master;
> +       }
> +
> +       /* Enable SPI */
> +       __raw_writel(AR71XX_SPI_FS_GPIO, rbspi->base + AR71XX_SPI_REG_FS);
> +
> +       return 0;
> +
> +err_put_master:
> +       spi_master_put(master);
> +
> +       return err;
> +}
> +
> +static int rb4xx_spi_remove(struct platform_device *pdev)
> +{
> +       struct rb4xx_spi *rbspi = platform_get_drvdata(pdev);
> +
> +       platform_set_drvdata(pdev, NULL);

This is redundant.

> +       spi_master_put(rbspi->master);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver rb4xx_spi_drv = {
> +       .probe          = rb4xx_spi_probe,
> +       .remove         = rb4xx_spi_remove,
> +       .driver         = {
> +               .name   = "rb4xx-spi",
> +       },
> +};
> +
> +module_platform_driver(rb4xx_spi_drv);
> +
> +MODULE_DESCRIPTION("Mikrotik RB4xx SPI controller driver");
> +MODULE_AUTHOR("Gabor Juhos <juhosg@openwrt.org>");
> +MODULE_AUTHOR("Bert Vermeulen <bert@biot.com>");
> +MODULE_LICENSE("GPL v2");
> --
> 1.9.1
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards
  2015-03-30 18:24 [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Bert Vermeulen
  2015-03-30 18:24 ` [PATCH v2 1/1] spi: Add SPI driver for Mikrotik RB4xx series boards Bert Vermeulen
@ 2015-03-31  8:44 ` Mark Brown
  2015-03-31 14:22   ` Ralf Baechle
  1 sibling, 1 reply; 5+ messages in thread
From: Mark Brown @ 2015-03-31  8:44 UTC (permalink / raw)
  To: Bert Vermeulen; +Cc: ralf, linux-mips, linux-kernel, linux-spi, andy.shevchenko

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

On Mon, Mar 30, 2015 at 08:24:16PM +0200, Bert Vermeulen wrote:
> Changes in v2:
> This is a near complete rewrite of the original OpenWrt driver. All comments
> were taken into account, and the spi_transfer.fast_write flag is gone.
> Instead, the cs_change flag is used. It's not too bad a hack, as it really
> does use CS.

Don't send cover letters for single patches, if there's anything that
needs saying it should either be in the changelog or after the ---.  A
separate cover letter adds to the mail volume and probably means that
the patch itself is inadequately described.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards
  2015-03-31  8:44 ` [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Mark Brown
@ 2015-03-31 14:22   ` Ralf Baechle
  0 siblings, 0 replies; 5+ messages in thread
From: Ralf Baechle @ 2015-03-31 14:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bert Vermeulen, linux-mips, linux-kernel, linux-spi, andy.shevchenko

On Tue, Mar 31, 2015 at 09:44:25AM +0100, Mark Brown wrote:

> On Mon, Mar 30, 2015 at 08:24:16PM +0200, Bert Vermeulen wrote:
> > Changes in v2:
> > This is a near complete rewrite of the original OpenWrt driver. All comments
> > were taken into account, and the spi_transfer.fast_write flag is gone.
> > Instead, the cs_change flag is used. It's not too bad a hack, as it really
> > does use CS.
> 
> Don't send cover letters for single patches, if there's anything that
> needs saying it should either be in the changelog or after the ---.  A
> separate cover letter adds to the mail volume and probably means that
> the patch itself is inadequately described.

My personal grief with cover letters is that patchwork only collects
patches but cover letters themselves aren't patches.  Which means working
through patchwork I might miss important information.  Maybe I should
request an enhancement to patchwork.

  Ralf

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2015-03-31 14:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-30 18:24 [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Bert Vermeulen
2015-03-30 18:24 ` [PATCH v2 1/1] spi: Add SPI driver for Mikrotik RB4xx series boards Bert Vermeulen
2015-03-30 19:14   ` Andy Shevchenko
2015-03-31  8:44 ` [PATCH v2 0/1] spi: Add driver for Routerboard RB4xx boards Mark Brown
2015-03-31 14:22   ` Ralf Baechle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).