[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Marek Vasut]

This is slightly reworked version of the SPI driver.
Support for DT has been added and it's been converted
to queued API.

Based on previous attempt by:
Fabio Estevam <fabio.estevam@freescale.com>

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Chris Ball <cjb@laptop.org>
Cc: Detlev Zundel <dzu@denx.de>
CC: Dong Aisheng <b29396@freescale.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
Cc: Rob Herring <rob.herring@calxeda.com>
CC: Shawn Guo <shawn.guo@linaro.org>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Wolfgang Denk <wd@denx.de>
---
 drivers/spi/Kconfig   |    6 +
 drivers/spi/Makefile  |    1 +
 drivers/spi/spi-mxs.c |  407 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)
 create mode 100644 drivers/spi/spi-mxs.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index cd2fe35..85ca6a7 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -355,6 +355,12 @@ config SPI_STMP3XXX
 	help
 	  SPI driver for Freescale STMP37xx/378x SoC SSP interface
 
+config SPI_MXS
+	tristate "Freescale MXS SPI controller"
+	depends on ARCH_MXS
+	help
+	  SPI driver for Freescale MXS devices.
+
 config SPI_TEGRA
 	tristate "Nvidia Tegra SPI controller"
 	depends on ARCH_TEGRA && TEGRA_SYSTEM_DMA
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 9d75d21..a684d1e 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
 obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
+obj-$(CONFIG_SPI_MXS)			+= spi-mxs.o
 obj-$(CONFIG_SPI_NUC900)		+= spi-nuc900.o
 obj-$(CONFIG_SPI_OC_TINY)		+= spi-oc-tiny.o
 obj-$(CONFIG_SPI_OMAP_UWIRE)		+= spi-omap-uwire.o
diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
new file mode 100644
index 0000000..81a2643
--- /dev/null
+++ b/drivers/spi/spi-mxs.c
@@ -0,0 +1,407 @@
+/*
+ * Freescale MXS SPI master driver
+ *
+ * Copyright 2012 DENX Software Engineering, GmbH.
+ * Copyright 2012 Freescale Semiconductor, Inc.
+ * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
+ *
+ * Rework and transition to new API by:
+ * Marek Vasut <marex@denx.de>
+ *
+ * Based on previous attempt by:
+ * Fabio Estevam <fabio.estevam@freescale.com>
+ *
+ * Based on code from U-Boot bootloader by:
+ * Marek Vasut <marex@denx.de>
+ *
+ * Based on spi-stmp.c, which is:
+ * Author: Dmitry Pervushin <dimka@embeddedalley.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.
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/highmem.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/completion.h>
+#include <linux/gpio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/module.h>
+#include <linux/fsl/mxs-dma.h>
+#include <linux/pinctrl/consumer.h>
+#include <linux/stmp_device.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mxs-spi.h>
+
+#define SSP_TIMEOUT		200	/* 200 ms */
+
+static int mxs_spi_setup_transfer(struct spi_device *spi,
+				struct spi_transfer *t)
+{
+	struct mxs_ssp *ssp = spi_master_get_devdata(spi->master);
+	uint8_t bits_per_word;
+	uint32_t hz = 0;
+
+	bits_per_word = spi->bits_per_word;
+	if (t && t->bits_per_word)
+		bits_per_word = t->bits_per_word;
+
+	if (bits_per_word != 8) {
+		dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
+					__func__, bits_per_word);
+		return -EINVAL;
+	}
+
+	if (spi->max_speed_hz)
+		hz = spi->max_speed_hz;
+	if (t && t->speed_hz)
+		hz = t->speed_hz;
+	if (hz == 0) {
+		dev_err(&spi->dev, "Cannot continue with zero clock\n");
+		return -EINVAL;
+	}
+
+	mxs_ssp_set_clk_rate(ssp, hz);
+
+	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
+		     BF_SSP_CTRL1_WORD_LENGTH
+		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
+		     ((spi->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
+		     ((spi->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
+		     ssp->base + HW_SSP_CTRL1(ssp));
+
+	writel(0x0, ssp->base + HW_SSP_CMD0 + STMP_OFFSET_REG_SET);
+
+	return 0;
+}
+
+static void mxs_spi_cleanup(struct spi_device *spi)
+{
+	return;
+}
+
+/* the spi->mode bits understood by this driver: */
+static int mxs_spi_setup(struct spi_device *spi)
+{
+	int err = 0;
+
+	if (!spi->bits_per_word)
+		spi->bits_per_word = 8;
+
+	if (spi->mode & ~(SPI_CPOL | SPI_CPHA))
+		return -EINVAL;
+
+	err = mxs_spi_setup_transfer(spi, NULL);
+	if (err) {
+		dev_err(&spi->dev,
+			"Failed to setup transfer, error = %d\n", err);
+	}
+
+	return err;
+}
+
+static void mxs_spi_set_cs(struct mxs_ssp *ssp, unsigned cs)
+{
+	const uint32_t mask =
+		BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
+	uint32_t select = 0;
+
+	writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+
+	if (cs & 1)
+		select |= BM_SSP_CTRL0_WAIT_FOR_CMD;
+	if (cs & 2)
+		select |= BM_SSP_CTRL0_WAIT_FOR_IRQ;
+
+	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
+
+static inline void mxs_spi_enable(struct mxs_ssp *ssp)
+{
+	writel(BM_SSP_CTRL0_LOCK_CS,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+}
+
+static inline void mxs_spi_disable(struct mxs_ssp *ssp)
+{
+	writel(BM_SSP_CTRL0_LOCK_CS,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+	writel(BM_SSP_CTRL0_IGNORE_CRC,
+		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+}
+
+static int mxs_ssp_wait_set(struct mxs_ssp *ssp, int offset, int mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
+
+	while (!(readl_relaxed(ssp->base + offset) & mask)) {
+		udelay(1);
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int mxs_ssp_wait_clr(struct mxs_ssp *ssp, int offset, int mask)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
+
+	while ((readl_relaxed(ssp->base + offset) & mask)) {
+		udelay(1);
+		if (time_after(jiffies, timeout))
+			return -ETIMEDOUT;
+	}
+	return 0;
+}
+
+static int mxs_spi_txrx_pio(struct mxs_ssp *ssp, int cs,
+			    unsigned char *buf, int len,
+			    int *first, int *last, int write)
+{
+	if (*first) {
+		mxs_spi_enable(ssp);
+		*first = 0;
+	}
+
+	mxs_spi_set_cs(ssp, cs);
+
+	while (len--) {
+		if (*last && len == 0) {
+			mxs_spi_disable(ssp);
+			*last = 0;
+		}
+
+		if (ssp->devid == IMX23_SSP) {
+			writel(BM_SSP_CTRL0_XFER_COUNT,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+			writel(1,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+		} else {
+			writel(1, ssp->base + HW_SSP_XFER_SIZE);
+		}
+
+		if (write)
+			writel(BM_SSP_CTRL0_READ,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
+		else
+			writel(BM_SSP_CTRL0_READ,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
+		writel(BM_SSP_CTRL0_RUN,
+				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
+		if (mxs_ssp_wait_set(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
+			return -ETIMEDOUT;
+
+		if (write)
+			writel(*buf, ssp->base + HW_SSP_DATA);
+
+		writel(BM_SSP_CTRL0_DATA_XFER,
+			     ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
+
+		if (!write) {
+			if (mxs_ssp_wait_clr(ssp, HW_SSP_STATUS(ssp),
+						BM_SSP_STATUS_FIFO_EMPTY))
+				return -ETIMEDOUT;
+
+			*buf = (readl(ssp->base + HW_SSP_DATA) & 0xff);
+		}
+
+		if (mxs_ssp_wait_clr(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
+			return -ETIMEDOUT;
+
+		buf++;
+	}
+
+	if (len <= 0)
+		return 0;
+
+	return -ETIMEDOUT;
+}
+
+static int mxs_spi_transfer_one(struct spi_master *spi, struct spi_message *m)
+{
+	struct mxs_ssp *ssp = spi_master_get_devdata(spi);
+	int first, last;
+	struct spi_transfer *t, *tmp_t;
+	int status = 0;
+	int cs;
+
+	first = last = 0;
+
+	cs = m->spi->chip_select;
+
+	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
+
+		mxs_spi_setup_transfer(m->spi, t);
+
+		if (&t->transfer_list == m->transfers.next)
+			first = !0;
+		if (&t->transfer_list == m->transfers.prev)
+			last = !0;
+		if (t->rx_buf && t->tx_buf) {
+			pr_debug("%s: cannot send and receive simultaneously\n",
+				 __func__);
+			return -EINVAL;
+		}
+
+		if (t->tx_buf)
+			status = mxs_spi_txrx_pio(ssp, cs, (void *)t->tx_buf,
+					     t->len, &first, &last, 1);
+		if (t->rx_buf)
+			status = mxs_spi_txrx_pio(ssp, cs, t->rx_buf,
+					     t->len, &first, &last, 0);
+		m->actual_length += t->len;
+		if (status)
+			break;
+
+		first = last = 0;
+	}
+
+	m->status = 0;
+	spi_finalize_current_message(spi);
+
+	return status;
+}
+
+static const struct of_device_id mxs_spi_dt_ids[] = {
+	{ .compatible = "fsl,imx23-spi", .data = (void *) IMX23_SSP, },
+	{ .compatible = "fsl,imx28-spi", .data = (void *) IMX28_SSP, },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, mxs_spi_dt_ids);
+
+static int __devinit mxs_spi_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *of_id =
+			of_match_device(mxs_spi_dt_ids, &pdev->dev);
+	struct device_node *np = pdev->dev.of_node;
+	struct spi_master *host;
+	struct mxs_ssp *ssp;
+	struct resource *iores;
+	struct pinctrl *pinctrl;
+	int ret = 0;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iores)
+		return -EINVAL;
+
+	host = spi_alloc_master(&pdev->dev, sizeof(struct mxs_ssp));
+	if (!host)
+		return -ENOMEM;
+
+	ssp = spi_master_get_devdata(host);
+	ssp->dev = &pdev->dev;
+	ssp->base = devm_request_and_ioremap(&pdev->dev, iores);
+	if (!ssp->base) {
+		ret = -EADDRNOTAVAIL;
+		goto out_spi_free;
+	}
+
+	if (np)
+		ssp->devid = (enum mxs_ssp_id) of_id->data;
+	else
+		ssp->devid = pdev->id_entry->driver_data;
+
+	host->transfer_one_message = mxs_spi_transfer_one;
+	host->setup = mxs_spi_setup;
+	host->cleanup = mxs_spi_cleanup;
+	host->mode_bits = SPI_CPOL | SPI_CPHA;
+	host->num_chipselect = 3;
+	host->dev.of_node = np;
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl)) {
+		ret = PTR_ERR(pinctrl);
+		goto out_spi_free;
+	}
+
+	ssp->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ssp->clk)) {
+		ret = PTR_ERR(ssp->clk);
+		goto out_spi_free;
+	}
+
+	clk_prepare_enable(ssp->clk);
+	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
+
+	stmp_reset_block(ssp->base);
+
+	platform_set_drvdata(pdev, host);
+
+	ret = spi_register_master(host);
+	if (ret) {
+		dev_err(&pdev->dev, "Cannot register SPI master, %d\n", ret);
+		goto out_clk_put;
+	}
+
+	return 0;
+
+out_clk_put:
+	clk_disable_unprepare(ssp->clk);
+	clk_put(ssp->clk);
+out_spi_free:
+	spi_master_put(host);
+	return ret;
+}
+
+static int __devexit mxs_spi_remove(struct platform_device *pdev)
+{
+	struct spi_master *host;
+	struct mxs_ssp *ssp;
+
+	host = platform_get_drvdata(pdev);
+	if (host)
+		return 0;
+
+	ssp = spi_master_get_devdata(host);
+
+	spi_unregister_master(host);
+
+	platform_set_drvdata(pdev, NULL);
+
+	clk_disable_unprepare(ssp->clk);
+	clk_put(ssp->clk);
+
+	spi_master_put(host);
+
+	return 0;
+}
+
+static struct platform_driver mxs_spi_driver = {
+	.probe	= mxs_spi_probe,
+	.remove	= __devexit_p(mxs_spi_remove),
+	.driver	= {
+		.name	= "mxs-spi",
+		.owner	= THIS_MODULE,
+		.of_match_table = mxs_spi_dt_ids,
+	},
+};
+
+module_platform_driver(mxs_spi_driver);
+
+MODULE_AUTHOR("Dmitry Pervushin <dimka@embeddedalley.com>");
+MODULE_DESCRIPTION("MXS SPI master driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:mxs-spi");
-- 
1.7.10

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Fabio Estevam]

Hi Marek,

On Sat, Jun 23, 2012 at 3:43 PM, Marek Vasut <marex@denx.de> wrote:

> + ? ? ? ssp->clk = clk_get(&pdev->dev, NULL);
> + ? ? ? if (IS_ERR(ssp->clk)) {
> + ? ? ? ? ? ? ? ret = PTR_ERR(ssp->clk);
> + ? ? ? ? ? ? ? goto out_spi_free;
> + ? ? ? }

You could use devm_clk_get here instead,


> +
> + ? ? ? clk_prepare_enable(ssp->clk);
> + ? ? ? ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
> +
> + ? ? ? stmp_reset_block(ssp->base);
> +
> + ? ? ? platform_set_drvdata(pdev, host);
> +
> + ? ? ? ret = spi_register_master(host);
> + ? ? ? if (ret) {
> + ? ? ? ? ? ? ? dev_err(&pdev->dev, "Cannot register SPI master, %d\n", ret);
> + ? ? ? ? ? ? ? goto out_clk_put;
> + ? ? ? }
> +
> + ? ? ? return 0;
> +
> +out_clk_put:
> + ? ? ? clk_disable_unprepare(ssp->clk);
> + ? ? ? clk_put(ssp->clk);

,and then you would not need this clk_put here.

Driver looks good. Thanks for working on it.

Regards,

Fabio Estevam

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Marek Vasut]

Dear Fabio Estevam,

> Hi Marek,
> 
> On Sat, Jun 23, 2012 at 3:43 PM, Marek Vasut <marex@denx.de> wrote:
> > +       ssp->clk = clk_get(&pdev->dev, NULL);
> > +       if (IS_ERR(ssp->clk)) {
> > +               ret = PTR_ERR(ssp->clk);
> > +               goto out_spi_free;
> > +       }
> 
> You could use devm_clk_get here instead,
> 
> > +
> > +       clk_prepare_enable(ssp->clk);
> > +       ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
> > +
> > +       stmp_reset_block(ssp->base);
> > +
> > +       platform_set_drvdata(pdev, host);
> > +
> > +       ret = spi_register_master(host);
> > +       if (ret) {
> > +               dev_err(&pdev->dev, "Cannot register SPI master, %d\n",
> > ret); +               goto out_clk_put;
> > +       }
> > +
> > +       return 0;
> > +
> > +out_clk_put:
> > +       clk_disable_unprepare(ssp->clk);
> > +       clk_put(ssp->clk);
> 
> ,and then you would not need this clk_put here.

Oh, good point! I'll wait a little bit more until someone else reviews it and 
then I'll resubmit new version.

btw. I'm already working on a combined PIO/DMA-capable version -- seems like 
using PIO for small transfers and DMA for large transfers is good strategy that 
gives me about 200kbps boost ;-)

> Driver looks good. Thanks for working on it.
> 
> Regards,
> 
> Fabio Estevam

Best regards,
Marek Vasut

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Shawn Guo]

On Sat, Jun 23, 2012 at 08:43:53PM +0200, Marek Vasut wrote:
> This is slightly reworked version of the SPI driver.
> Support for DT has been added and it's been converted
> to queued API.
> 
> Based on previous attempt by:
> Fabio Estevam <fabio.estevam@freescale.com>
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Chris Ball <cjb@laptop.org>
> Cc: Detlev Zundel <dzu@denx.de>
> CC: Dong Aisheng <b29396@freescale.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
> Cc: Rob Herring <rob.herring@calxeda.com>
> CC: Shawn Guo <shawn.guo@linaro.org>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
>  drivers/spi/Kconfig   |    6 +
>  drivers/spi/Makefile  |    1 +
>  drivers/spi/spi-mxs.c |  407 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 414 insertions(+)
>  create mode 100644 drivers/spi/spi-mxs.c
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index cd2fe35..85ca6a7 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -355,6 +355,12 @@ config SPI_STMP3XXX
>  	help
>  	  SPI driver for Freescale STMP37xx/378x SoC SSP interface
>  
> +config SPI_MXS
> +	tristate "Freescale MXS SPI controller"
> +	depends on ARCH_MXS
> +	help
> +	  SPI driver for Freescale MXS devices.
> +
>  config SPI_TEGRA
>  	tristate "Nvidia Tegra SPI controller"
>  	depends on ARCH_TEGRA && TEGRA_SYSTEM_DMA
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 9d75d21..a684d1e 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -35,6 +35,7 @@ obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
>  obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
>  obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
>  obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
> +obj-$(CONFIG_SPI_MXS)			+= spi-mxs.o
>  obj-$(CONFIG_SPI_NUC900)		+= spi-nuc900.o
>  obj-$(CONFIG_SPI_OC_TINY)		+= spi-oc-tiny.o
>  obj-$(CONFIG_SPI_OMAP_UWIRE)		+= spi-omap-uwire.o
> diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> new file mode 100644
> index 0000000..81a2643
> --- /dev/null
> +++ b/drivers/spi/spi-mxs.c
> @@ -0,0 +1,407 @@
> +/*
> + * Freescale MXS SPI master driver
> + *
> + * Copyright 2012 DENX Software Engineering, GmbH.
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
> + *
> + * Rework and transition to new API by:
> + * Marek Vasut <marex@denx.de>
> + *
> + * Based on previous attempt by:
> + * Fabio Estevam <fabio.estevam@freescale.com>
> + *
> + * Based on code from U-Boot bootloader by:
> + * Marek Vasut <marex@denx.de>
> + *
> + * Based on spi-stmp.c, which is:
> + * Author: Dmitry Pervushin <dimka@embeddedalley.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.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/ioport.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/highmem.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/completion.h>
> +#include <linux/gpio.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/module.h>
> +#include <linux/fsl/mxs-dma.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/stmp_device.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mxs-spi.h>

All these headers are needed? dma-mapping.h, dmaengine.h, highmem.h,
mxs-dma.h etc?  Or you will need them later?

> +
> +#define SSP_TIMEOUT		200	/* 200 ms */
> +
> +static int mxs_spi_setup_transfer(struct spi_device *spi,
> +				struct spi_transfer *t)
> +{
> +	struct mxs_ssp *ssp = spi_master_get_devdata(spi->master);
> +	uint8_t bits_per_word;
> +	uint32_t hz = 0;
> +
> +	bits_per_word = spi->bits_per_word;
> +	if (t && t->bits_per_word)
> +		bits_per_word = t->bits_per_word;
> +
> +	if (bits_per_word != 8) {
> +		dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
> +					__func__, bits_per_word);
> +		return -EINVAL;
> +	}
> +
> +	if (spi->max_speed_hz)
> +		hz = spi->max_speed_hz;
> +	if (t && t->speed_hz)
> +		hz = t->speed_hz;
> +	if (hz == 0) {
> +		dev_err(&spi->dev, "Cannot continue with zero clock\n");
> +		return -EINVAL;
> +	}
> +
> +	mxs_ssp_set_clk_rate(ssp, hz);
> +
> +	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
> +		     BF_SSP_CTRL1_WORD_LENGTH
> +		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
> +		     ((spi->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
> +		     ((spi->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
> +		     ssp->base + HW_SSP_CTRL1(ssp));
> +
> +	writel(0x0, ssp->base + HW_SSP_CMD0 + STMP_OFFSET_REG_SET);
> +
> +	return 0;
> +}
> +
> +static void mxs_spi_cleanup(struct spi_device *spi)
> +{
> +	return;
> +}
> +
> +/* the spi->mode bits understood by this driver: */

Incomplete comment?  Remove it or make it complete?

> +static int mxs_spi_setup(struct spi_device *spi)
> +{
> +	int err = 0;
> +
> +	if (!spi->bits_per_word)
> +		spi->bits_per_word = 8;
> +
> +	if (spi->mode & ~(SPI_CPOL | SPI_CPHA))
> +		return -EINVAL;
> +
> +	err = mxs_spi_setup_transfer(spi, NULL);
> +	if (err) {
> +		dev_err(&spi->dev,
> +			"Failed to setup transfer, error = %d\n", err);
> +	}

Unnecessary braces.

> +
> +	return err;
> +}
> +
> +static void mxs_spi_set_cs(struct mxs_ssp *ssp, unsigned cs)
> +{
> +	const uint32_t mask =
> +		BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
> +	uint32_t select = 0;
> +
> +	writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +
> +	if (cs & 1)
> +		select |= BM_SSP_CTRL0_WAIT_FOR_CMD;
> +	if (cs & 2)
> +		select |= BM_SSP_CTRL0_WAIT_FOR_IRQ;
> +
> +	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +}
> +
> +static inline void mxs_spi_enable(struct mxs_ssp *ssp)
> +{
> +	writel(BM_SSP_CTRL0_LOCK_CS,
> +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +	writel(BM_SSP_CTRL0_IGNORE_CRC,
> +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +}
> +
> +static inline void mxs_spi_disable(struct mxs_ssp *ssp)
> +{
> +	writel(BM_SSP_CTRL0_LOCK_CS,
> +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +	writel(BM_SSP_CTRL0_IGNORE_CRC,
> +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +}
> +
> +static int mxs_ssp_wait_set(struct mxs_ssp *ssp, int offset, int mask)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
> +
> +	while (!(readl_relaxed(ssp->base + offset) & mask)) {
> +		udelay(1);
> +		if (time_after(jiffies, timeout))
> +			return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
> +static int mxs_ssp_wait_clr(struct mxs_ssp *ssp, int offset, int mask)
> +{
> +	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
> +
> +	while ((readl_relaxed(ssp->base + offset) & mask)) {
> +		udelay(1);
> +		if (time_after(jiffies, timeout))
> +			return -ETIMEDOUT;
> +	}
> +	return 0;
> +}
> +
Merge these two functions into mxs_ssp_wait with one more argument?

> +static int mxs_spi_txrx_pio(struct mxs_ssp *ssp, int cs,
> +			    unsigned char *buf, int len,
> +			    int *first, int *last, int write)
> +{
> +	if (*first) {
> +		mxs_spi_enable(ssp);
> +		*first = 0;
> +	}
> +
> +	mxs_spi_set_cs(ssp, cs);
> +
> +	while (len--) {
> +		if (*last && len == 0) {
> +			mxs_spi_disable(ssp);
> +			*last = 0;
> +		}
> +
> +		if (ssp->devid == IMX23_SSP) {
> +			writel(BM_SSP_CTRL0_XFER_COUNT,
> +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +			writel(1,
> +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +		} else {
> +			writel(1, ssp->base + HW_SSP_XFER_SIZE);
> +		}
> +
> +		if (write)
> +			writel(BM_SSP_CTRL0_READ,
> +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> +		else
> +			writel(BM_SSP_CTRL0_READ,
> +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +
> +		writel(BM_SSP_CTRL0_RUN,
> +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +
> +		if (mxs_ssp_wait_set(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
> +			return -ETIMEDOUT;
> +
> +		if (write)
> +			writel(*buf, ssp->base + HW_SSP_DATA);
> +
> +		writel(BM_SSP_CTRL0_DATA_XFER,
> +			     ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> +
> +		if (!write) {
> +			if (mxs_ssp_wait_clr(ssp, HW_SSP_STATUS(ssp),
> +						BM_SSP_STATUS_FIFO_EMPTY))
> +				return -ETIMEDOUT;
> +
> +			*buf = (readl(ssp->base + HW_SSP_DATA) & 0xff);
> +		}
> +
> +		if (mxs_ssp_wait_clr(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
> +			return -ETIMEDOUT;
> +
> +		buf++;
> +	}
> +
> +	if (len <= 0)
> +		return 0;
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int mxs_spi_transfer_one(struct spi_master *spi, struct spi_message *m)
> +{
> +	struct mxs_ssp *ssp = spi_master_get_devdata(spi);
> +	int first, last;
> +	struct spi_transfer *t, *tmp_t;
> +	int status = 0;
> +	int cs;
> +
> +	first = last = 0;
> +
> +	cs = m->spi->chip_select;
> +
> +	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
> +
> +		mxs_spi_setup_transfer(m->spi, t);
> +
> +		if (&t->transfer_list == m->transfers.next)
> +			first = !0;

Why not simply "first = 1;"?

> +		if (&t->transfer_list == m->transfers.prev)
> +			last = !0;

Ditto

> +		if (t->rx_buf && t->tx_buf) {
> +			pr_debug("%s: cannot send and receive simultaneously\n",
> +				 __func__);

dev_dbg?  Then __func__ is not important to be there.

> +			return -EINVAL;
> +		}
> +
> +		if (t->tx_buf)
> +			status = mxs_spi_txrx_pio(ssp, cs, (void *)t->tx_buf,

Is the cast really needed?  The t->rx_buf below seems not having it.

> +					     t->len, &first, &last, 1);
> +		if (t->rx_buf)
> +			status = mxs_spi_txrx_pio(ssp, cs, t->rx_buf,
> +					     t->len, &first, &last, 0);
> +		m->actual_length += t->len;
> +		if (status)
> +			break;
> +
> +		first = last = 0;
> +	}
> +
> +	m->status = 0;
> +	spi_finalize_current_message(spi);
> +
> +	return status;
> +}
> +
> +static const struct of_device_id mxs_spi_dt_ids[] = {
> +	{ .compatible = "fsl,imx23-spi", .data = (void *) IMX23_SSP, },
> +	{ .compatible = "fsl,imx28-spi", .data = (void *) IMX28_SSP, },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mxs_spi_dt_ids);
> +
> +static int __devinit mxs_spi_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *of_id =
> +			of_match_device(mxs_spi_dt_ids, &pdev->dev);
> +	struct device_node *np = pdev->dev.of_node;
> +	struct spi_master *host;
> +	struct mxs_ssp *ssp;
> +	struct resource *iores;
> +	struct pinctrl *pinctrl;
> +	int ret = 0;
> +
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!iores)
> +		return -EINVAL;

Moving the call right before devm_request_and_ioremap seems reasonable?
Also the error check could be saved, since devm_request_and_ioremap
will take care of it.

> +
> +	host = spi_alloc_master(&pdev->dev, sizeof(struct mxs_ssp));

sizeof(*ssp)

> +	if (!host)
> +		return -ENOMEM;
> +
> +	ssp = spi_master_get_devdata(host);
> +	ssp->dev = &pdev->dev;
> +	ssp->base = devm_request_and_ioremap(&pdev->dev, iores);
> +	if (!ssp->base) {
> +		ret = -EADDRNOTAVAIL;
> +		goto out_spi_free;
> +	}
> +
> +	if (np)
> +		ssp->devid = (enum mxs_ssp_id) of_id->data;
> +	else
> +		ssp->devid = pdev->id_entry->driver_data;
> +
> +	host->transfer_one_message = mxs_spi_transfer_one;
> +	host->setup = mxs_spi_setup;
> +	host->cleanup = mxs_spi_cleanup;
> +	host->mode_bits = SPI_CPOL | SPI_CPHA;
> +	host->num_chipselect = 3;
> +	host->dev.of_node = np;
> +
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	if (IS_ERR(pinctrl)) {
> +		ret = PTR_ERR(pinctrl);
> +		goto out_spi_free;
> +	}
> +
> +	ssp->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ssp->clk)) {
> +		ret = PTR_ERR(ssp->clk);
> +		goto out_spi_free;
> +	}
> +
> +	clk_prepare_enable(ssp->clk);
> +	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
> +
> +	stmp_reset_block(ssp->base);
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	ret = spi_register_master(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot register SPI master, %d\n", ret);
> +		goto out_clk_put;
> +	}
> +
> +	return 0;
> +
> +out_clk_put:
> +	clk_disable_unprepare(ssp->clk);
> +	clk_put(ssp->clk);
> +out_spi_free:
> +	spi_master_put(host);

spi_master_put will not free the memory, so we have to call kfree to
do it.

> +	return ret;
> +}
> +
> +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *host;
> +	struct mxs_ssp *ssp;
> +
> +	host = platform_get_drvdata(pdev);
> +	if (host)
> +		return 0;

Hmm, why the check and return here?

> +
> +	ssp = spi_master_get_devdata(host);
> +
> +	spi_unregister_master(host);
> +
> +	platform_set_drvdata(pdev, NULL);
> +
> +	clk_disable_unprepare(ssp->clk);
> +	clk_put(ssp->clk);
> +
> +	spi_master_put(host);

kfree

> +
> +	return 0;
> +}
> +
> +static struct platform_driver mxs_spi_driver = {
> +	.probe	= mxs_spi_probe,
> +	.remove	= __devexit_p(mxs_spi_remove),
> +	.driver	= {
> +		.name	= "mxs-spi",
> +		.owner	= THIS_MODULE,
> +		.of_match_table = mxs_spi_dt_ids,
> +	},
> +};
> +
> +module_platform_driver(mxs_spi_driver);
> +
> +MODULE_AUTHOR("Dmitry Pervushin <dimka@embeddedalley.com>");
> +MODULE_DESCRIPTION("MXS SPI master driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:mxs-spi");
> -- 
> 1.7.10
> 

-- 
Regards,
Shawn

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Marek Vasut]

Dear Shawn Guo,

> On Sat, Jun 23, 2012 at 08:43:53PM +0200, Marek Vasut wrote:
> > This is slightly reworked version of the SPI driver.
> > Support for DT has been added and it's been converted
> > to queued API.
> > 
> > Based on previous attempt by:
> > Fabio Estevam <fabio.estevam@freescale.com>
> > 
> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Chris Ball <cjb@laptop.org>
> > Cc: Detlev Zundel <dzu@denx.de>
> > CC: Dong Aisheng <b29396@freescale.com>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: Linux ARM kernel <linux-arm-kernel@lists.infradead.org>
> > Cc: Rob Herring <rob.herring@calxeda.com>
> > CC: Shawn Guo <shawn.guo@linaro.org>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Wolfgang Denk <wd@denx.de>
> > ---
> > 
> >  drivers/spi/Kconfig   |    6 +
> >  drivers/spi/Makefile  |    1 +
> >  drivers/spi/spi-mxs.c |  407
> >  +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 414
> >  insertions(+)
> >  create mode 100644 drivers/spi/spi-mxs.c
> > 
> > diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> > index cd2fe35..85ca6a7 100644
> > --- a/drivers/spi/Kconfig
> > +++ b/drivers/spi/Kconfig
> > @@ -355,6 +355,12 @@ config SPI_STMP3XXX
> > 
> >  	help
> >  	
> >  	  SPI driver for Freescale STMP37xx/378x SoC SSP interface
> > 
> > +config SPI_MXS
> > +	tristate "Freescale MXS SPI controller"
> > +	depends on ARCH_MXS
> > +	help
> > +	  SPI driver for Freescale MXS devices.
> > +
> > 
> >  config SPI_TEGRA
> >  
> >  	tristate "Nvidia Tegra SPI controller"
> >  	depends on ARCH_TEGRA && TEGRA_SYSTEM_DMA
> > 
> > diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> > index 9d75d21..a684d1e 100644
> > --- a/drivers/spi/Makefile
> > +++ b/drivers/spi/Makefile
> > @@ -35,6 +35,7 @@ obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
> > 
> >  obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
> >  obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o
> >  obj-$(CONFIG_SPI_MPC52xx)		+= spi-mpc52xx.o
> > 
> > +obj-$(CONFIG_SPI_MXS)			+= spi-mxs.o
> > 
> >  obj-$(CONFIG_SPI_NUC900)		+= spi-nuc900.o
> >  obj-$(CONFIG_SPI_OC_TINY)		+= spi-oc-tiny.o
> >  obj-$(CONFIG_SPI_OMAP_UWIRE)		+= spi-omap-uwire.o
> > 
> > diff --git a/drivers/spi/spi-mxs.c b/drivers/spi/spi-mxs.c
> > new file mode 100644
> > index 0000000..81a2643
> > --- /dev/null
> > +++ b/drivers/spi/spi-mxs.c
> > @@ -0,0 +1,407 @@
> > +/*
> > + * Freescale MXS SPI master driver
> > + *
> > + * Copyright 2012 DENX Software Engineering, GmbH.
> > + * Copyright 2012 Freescale Semiconductor, Inc.
> > + * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
> > + *
> > + * Rework and transition to new API by:
> > + * Marek Vasut <marex@denx.de>
> > + *
> > + * Based on previous attempt by:
> > + * Fabio Estevam <fabio.estevam@freescale.com>
> > + *
> > + * Based on code from U-Boot bootloader by:
> > + * Marek Vasut <marex@denx.de>
> > + *
> > + * Based on spi-stmp.c, which is:
> > + * Author: Dmitry Pervushin <dimka@embeddedalley.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.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/ioport.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/delay.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/highmem.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/completion.h>
> > +#include <linux/gpio.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/fsl/mxs-dma.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/stmp_device.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/spi/mxs-spi.h>
> 
> All these headers are needed? dma-mapping.h, dmaengine.h, highmem.h,
> mxs-dma.h etc?  Or you will need them later?

Not later, I already do need them. It's just that I didn't submit the last patch 
yet, since I developed it only after these were submitted.

> > +
> > +#define SSP_TIMEOUT		200	/* 200 ms */
> > +
> > +static int mxs_spi_setup_transfer(struct spi_device *spi,
> > +				struct spi_transfer *t)
> > +{
> > +	struct mxs_ssp *ssp = spi_master_get_devdata(spi->master);
> > +	uint8_t bits_per_word;
> > +	uint32_t hz = 0;
> > +
> > +	bits_per_word = spi->bits_per_word;
> > +	if (t && t->bits_per_word)
> > +		bits_per_word = t->bits_per_word;
> > +
> > +	if (bits_per_word != 8) {
> > +		dev_err(&spi->dev, "%s, unsupported bits_per_word=%d\n",
> > +					__func__, bits_per_word);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (spi->max_speed_hz)
> > +		hz = spi->max_speed_hz;
> > +	if (t && t->speed_hz)
> > +		hz = t->speed_hz;
> > +	if (hz == 0) {
> > +		dev_err(&spi->dev, "Cannot continue with zero clock\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	mxs_ssp_set_clk_rate(ssp, hz);
> > +
> > +	writel(BF_SSP_CTRL1_SSP_MODE(BV_SSP_CTRL1_SSP_MODE__SPI) |
> > +		     BF_SSP_CTRL1_WORD_LENGTH
> > +		     (BV_SSP_CTRL1_WORD_LENGTH__EIGHT_BITS) |
> > +		     ((spi->mode & SPI_CPOL) ? BM_SSP_CTRL1_POLARITY : 0) |
> > +		     ((spi->mode & SPI_CPHA) ? BM_SSP_CTRL1_PHASE : 0),
> > +		     ssp->base + HW_SSP_CTRL1(ssp));
> > +
> > +	writel(0x0, ssp->base + HW_SSP_CMD0 + STMP_OFFSET_REG_SET);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mxs_spi_cleanup(struct spi_device *spi)
> > +{
> > +	return;
> > +}
> > +
> > +/* the spi->mode bits understood by this driver: */
> 
> Incomplete comment?  Remove it or make it complete?

Good catch.

> > +static int mxs_spi_setup(struct spi_device *spi)
> > +{
> > +	int err = 0;
> > +
> > +	if (!spi->bits_per_word)
> > +		spi->bits_per_word = 8;
> > +
> > +	if (spi->mode & ~(SPI_CPOL | SPI_CPHA))
> > +		return -EINVAL;
> > +
> > +	err = mxs_spi_setup_transfer(spi, NULL);
> > +	if (err) {
> > +		dev_err(&spi->dev,
> > +			"Failed to setup transfer, error = %d\n", err);
> > +	}
> 
> Unnecessary braces.

It's much more readable for multiline code.

> > +
> > +	return err;
> > +}
> > +
> > +static void mxs_spi_set_cs(struct mxs_ssp *ssp, unsigned cs)
> > +{
> > +	const uint32_t mask =
> > +		BM_SSP_CTRL0_WAIT_FOR_CMD | BM_SSP_CTRL0_WAIT_FOR_IRQ;
> > +	uint32_t select = 0;
> > +
> > +	writel(mask, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +
> > +	if (cs & 1)
> > +		select |= BM_SSP_CTRL0_WAIT_FOR_CMD;
> > +	if (cs & 2)
> > +		select |= BM_SSP_CTRL0_WAIT_FOR_IRQ;
> > +
> > +	writel(select, ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +}
> > +
> > +static inline void mxs_spi_enable(struct mxs_ssp *ssp)
> > +{
> > +	writel(BM_SSP_CTRL0_LOCK_CS,
> > +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +	writel(BM_SSP_CTRL0_IGNORE_CRC,
> > +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +}
> > +
> > +static inline void mxs_spi_disable(struct mxs_ssp *ssp)
> > +{
> > +	writel(BM_SSP_CTRL0_LOCK_CS,
> > +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +	writel(BM_SSP_CTRL0_IGNORE_CRC,
> > +		ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +}
> > +
> > +static int mxs_ssp_wait_set(struct mxs_ssp *ssp, int offset, int mask)
> > +{
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
> > +
> > +	while (!(readl_relaxed(ssp->base + offset) & mask)) {
> > +		udelay(1);
> > +		if (time_after(jiffies, timeout))
> > +			return -ETIMEDOUT;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int mxs_ssp_wait_clr(struct mxs_ssp *ssp, int offset, int mask)
> > +{
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(SSP_TIMEOUT);
> > +
> > +	while ((readl_relaxed(ssp->base + offset) & mask)) {
> > +		udelay(1);
> > +		if (time_after(jiffies, timeout))
> > +			return -ETIMEDOUT;
> > +	}
> > +	return 0;
> > +}
> > +
> 
> Merge these two functions into mxs_ssp_wait with one more argument?
> 
> > +static int mxs_spi_txrx_pio(struct mxs_ssp *ssp, int cs,
> > +			    unsigned char *buf, int len,
> > +			    int *first, int *last, int write)
> > +{
> > +	if (*first) {
> > +		mxs_spi_enable(ssp);
> > +		*first = 0;
> > +	}
> > +
> > +	mxs_spi_set_cs(ssp, cs);
> > +
> > +	while (len--) {
> > +		if (*last && len == 0) {
> > +			mxs_spi_disable(ssp);
> > +			*last = 0;
> > +		}
> > +
> > +		if (ssp->devid == IMX23_SSP) {
> > +			writel(BM_SSP_CTRL0_XFER_COUNT,
> > +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +			writel(1,
> > +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +		} else {
> > +			writel(1, ssp->base + HW_SSP_XFER_SIZE);
> > +		}
> > +
> > +		if (write)
> > +			writel(BM_SSP_CTRL0_READ,
> > +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_CLR);
> > +		else
> > +			writel(BM_SSP_CTRL0_READ,
> > +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +
> > +		writel(BM_SSP_CTRL0_RUN,
> > +				ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +
> > +		if (mxs_ssp_wait_set(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
> > +			return -ETIMEDOUT;
> > +
> > +		if (write)
> > +			writel(*buf, ssp->base + HW_SSP_DATA);
> > +
> > +		writel(BM_SSP_CTRL0_DATA_XFER,
> > +			     ssp->base + HW_SSP_CTRL0 + STMP_OFFSET_REG_SET);
> > +
> > +		if (!write) {
> > +			if (mxs_ssp_wait_clr(ssp, HW_SSP_STATUS(ssp),
> > +						BM_SSP_STATUS_FIFO_EMPTY))
> > +				return -ETIMEDOUT;
> > +
> > +			*buf = (readl(ssp->base + HW_SSP_DATA) & 0xff);
> > +		}
> > +
> > +		if (mxs_ssp_wait_clr(ssp, HW_SSP_CTRL0, BM_SSP_CTRL0_RUN))
> > +			return -ETIMEDOUT;
> > +
> > +		buf++;
> > +	}
> > +
> > +	if (len <= 0)
> > +		return 0;
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +static int mxs_spi_transfer_one(struct spi_master *spi, struct
> > spi_message *m) +{
> > +	struct mxs_ssp *ssp = spi_master_get_devdata(spi);
> > +	int first, last;
> > +	struct spi_transfer *t, *tmp_t;
> > +	int status = 0;
> > +	int cs;
> > +
> > +	first = last = 0;
> > +
> > +	cs = m->spi->chip_select;
> > +
> > +	list_for_each_entry_safe(t, tmp_t, &m->transfers, transfer_list) {
> > +
> > +		mxs_spi_setup_transfer(m->spi, t);
> > +
> > +		if (&t->transfer_list == m->transfers.next)
> > +			first = !0;
> 
> Why not simply "first = 1;"?
> 
> > +		if (&t->transfer_list == m->transfers.prev)
> > +			last = !0;
> 
> Ditto
> 
> > +		if (t->rx_buf && t->tx_buf) {
> > +			pr_debug("%s: cannot send and receive simultaneously\n",
> > +				 __func__);
> 
> dev_dbg?  Then __func__ is not important to be there.

Correct.

> > +			return -EINVAL;
> > +		}
> > +
> > +		if (t->tx_buf)
> > +			status = mxs_spi_txrx_pio(ssp, cs, (void *)t->tx_buf,
> 
> Is the cast really needed?  The t->rx_buf below seems not having it.

Yes, it is. Since t->tx_buf is const void *, we need to cast it so the compiler 
won't complain.

> > +					     t->len, &first, &last, 1);
> > +		if (t->rx_buf)
> > +			status = mxs_spi_txrx_pio(ssp, cs, t->rx_buf,
> > +					     t->len, &first, &last, 0);
> > +		m->actual_length += t->len;
> > +		if (status)
> > +			break;
> > +
> > +		first = last = 0;
> > +	}
> > +
> > +	m->status = 0;
> > +	spi_finalize_current_message(spi);
> > +
> > +	return status;
> > +}
> > +
> > +static const struct of_device_id mxs_spi_dt_ids[] = {
> > +	{ .compatible = "fsl,imx23-spi", .data = (void *) IMX23_SSP, },
> > +	{ .compatible = "fsl,imx28-spi", .data = (void *) IMX28_SSP, },
> > +	{ /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mxs_spi_dt_ids);
> > +
> > +static int __devinit mxs_spi_probe(struct platform_device *pdev)
> > +{
> > +	const struct of_device_id *of_id =
> > +			of_match_device(mxs_spi_dt_ids, &pdev->dev);
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct spi_master *host;
> > +	struct mxs_ssp *ssp;
> > +	struct resource *iores;
> > +	struct pinctrl *pinctrl;
> > +	int ret = 0;
> > +
> > +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!iores)
> > +		return -EINVAL;
> 
> Moving the call right before devm_request_and_ioremap seems reasonable?
> Also the error check could be saved, since devm_request_and_ioremap
> will take care of it.

Actually I tried to follow the code path of mxs_mmc here as much as possible, 
hoping we might eventually be able to converge them and make some code shared.

> > +
> > +	host = spi_alloc_master(&pdev->dev, sizeof(struct mxs_ssp));
> 
> sizeof(*ssp)
> 
> > +	if (!host)
> > +		return -ENOMEM;
> > +
> > +	ssp = spi_master_get_devdata(host);
> > +	ssp->dev = &pdev->dev;
> > +	ssp->base = devm_request_and_ioremap(&pdev->dev, iores);
> > +	if (!ssp->base) {
> > +		ret = -EADDRNOTAVAIL;
> > +		goto out_spi_free;
> > +	}
> > +
> > +	if (np)
> > +		ssp->devid = (enum mxs_ssp_id) of_id->data;
> > +	else
> > +		ssp->devid = pdev->id_entry->driver_data;
> > +
> > +	host->transfer_one_message = mxs_spi_transfer_one;
> > +	host->setup = mxs_spi_setup;
> > +	host->cleanup = mxs_spi_cleanup;
> > +	host->mode_bits = SPI_CPOL | SPI_CPHA;
> > +	host->num_chipselect = 3;
> > +	host->dev.of_node = np;
> > +
> > +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> > +	if (IS_ERR(pinctrl)) {
> > +		ret = PTR_ERR(pinctrl);
> > +		goto out_spi_free;
> > +	}
> > +
> > +	ssp->clk = clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(ssp->clk)) {
> > +		ret = PTR_ERR(ssp->clk);
> > +		goto out_spi_free;
> > +	}
> > +
> > +	clk_prepare_enable(ssp->clk);
> > +	ssp->clk_rate = clk_get_rate(ssp->clk) / 1000;
> > +
> > +	stmp_reset_block(ssp->base);
> > +
> > +	platform_set_drvdata(pdev, host);
> > +
> > +	ret = spi_register_master(host);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Cannot register SPI master, %d\n", ret);
> > +		goto out_clk_put;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_clk_put:
> > +	clk_disable_unprepare(ssp->clk);
> > +	clk_put(ssp->clk);
> > +out_spi_free:
> > +	spi_master_put(host);
> 
> spi_master_put will not free the memory, so we have to call kfree to
> do it.
> 
> > +	return ret;
> > +}
> > +
> > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > +{
> > +	struct spi_master *host;
> > +	struct mxs_ssp *ssp;
> > +
> > +	host = platform_get_drvdata(pdev);
> > +	if (host)
> > +		return 0;
> 
> Hmm, why the check and return here?

If it's not set, things went _very_ wrong somewhere. Maybe BUG_ON() might be 
better?

> > +
> > +	ssp = spi_master_get_devdata(host);
> > +
> > +	spi_unregister_master(host);
> > +
> > +	platform_set_drvdata(pdev, NULL);
> > +
> > +	clk_disable_unprepare(ssp->clk);
> > +	clk_put(ssp->clk);
> > +
> > +	spi_master_put(host);
> 
> kfree
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver mxs_spi_driver = {
> > +	.probe	= mxs_spi_probe,
> > +	.remove	= __devexit_p(mxs_spi_remove),
> > +	.driver	= {
> > +		.name	= "mxs-spi",
> > +		.owner	= THIS_MODULE,
> > +		.of_match_table = mxs_spi_dt_ids,
> > +	},
> > +};
> > +
> > +module_platform_driver(mxs_spi_driver);
> > +
> > +MODULE_AUTHOR("Dmitry Pervushin <dimka@embeddedalley.com>");
> > +MODULE_DESCRIPTION("MXS SPI master driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:mxs-spi");

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Shawn Guo]

On Tue, Jun 26, 2012 at 02:22:54PM +0200, Marek Vasut wrote:
> > > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > > +{
> > > +	struct spi_master *host;
> > > +	struct mxs_ssp *ssp;
> > > +
> > > +	host = platform_get_drvdata(pdev);
> > > +	if (host)
> > > +		return 0;
> > 
> > Hmm, why the check and return here?
> 
> If it's not set, things went _very_ wrong somewhere. Maybe BUG_ON() might be 
> better?
> 
So shouldn't it be "if (!host)"?

And I still do not understand how that could happen.  I think
mxs_spi_remove will only be called in case that mxs_spi_probe
succeeds, which means platform_set_drvdata must have been called
already?

-- 
Regards,
Shawn

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Marek Vasut]

Dear Shawn Guo,

> On Tue, Jun 26, 2012 at 02:22:54PM +0200, Marek Vasut wrote:
> > > > +static int __devexit mxs_spi_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct spi_master *host;
> > > > +	struct mxs_ssp *ssp;
> > > > +
> > > > +	host = platform_get_drvdata(pdev);
> > > > +	if (host)
> > > > +		return 0;
> > > 
> > > Hmm, why the check and return here?
> > 
> > If it's not set, things went _very_ wrong somewhere. Maybe BUG_ON() might
> > be better?
> 
> So shouldn't it be "if (!host)"?
> 
> And I still do not understand how that could happen.  I think
> mxs_spi_remove will only be called in case that mxs_spi_probe
> succeeds, which means platform_set_drvdata must have been called
> already?

Ok, we can remove that altogether then.

Best regards,
Marek Vasut

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Marek Vasut]

Dear Alain-Serge Nagni,

> Hi Fabio and Marek,
>  So I did rebuild the Kernel and UBoot to support device tree.  I did
> reboot the system and this time I?m not having any kernel panic. Which is
> a good signJ. After investigation I notice that the  SSP2 node status is
> marked ?disabled? (in the file imx28.dtsi).  So there is no way based on
> the patch that was publish to have the imx-spi driver loaded in the kernel
> at boot time. 
> Mareck,
>  I guess that  you have a piece of code that you forgot to publish? If not
> how can I have your SPI drive to run? 
>  Right now I guess that the next step would be to define the SPI node in
> the im28-evk.dts file.  I will open a thread to see if anybody as a
> solution for that. What is your take on that?
>  
> Thanks guys,
> Alain-Serge         

[...]

Fabio already answered your question. Lemme teach you a few more rules:
1) Do not top post in the email conversation ;-)
2) About DT. You're supposed to include imx28.dtsi in your imx28-yourboard.dts 
file. Then everything that's already defined in the imx28.dtsi will be present 
in your imx28-yourboard.dts . But the key thing is, if you define something 
again, it will override the stuff already defined in the imx28.dtsi file. But 
you don't have to redefine everything, just redefine what you need changed.

Best regards,
Marek Vasut

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Alain-Serge Nagni]

Hi Marek,

?1) Thanks for the advice, I will remember to not top post :-)



?2) I spent lot of time testing your new additions to the mxs SPI driver and it works fine. Great job.



Regards,

Alain-Serge


--- On Thu, 6/28/12, Marek Vasut <marex@denx.de> wrote:

From: Marek Vasut <marex@denx.de>
Subject: Re: [PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28
To: linux-arm-kernel at lists.infradead.org
Cc: "Alain-Serge Nagni" <alainsergenagni@yahoo.com>, "Fabio Estevam" <festevam@gmail.com>, "Shawn Guo" <shawn.guo@linaro.org>
Date: Thursday, June 28, 2012, 6:09 PM

Dear Alain-Serge Nagni,

> Hi Fabio and Marek,
>? So I did rebuild the Kernel and UBoot to support device tree.? I did
> reboot the system and this time I?m not having any kernel panic. Which is
> a good signJ. After investigation I notice that the? SSP2 node status is
> marked ?disabled? (in the file imx28.dtsi).? So there is no way based on
> the patch that was publish to have the imx-spi driver loaded in the kernel
> at boot time. 
> Mareck,
>? I guess that? you have a piece of code that you forgot to publish? If not
> how can I have your SPI drive to run? 
>? Right now I guess that the next step would be to define the SPI node in
> the im28-evk.dts file.? I will open a thread to see if anybody as a
> solution for that. What is your take on that?
>? 
> Thanks guys,
> Alain-Serge? ? ? ???

[...]

Fabio already answered your question. Lemme teach you a few more rules:
1) Do not top post in the email conversation ;-)
2) About DT. You're supposed to include imx28.dtsi in your imx28-yourboard.dts 
file. Then everything that's already defined in the imx28.dtsi will be present 
in your imx28-yourboard.dts . But the key thing is, if you define something 
again, it will override the stuff already defined in the imx28.dtsi file. But 
you don't have to redefine everything, just redefine what you need changed.

Best regards,
Marek Vasut
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120707/f1a8f008/attachment-0001.html>

[PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28

[Marek Vasut]

Dear Alain-Serge Nagni,

> Hi Marek,
> 
>  1) Thanks for the advice, I will remember to not top post :-)

You did it again ;-)

>  2) I spent lot of time testing your new additions to the mxs SPI driver
> and it works fine. Great job.

On what platform did you test it? And with what SPI peripherals? Can I possibly 
get your proper Tested-by: line?

> Regards,
> 
> Alain-Serge
> 
> 
> --- On Thu, 6/28/12, Marek Vasut <marex@denx.de> wrote:
> 
> From: Marek Vasut <marex@denx.de>
> Subject: Re: [PATCH 7/7] ARM: mxs: Add SPI driver for mx233/mx28
> To: linux-arm-kernel at lists.infradead.org
> Cc: "Alain-Serge Nagni" <alainsergenagni@yahoo.com>, "Fabio Estevam"
> <festevam@gmail.com>, "Shawn Guo" <shawn.guo@linaro.org> Date: Thursday,
> June 28, 2012, 6:09 PM
> 
> Dear Alain-Serge Nagni,
> 
> > Hi Fabio and Marek,
> >
> >  So I did rebuild the Kernel and UBoot to support device tree.  I did
> >
> > reboot the system and this time I?m not having any kernel panic. Which is
> > a good signJ. After investigation I notice that the  SSP2 node status is
> > marked ?disabled? (in the file imx28.dtsi).  So there is no way based on
> > the patch that was publish to have the imx-spi driver loaded in the
> > kernel at boot time.
> > Mareck,
> >
> >  I guess that  you have a piece of code that you forgot to publish? If
> >not
> >
> > how can I have your SPI drive to run?
> >
> >  Right now I guess that the next step would be to define the SPI node in
> >
> > the im28-evk.dts file.  I will open a thread to see if anybody as a
> > solution for that. What is your take on that?
> >
> > 
> >
> > Thanks guys,
> > Alain-Serge         
> 
> [...]
> 
> Fabio already answered your question. Lemme teach you a few more rules:
> 1) Do not top post in the email conversation ;-)
> 2) About DT. You're supposed to include imx28.dtsi in your
> imx28-yourboard.dts file. Then everything that's already defined in the
> imx28.dtsi will be present in your imx28-yourboard.dts . But the key thing
> is, if you define something again, it will override the stuff already
> defined in the imx28.dtsi file. But you don't have to redefine everything,
> just redefine what you need changed.
> 
> Best regards,
> Marek Vasut

Best regards,
Marek Vasut