linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
@ 2009-06-18  2:55 Grant Likely
  2009-06-18  6:58 ` [spi-devel-general] " Wolfram Sang
  2009-10-21 13:17 ` Wolfram Sang
  0 siblings, 2 replies; 8+ messages in thread
From: Grant Likely @ 2009-06-18  2:55 UTC (permalink / raw)
  To: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel

From: Grant Likely <grant.likely@secretlab.ca>

Adds support for the dedicated SPI device on the Freescale MPC5200(b)
SoC.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

Hi David,

It's been a while since I've posted this, but I believe I've addressed
all the outstanding comments against v3, and people are asking me for it.
The pre-message stuff is all gone and the error handling should be
better now.

BTW, the premessage stuff was to handle a platform I had where some of
the CS lines were controlled with a separate SPI transaction to the
same SPI controller.  It almost looks like an SPI bridge.  It requires
more thought before I try again.

Being a new driver, it would be nice to get it into 2.6.31, but
definitely not critical.

g.

 drivers/spi/Kconfig             |    8 +
 drivers/spi/Makefile            |    1 
 drivers/spi/mpc52xx_spi.c       |  520 +++++++++++++++++++++++++++++++++++++++
 include/linux/spi/mpc52xx_spi.h |   10 +
 4 files changed, 539 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/mpc52xx_spi.c
 create mode 100644 include/linux/spi/mpc52xx_spi.h


diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 83a185d..1994bcd 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -132,6 +132,14 @@ config SPI_LM70_LLP
 	  which interfaces to an LM70 temperature sensor using
 	  a parallel port.
 
+config SPI_MPC52xx
+	tristate "Freescale MPC52xx SPI (non-PSC) controller support"
+	depends on PPC_MPC52xx && SPI
+	select SPI_MASTER_OF
+	help
+	  This drivers supports the MPC52xx SPI controller in master SPI
+	  mode.
+
 config SPI_MPC52xx_PSC
 	tristate "Freescale MPC52xx PSC SPI controller"
 	depends on PPC_MPC52xx && EXPERIMENTAL
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 5d04519..8de32c7 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE)		+= omap_uwire.o
 obj-$(CONFIG_SPI_OMAP24XX)		+= omap2_mcspi.o
 obj-$(CONFIG_SPI_ORION)			+= orion_spi.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
+obj-$(CONFIG_SPI_MPC52xx)		+= mpc52xx_spi.o
 obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
 obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
 obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
new file mode 100644
index 0000000..ef8379b
--- /dev/null
+++ b/drivers/spi/mpc52xx_spi.c
@@ -0,0 +1,520 @@
+/*
+ * MPC52xx SPI bus driver.
+ *
+ * Copyright (C) 2008 Secret Lab Technologies Ltd.
+ *
+ * This file is released under the GPLv2
+ *
+ * This is the driver for the MPC5200's dedicated SPI controller.
+ *
+ * Note: this driver does not support the MPC5200 PSC in SPI mode.  For
+ * that driver see drivers/spi/mpc52xx_psc_spi.c
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/errno.h>
+#include <linux/of_platform.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/mpc52xx_spi.h>
+#include <linux/of_spi.h>
+#include <linux/io.h>
+#include <asm/time.h>
+#include <asm/mpc52xx.h>
+
+MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
+MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
+MODULE_LICENSE("GPL");
+
+/* Register offsets */
+#define SPI_CTRL1	0x00
+#define SPI_CTRL1_SPIE		(1 << 7)
+#define SPI_CTRL1_SPE		(1 << 6)
+#define SPI_CTRL1_MSTR		(1 << 4)
+#define SPI_CTRL1_CPOL		(1 << 3)
+#define SPI_CTRL1_CPHA		(1 << 2)
+#define SPI_CTRL1_SSOE		(1 << 1)
+#define SPI_CTRL1_LSBFE		(1 << 0)
+
+#define SPI_CTRL2	0x01
+#define SPI_BRR		0x04
+
+#define SPI_STATUS	0x05
+#define SPI_STATUS_SPIF		(1 << 7)
+#define SPI_STATUS_WCOL		(1 << 6)
+#define SPI_STATUS_MODF		(1 << 4)
+
+#define SPI_DATA	0x09
+#define SPI_PORTDATA	0x0d
+#define SPI_DATADIR	0x10
+
+/* FSM state return values */
+#define FSM_STOP	0	/* Nothing more for the state machine to */
+				/* do.  If something interesting happens */
+				/* then and IRQ will be received */
+#define FSM_POLL	1	/* need to poll for completion, an IRQ is */
+				/* not expected */
+#define FSM_CONTINUE	2	/* Keep iterating the state machine */
+
+/* Driver internal data */
+struct mpc52xx_spi {
+	struct spi_master *master;
+	u32 sysclk;
+	void __iomem *regs;
+	int irq0;	/* MODF irq */
+	int irq1;	/* SPIF irq */
+	int ipb_freq;
+
+	/* Statistics */
+	int msg_count;
+	int wcol_count;
+	int wcol_ticks;
+	u32 wcol_tx_timestamp;
+	int modf_count;
+	int byte_count;
+
+	struct list_head queue;		/* queue of pending messages */
+	spinlock_t lock;
+	struct work_struct work;
+
+
+	/* Details of current transfer (length, and buffer pointers) */
+	struct spi_message *message;	/* current message */
+	struct spi_transfer *transfer;	/* current transfer */
+	int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
+	int len;
+	int timestamp;
+	u8 *rx_buf;
+	const u8 *tx_buf;
+	int cs_change;
+};
+
+/*
+ * CS control function
+ */
+static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
+{
+	out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);
+}
+
+/*
+ * Start a new transfer.  This is called both by the idle state
+ * for the first transfer in a message, and by the wait state when the
+ * previous transfer in a message is complete.
+ */
+static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
+{
+	ms->rx_buf = ms->transfer->rx_buf;
+	ms->tx_buf = ms->transfer->tx_buf;
+	ms->len = ms->transfer->len;
+
+	/* Activate the chip select */
+	if (ms->cs_change)
+		mpc52xx_spi_chipsel(ms, 1);
+	ms->cs_change = ms->transfer->cs_change;
+
+	/* Write out the first byte */
+	ms->wcol_tx_timestamp = get_tbl();
+	if (ms->tx_buf)
+		out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
+	else
+		out_8(ms->regs + SPI_DATA, 0);
+}
+
+/* Forward declaration of state handlers */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+					 u8 status, u8 data);
+static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
+				     u8 status, u8 data);
+
+/*
+ * IDLE state
+ *
+ * No transfers are in progress; if another transfer is pending then retrieve
+ * it and kick it off.  Otherwise, stop processing the state machine
+ */
+static int
+mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+	struct spi_device *spi;
+	int spr, sppr;
+	u8 ctrl1;
+
+	if (status && (irq != NO_IRQ))
+		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+			status);
+
+	/* Check if there is another transfer waiting. */
+	if (list_empty(&ms->queue))
+		return FSM_STOP;
+
+	/* get the head of the queue */
+	ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
+	list_del_init(&ms->message->queue);
+
+	/* Setup the controller parameters */
+	ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
+	spi = ms->message->spi;
+	if (spi->mode & SPI_CPHA)
+		ctrl1 |= SPI_CTRL1_CPHA;
+	if (spi->mode & SPI_CPOL)
+		ctrl1 |= SPI_CTRL1_CPOL;
+	if (spi->mode & SPI_LSB_FIRST)
+		ctrl1 |= SPI_CTRL1_LSBFE;
+	out_8(ms->regs + SPI_CTRL1, ctrl1);
+
+	/* Setup the controller speed */
+	/* minimum divider is '2'.  Also, add '1' to force rounding the
+	 * divider up. */
+	sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
+	spr = 0;
+	if (sppr < 1)
+		sppr = 1;
+	while (((sppr - 1) & ~0x7) != 0) {
+		sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
+		spr++;
+	}
+	sppr--;		/* sppr quantity in register is offset by 1 */
+	if (spr > 7) {
+		/* Don't overrun limits of SPI baudrate register */
+		spr = 7;
+		sppr = 7;
+	}
+	out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
+
+	ms->cs_change = 1;
+	ms->transfer = container_of(ms->message->transfers.next,
+				    struct spi_transfer, transfer_list);
+
+	mpc52xx_spi_start_transfer(ms);
+	ms->state = mpc52xx_spi_fsmstate_transfer;
+
+	return FSM_CONTINUE;
+}
+
+/*
+ * TRANSFER state
+ *
+ * In the middle of a transfer.  If the SPI core has completed processing
+ * a byte, then read out the received data and write out the next byte
+ * (unless this transfer is finished; in which case go on to the wait
+ * state)
+ */
+static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
+					 u8 status, u8 data)
+{
+	if (!status)
+		return ms->irq0 ? FSM_STOP : FSM_POLL;
+
+	if (status & SPI_STATUS_WCOL) {
+		/* The SPI controller is stoopid.  At slower speeds, it may
+		 * raise the SPIF flag before the state machine is actually
+		 * finished, which causes a collision (internal to the state
+		 * machine only).  The manual recommends inserting a delay
+		 * between receiving the interrupt and sending the next byte,
+		 * but it can also be worked around simply by retrying the
+		 * transfer which is what we do here. */
+		ms->wcol_count++;
+		ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
+		ms->wcol_tx_timestamp = get_tbl();
+		data = 0;
+		if (ms->tx_buf)
+			data = *(ms->tx_buf-1);
+		out_8(ms->regs + SPI_DATA, data); /* try again */
+		return FSM_CONTINUE;
+	} else if (status & SPI_STATUS_MODF) {
+		ms->modf_count++;
+		dev_err(&ms->master->dev, "mode fault\n");
+		mpc52xx_spi_chipsel(ms, 0);
+		ms->message->status = -EIO;
+		ms->message->complete(ms->message->context);
+		ms->state = mpc52xx_spi_fsmstate_idle;
+		return FSM_CONTINUE;
+	}
+
+	/* Read data out of the spi device */
+	ms->byte_count++;
+	if (ms->rx_buf)
+		*ms->rx_buf++ = data;
+
+	/* Is the transfer complete? */
+	ms->len--;
+	if (ms->len == 0) {
+		ms->timestamp = get_tbl();
+		ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
+		ms->state = mpc52xx_spi_fsmstate_wait;
+		return FSM_CONTINUE;
+	}
+
+	/* Write out the next byte */
+	ms->wcol_tx_timestamp = get_tbl();
+	if (ms->tx_buf)
+		out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
+	else
+		out_8(ms->regs + SPI_DATA, 0);
+
+	return FSM_CONTINUE;
+}
+
+/*
+ * WAIT state
+ *
+ * A transfer has completed; need to wait for the delay period to complete
+ * before starting the next transfer
+ */
+static int
+mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
+{
+	if (status && irq)
+		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
+			status);
+
+	if (((int)get_tbl()) - ms->timestamp < 0)
+		return FSM_POLL;
+
+	ms->message->actual_length += ms->transfer->len;
+
+	/* Check if there is another transfer in this message.  If there
+	 * aren't then deactivate CS, notify sender, and drop back to idle
+	 * to start the next message. */
+	if (ms->transfer->transfer_list.next == &ms->message->transfers) {
+		ms->msg_count++;
+		mpc52xx_spi_chipsel(ms, 0);
+		ms->message->status = 0;
+		ms->message->complete(ms->message->context);
+		ms->state = mpc52xx_spi_fsmstate_idle;
+		return FSM_CONTINUE;
+	}
+
+	/* There is another transfer; kick it off */
+
+	if (ms->cs_change)
+		mpc52xx_spi_chipsel(ms, 0);
+
+	ms->transfer = container_of(ms->transfer->transfer_list.next,
+				    struct spi_transfer, transfer_list);
+	mpc52xx_spi_start_transfer(ms);
+	ms->state = mpc52xx_spi_fsmstate_transfer;
+	return FSM_CONTINUE;
+}
+
+/**
+ * mpc52xx_spi_fsm_process - Finite State Machine iteration function
+ * @irq: irq number that triggered the FSM or 0 for polling
+ * @ms: pointer to mpc52xx_spi driver data
+ */
+static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
+{
+	int rc = FSM_CONTINUE;
+	u8 status, data;
+
+	while (rc == FSM_CONTINUE) {
+		/* Interrupt cleared by read of STATUS followed by
+		 * read of DATA registers */
+		status = in_8(ms->regs + SPI_STATUS);
+		data = in_8(ms->regs + SPI_DATA);
+		rc = ms->state(irq, ms, status, data);
+	}
+
+	if (rc == FSM_POLL)
+		schedule_work(&ms->work);
+}
+
+/**
+ * mpc52xx_spi_irq - IRQ handler
+ */
+static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
+{
+	struct mpc52xx_spi *ms = _ms;
+	spin_lock(&ms->lock);
+	mpc52xx_spi_fsm_process(irq, ms);
+	spin_unlock(&ms->lock);
+	return IRQ_HANDLED;
+}
+
+/**
+ * mpc52xx_spi_wq - Workqueue function for polling the state machine
+ */
+static void mpc52xx_spi_wq(struct work_struct *work)
+{
+	struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
+	unsigned long flags;
+
+	spin_lock_irqsave(&ms->lock, flags);
+	mpc52xx_spi_fsm_process(0, ms);
+	spin_unlock_irqrestore(&ms->lock, flags);
+}
+
+/*
+ * spi_master ops
+ */
+
+static int mpc52xx_spi_setup(struct spi_device *spi)
+{
+	if (spi->bits_per_word % 8)
+		return -EINVAL;
+
+	if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
+		return -EINVAL;
+
+	if (spi->chip_select >= spi->master->num_chipselect)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+	struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
+	unsigned long flags;
+
+	m->actual_length = 0;
+	m->status = -EINPROGRESS;
+
+	spin_lock_irqsave(&ms->lock, flags);
+	list_add_tail(&m->queue, &ms->queue);
+	spin_unlock_irqrestore(&ms->lock, flags);
+	schedule_work(&ms->work);
+
+	return 0;
+}
+
+/*
+ * OF Platform Bus Binding
+ */
+static int __devinit mpc52xx_spi_probe(struct of_device *op,
+				       const struct of_device_id *match)
+{
+	struct spi_master *master;
+	struct mpc52xx_spi *ms;
+	void __iomem *regs;
+	int rc;
+
+	/* MMIO registers */
+	dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
+	regs = of_iomap(op->node, 0);
+	if (!regs)
+		return -ENODEV;
+
+	/* initialize the device */
+	out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
+	out_8(regs + SPI_CTRL2, 0x0);
+	out_8(regs + SPI_DATADIR, 0xe);	/* Set output pins */
+	out_8(regs + SPI_PORTDATA, 0x8);	/* Deassert /SS signal */
+
+	/* Clear the status register and re-read it to check for a MODF
+	 * failure.  This driver cannot currently handle multiple masters
+	 * on the SPI bus.  This fault will also occur if the SPI signals
+	 * are not connected to any pins (port_config setting) */
+	in_8(regs + SPI_STATUS);
+	in_8(regs + SPI_DATA);
+	if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
+		dev_err(&op->dev, "mode fault; is port_config correct?\n");
+		rc = -EIO;
+		goto err_init;
+	}
+
+	dev_dbg(&op->dev, "allocating spi_master struct\n");
+	master = spi_alloc_master(&op->dev, sizeof *ms);
+	if (!master) {
+		rc = -ENOMEM;
+		goto err_alloc;
+	}
+	master->bus_num = -1;
+	master->num_chipselect = 1;
+	master->setup = mpc52xx_spi_setup;
+	master->transfer = mpc52xx_spi_transfer;
+	dev_set_drvdata(&op->dev, master);
+
+	ms = spi_master_get_devdata(master);
+	ms->master = master;
+	ms->regs = regs;
+	ms->irq0 = irq_of_parse_and_map(op->node, 0);
+	ms->irq1 = irq_of_parse_and_map(op->node, 1);
+	ms->state = mpc52xx_spi_fsmstate_idle;
+	ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
+	spin_lock_init(&ms->lock);
+	INIT_LIST_HEAD(&ms->queue);
+	INIT_WORK(&ms->work, mpc52xx_spi_wq);
+
+	/* Decide if interrupts can be used */
+	if (ms->irq0 && ms->irq1) {
+		rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+				  "mpc5200-spi-modf", ms);
+		rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
+				  "mpc5200-spi-spiF", ms);
+		if (rc) {
+			free_irq(ms->irq0, ms);
+			free_irq(ms->irq1, ms);
+			ms->irq0 = ms->irq1 = 0;
+		}
+	} else {
+		/* operate in polled mode */
+		ms->irq0 = ms->irq1 = 0;
+	}
+
+	if (!ms->irq0)
+		dev_info(&op->dev, "using polled mode\n");
+
+	dev_dbg(&op->dev, "registering spi_master struct\n");
+	rc = spi_register_master(master);
+	if (rc)
+		goto err_register;
+
+	of_register_spi_devices(master, op->node);
+	dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
+
+	return rc;
+
+ err_register:
+	dev_err(&ms->master->dev, "initialization failed\n");
+	spi_master_put(master);
+ err_alloc:
+ err_init:
+	iounmap(regs);
+	return rc;
+}
+
+static int __devexit mpc52xx_spi_remove(struct of_device *op)
+{
+	struct spi_master *master = dev_get_drvdata(&op->dev);
+	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
+
+	free_irq(ms->irq0, ms);
+	free_irq(ms->irq1, ms);
+
+	spi_unregister_master(master);
+	spi_master_put(master);
+	iounmap(ms->regs);
+
+	return 0;
+}
+
+static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
+	{ .compatible = "fsl,mpc5200-spi", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
+
+static struct of_platform_driver mpc52xx_spi_of_driver = {
+	.owner = THIS_MODULE,
+	.name = "mpc52xx-spi",
+	.match_table = mpc52xx_spi_match,
+	.probe = mpc52xx_spi_probe,
+	.remove = __exit_p(mpc52xx_spi_remove),
+};
+
+static int __init mpc52xx_spi_init(void)
+{
+	return of_register_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_init(mpc52xx_spi_init);
+
+static void __exit mpc52xx_spi_exit(void)
+{
+	of_unregister_platform_driver(&mpc52xx_spi_of_driver);
+}
+module_exit(mpc52xx_spi_exit);
+
diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
new file mode 100644
index 0000000..d1004cf
--- /dev/null
+++ b/include/linux/spi/mpc52xx_spi.h
@@ -0,0 +1,10 @@
+
+#ifndef INCLUDE_MPC5200_SPI_H
+#define INCLUDE_MPC5200_SPI_H
+
+extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
+					    void (*hook)(struct spi_message *m,
+							 void *context),
+					    void *hook_context);
+
+#endif

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

* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-06-18  2:55 [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver Grant Likely
@ 2009-06-18  6:58 ` Wolfram Sang
  2009-06-18 13:35   ` Grant Likely
  2009-10-21 13:17 ` Wolfram Sang
  1 sibling, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2009-06-18  6:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general


[-- Attachment #1.1: Type: text/plain, Size: 20824 bytes --]

Hi Grant,

some comments below:

(by the way, have you tested this patch on hardware? I wonder because of the
SSOE-issue, but maybe it works despite the documentation.)

On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
> Hi David,
> 
> It's been a while since I've posted this, but I believe I've addressed
> all the outstanding comments against v3, and people are asking me for it.
> The pre-message stuff is all gone and the error handling should be
> better now.
> 
> BTW, the premessage stuff was to handle a platform I had where some of
> the CS lines were controlled with a separate SPI transaction to the
> same SPI controller.  It almost looks like an SPI bridge.  It requires
> more thought before I try again.
> 
> Being a new driver, it would be nice to get it into 2.6.31, but
> definitely not critical.
> 
> g.
> 
>  drivers/spi/Kconfig             |    8 +
>  drivers/spi/Makefile            |    1 
>  drivers/spi/mpc52xx_spi.c       |  520 +++++++++++++++++++++++++++++++++++++++
>  include/linux/spi/mpc52xx_spi.h |   10 +
>  4 files changed, 539 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/spi/mpc52xx_spi.c
>  create mode 100644 include/linux/spi/mpc52xx_spi.h
> 
> 
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 83a185d..1994bcd 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -132,6 +132,14 @@ config SPI_LM70_LLP
>  	  which interfaces to an LM70 temperature sensor using
>  	  a parallel port.
>  
> +config SPI_MPC52xx
> +	tristate "Freescale MPC52xx SPI (non-PSC) controller support"
> +	depends on PPC_MPC52xx && SPI
> +	select SPI_MASTER_OF
> +	help
> +	  This drivers supports the MPC52xx SPI controller in master SPI
> +	  mode.
> +
>  config SPI_MPC52xx_PSC
>  	tristate "Freescale MPC52xx PSC SPI controller"
>  	depends on PPC_MPC52xx && EXPERIMENTAL
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index 5d04519..8de32c7 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -24,6 +24,7 @@ obj-$(CONFIG_SPI_OMAP_UWIRE)		+= omap_uwire.o
>  obj-$(CONFIG_SPI_OMAP24XX)		+= omap2_mcspi.o
>  obj-$(CONFIG_SPI_ORION)			+= orion_spi.o
>  obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
> +obj-$(CONFIG_SPI_MPC52xx)		+= mpc52xx_spi.o
>  obj-$(CONFIG_SPI_MPC83xx)		+= spi_mpc83xx.o
>  obj-$(CONFIG_SPI_S3C24XX_GPIO)		+= spi_s3c24xx_gpio.o
>  obj-$(CONFIG_SPI_S3C24XX)		+= spi_s3c24xx.o
> diff --git a/drivers/spi/mpc52xx_spi.c b/drivers/spi/mpc52xx_spi.c
> new file mode 100644
> index 0000000..ef8379b
> --- /dev/null
> +++ b/drivers/spi/mpc52xx_spi.c
> @@ -0,0 +1,520 @@
> +/*
> + * MPC52xx SPI bus driver.
> + *
> + * Copyright (C) 2008 Secret Lab Technologies Ltd.
> + *
> + * This file is released under the GPLv2
> + *
> + * This is the driver for the MPC5200's dedicated SPI controller.
> + *
> + * Note: this driver does not support the MPC5200 PSC in SPI mode.  For
> + * that driver see drivers/spi/mpc52xx_psc_spi.c
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/errno.h>
> +#include <linux/of_platform.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/spi/spi.h>
> +#include <linux/spi/mpc52xx_spi.h>

Is this still needed? See last comment...

> +#include <linux/of_spi.h>
> +#include <linux/io.h>
> +#include <asm/time.h>

Really asm?

> +#include <asm/mpc52xx.h>
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
> +MODULE_LICENSE("GPL");
> +
> +/* Register offsets */
> +#define SPI_CTRL1	0x00
> +#define SPI_CTRL1_SPIE		(1 << 7)
> +#define SPI_CTRL1_SPE		(1 << 6)
> +#define SPI_CTRL1_MSTR		(1 << 4)
> +#define SPI_CTRL1_CPOL		(1 << 3)
> +#define SPI_CTRL1_CPHA		(1 << 2)
> +#define SPI_CTRL1_SSOE		(1 << 1)
> +#define SPI_CTRL1_LSBFE		(1 << 0)
> +
> +#define SPI_CTRL2	0x01
> +#define SPI_BRR		0x04
> +
> +#define SPI_STATUS	0x05
> +#define SPI_STATUS_SPIF		(1 << 7)
> +#define SPI_STATUS_WCOL		(1 << 6)
> +#define SPI_STATUS_MODF		(1 << 4)
> +
> +#define SPI_DATA	0x09
> +#define SPI_PORTDATA	0x0d
> +#define SPI_DATADIR	0x10
> +
> +/* FSM state return values */
> +#define FSM_STOP	0	/* Nothing more for the state machine to */
> +				/* do.  If something interesting happens */
> +				/* then and IRQ will be received */

s/and/an/? Throughout the comments, there is sometimes a double space.

> +#define FSM_POLL	1	/* need to poll for completion, an IRQ is */
> +				/* not expected */
> +#define FSM_CONTINUE	2	/* Keep iterating the state machine */
> +
> +/* Driver internal data */
> +struct mpc52xx_spi {
> +	struct spi_master *master;
> +	u32 sysclk;

unused?

> +	void __iomem *regs;
> +	int irq0;	/* MODF irq */
> +	int irq1;	/* SPIF irq */
> +	int ipb_freq;

unsigned int will suit it better IMHO.

> +
> +	/* Statistics */
> +	int msg_count;
> +	int wcol_count;
> +	int wcol_ticks;
> +	u32 wcol_tx_timestamp;
> +	int modf_count;
> +	int byte_count;

Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
them will be ugly. Well, I can't make up if this is just overhead or useful
debugging aid, so no real objection :)

> +
> +	struct list_head queue;		/* queue of pending messages */
> +	spinlock_t lock;
> +	struct work_struct work;
> +
> +
> +	/* Details of current transfer (length, and buffer pointers) */
> +	struct spi_message *message;	/* current message */
> +	struct spi_transfer *transfer;	/* current transfer */
> +	int (*state)(int irq, struct mpc52xx_spi *ms, u8 status, u8 data);
> +	int len;
> +	int timestamp;
> +	u8 *rx_buf;
> +	const u8 *tx_buf;
> +	int cs_change;
> +};
> +
> +/*
> + * CS control function
> + */
> +static void mpc52xx_spi_chipsel(struct mpc52xx_spi *ms, int value)
> +{
> +	out_8(ms->regs + SPI_PORTDATA, value ? 0 : 0x08);

Magic value.

But I wonder more about the usage of the SS pin and if this chipsel is needed
at all (sadly I cannot test as I don't have any board with SPI connected to
that device). You define the SS-pin as output, but do not set the SSOE-bit.
More, you use the MODF-feature, so the SS-pin should be defined as input?
According to Table 17.3 in the PM, you have that pin defined as generic purpose
output.

> +}
> +
> +/*
> + * Start a new transfer.  This is called both by the idle state
> + * for the first transfer in a message, and by the wait state when the
> + * previous transfer in a message is complete.
> + */
> +static void mpc52xx_spi_start_transfer(struct mpc52xx_spi *ms)
> +{
> +	ms->rx_buf = ms->transfer->rx_buf;
> +	ms->tx_buf = ms->transfer->tx_buf;
> +	ms->len = ms->transfer->len;
> +
> +	/* Activate the chip select */
> +	if (ms->cs_change)
> +		mpc52xx_spi_chipsel(ms, 1);
> +	ms->cs_change = ms->transfer->cs_change;
> +
> +	/* Write out the first byte */
> +	ms->wcol_tx_timestamp = get_tbl();
> +	if (ms->tx_buf)
> +		out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> +	else
> +		out_8(ms->regs + SPI_DATA, 0);
> +}
> +
> +/* Forward declaration of state handlers */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> +					 u8 status, u8 data);
> +static int mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms,
> +				     u8 status, u8 data);
> +
> +/*
> + * IDLE state
> + *
> + * No transfers are in progress; if another transfer is pending then retrieve
> + * it and kick it off.  Otherwise, stop processing the state machine
> + */
> +static int
> +mpc52xx_spi_fsmstate_idle(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> +	struct spi_device *spi;
> +	int spr, sppr;
> +	u8 ctrl1;
> +
> +	if (status && (irq != NO_IRQ))
> +		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> +			status);
> +
> +	/* Check if there is another transfer waiting. */
> +	if (list_empty(&ms->queue))
> +		return FSM_STOP;
> +
> +	/* get the head of the queue */
> +	ms->message = list_first_entry(&ms->queue, struct spi_message, queue);
> +	list_del_init(&ms->message->queue);
> +
> +	/* Setup the controller parameters */
> +	ctrl1 = SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR;
> +	spi = ms->message->spi;
> +	if (spi->mode & SPI_CPHA)
> +		ctrl1 |= SPI_CTRL1_CPHA;
> +	if (spi->mode & SPI_CPOL)
> +		ctrl1 |= SPI_CTRL1_CPOL;
> +	if (spi->mode & SPI_LSB_FIRST)
> +		ctrl1 |= SPI_CTRL1_LSBFE;
> +	out_8(ms->regs + SPI_CTRL1, ctrl1);
> +
> +	/* Setup the controller speed */
> +	/* minimum divider is '2'.  Also, add '1' to force rounding the
> +	 * divider up. */
> +	sppr = ((ms->ipb_freq / ms->message->spi->max_speed_hz) + 1) >> 1;
> +	spr = 0;
> +	if (sppr < 1)
> +		sppr = 1;
> +	while (((sppr - 1) & ~0x7) != 0) {
> +		sppr = (sppr + 1) >> 1; /* add '1' to force rounding up */
> +		spr++;
> +	}
> +	sppr--;		/* sppr quantity in register is offset by 1 */
> +	if (spr > 7) {
> +		/* Don't overrun limits of SPI baudrate register */
> +		spr = 7;
> +		sppr = 7;
> +	}
> +	out_8(ms->regs + SPI_BRR, sppr << 4 | spr); /* Set speed */
> +
> +	ms->cs_change = 1;
> +	ms->transfer = container_of(ms->message->transfers.next,
> +				    struct spi_transfer, transfer_list);
> +
> +	mpc52xx_spi_start_transfer(ms);
> +	ms->state = mpc52xx_spi_fsmstate_transfer;
> +
> +	return FSM_CONTINUE;
> +}
> +
> +/*
> + * TRANSFER state
> + *
> + * In the middle of a transfer.  If the SPI core has completed processing
> + * a byte, then read out the received data and write out the next byte
> + * (unless this transfer is finished; in which case go on to the wait
> + * state)
> + */
> +static int mpc52xx_spi_fsmstate_transfer(int irq, struct mpc52xx_spi *ms,
> +					 u8 status, u8 data)
> +{
> +	if (!status)
> +		return ms->irq0 ? FSM_STOP : FSM_POLL;
> +
> +	if (status & SPI_STATUS_WCOL) {
> +		/* The SPI controller is stoopid.  At slower speeds, it may
> +		 * raise the SPIF flag before the state machine is actually
> +		 * finished, which causes a collision (internal to the state
> +		 * machine only).  The manual recommends inserting a delay
> +		 * between receiving the interrupt and sending the next byte,
> +		 * but it can also be worked around simply by retrying the
> +		 * transfer which is what we do here. */
> +		ms->wcol_count++;
> +		ms->wcol_ticks += get_tbl() - ms->wcol_tx_timestamp;
> +		ms->wcol_tx_timestamp = get_tbl();
> +		data = 0;
> +		if (ms->tx_buf)
> +			data = *(ms->tx_buf-1);

spaces around operator.

> +		out_8(ms->regs + SPI_DATA, data); /* try again */
> +		return FSM_CONTINUE;
> +	} else if (status & SPI_STATUS_MODF) {
> +		ms->modf_count++;
> +		dev_err(&ms->master->dev, "mode fault\n");
> +		mpc52xx_spi_chipsel(ms, 0);
> +		ms->message->status = -EIO;
> +		ms->message->complete(ms->message->context);
> +		ms->state = mpc52xx_spi_fsmstate_idle;
> +		return FSM_CONTINUE;
> +	}
> +
> +	/* Read data out of the spi device */
> +	ms->byte_count++;
> +	if (ms->rx_buf)
> +		*ms->rx_buf++ = data;
> +
> +	/* Is the transfer complete? */
> +	ms->len--;
> +	if (ms->len == 0) {
> +		ms->timestamp = get_tbl();
> +		ms->timestamp += ms->transfer->delay_usecs * tb_ticks_per_usec;
> +		ms->state = mpc52xx_spi_fsmstate_wait;
> +		return FSM_CONTINUE;
> +	}
> +
> +	/* Write out the next byte */
> +	ms->wcol_tx_timestamp = get_tbl();
> +	if (ms->tx_buf)
> +		out_8(ms->regs + SPI_DATA, *ms->tx_buf++);
> +	else
> +		out_8(ms->regs + SPI_DATA, 0);
> +
> +	return FSM_CONTINUE;
> +}
> +
> +/*
> + * WAIT state
> + *
> + * A transfer has completed; need to wait for the delay period to complete
> + * before starting the next transfer
> + */
> +static int
> +mpc52xx_spi_fsmstate_wait(int irq, struct mpc52xx_spi *ms, u8 status, u8 data)
> +{
> +	if (status && irq)
> +		dev_err(&ms->master->dev, "spurious irq, status=0x%.2x\n",
> +			status);
> +
> +	if (((int)get_tbl()) - ms->timestamp < 0)
> +		return FSM_POLL;
> +
> +	ms->message->actual_length += ms->transfer->len;
> +
> +	/* Check if there is another transfer in this message.  If there
> +	 * aren't then deactivate CS, notify sender, and drop back to idle
> +	 * to start the next message. */
> +	if (ms->transfer->transfer_list.next == &ms->message->transfers) {
> +		ms->msg_count++;
> +		mpc52xx_spi_chipsel(ms, 0);
> +		ms->message->status = 0;
> +		ms->message->complete(ms->message->context);
> +		ms->state = mpc52xx_spi_fsmstate_idle;
> +		return FSM_CONTINUE;
> +	}
> +
> +	/* There is another transfer; kick it off */
> +
> +	if (ms->cs_change)
> +		mpc52xx_spi_chipsel(ms, 0);
> +
> +	ms->transfer = container_of(ms->transfer->transfer_list.next,
> +				    struct spi_transfer, transfer_list);
> +	mpc52xx_spi_start_transfer(ms);
> +	ms->state = mpc52xx_spi_fsmstate_transfer;
> +	return FSM_CONTINUE;
> +}
> +
> +/**
> + * mpc52xx_spi_fsm_process - Finite State Machine iteration function
> + * @irq: irq number that triggered the FSM or 0 for polling
> + * @ms: pointer to mpc52xx_spi driver data
> + */
> +static void mpc52xx_spi_fsm_process(int irq, struct mpc52xx_spi *ms)
> +{
> +	int rc = FSM_CONTINUE;
> +	u8 status, data;
> +
> +	while (rc == FSM_CONTINUE) {
> +		/* Interrupt cleared by read of STATUS followed by
> +		 * read of DATA registers */
> +		status = in_8(ms->regs + SPI_STATUS);
> +		data = in_8(ms->regs + SPI_DATA);
> +		rc = ms->state(irq, ms, status, data);
> +	}
> +
> +	if (rc == FSM_POLL)
> +		schedule_work(&ms->work);
> +}
> +
> +/**
> + * mpc52xx_spi_irq - IRQ handler
> + */
> +static irqreturn_t mpc52xx_spi_irq(int irq, void *_ms)
> +{
> +	struct mpc52xx_spi *ms = _ms;
> +	spin_lock(&ms->lock);
> +	mpc52xx_spi_fsm_process(irq, ms);
> +	spin_unlock(&ms->lock);
> +	return IRQ_HANDLED;
> +}
> +
> +/**
> + * mpc52xx_spi_wq - Workqueue function for polling the state machine
> + */
> +static void mpc52xx_spi_wq(struct work_struct *work)
> +{
> +	struct mpc52xx_spi *ms = container_of(work, struct mpc52xx_spi, work);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&ms->lock, flags);
> +	mpc52xx_spi_fsm_process(0, ms);
> +	spin_unlock_irqrestore(&ms->lock, flags);
> +}
> +
> +/*
> + * spi_master ops
> + */
> +
> +static int mpc52xx_spi_setup(struct spi_device *spi)
> +{
> +	if (spi->bits_per_word % 8)
> +		return -EINVAL;
> +
> +	if (spi->mode & ~(SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST))
> +		return -EINVAL;
> +
> +	if (spi->chip_select >= spi->master->num_chipselect)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int mpc52xx_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(spi->master);
> +	unsigned long flags;
> +
> +	m->actual_length = 0;
> +	m->status = -EINPROGRESS;
> +
> +	spin_lock_irqsave(&ms->lock, flags);
> +	list_add_tail(&m->queue, &ms->queue);
> +	spin_unlock_irqrestore(&ms->lock, flags);
> +	schedule_work(&ms->work);
> +
> +	return 0;
> +}
> +
> +/*
> + * OF Platform Bus Binding
> + */
> +static int __devinit mpc52xx_spi_probe(struct of_device *op,
> +				       const struct of_device_id *match)
> +{
> +	struct spi_master *master;
> +	struct mpc52xx_spi *ms;
> +	void __iomem *regs;
> +	int rc;
> +
> +	/* MMIO registers */
> +	dev_dbg(&op->dev, "probing mpc5200 SPI device\n");
> +	regs = of_iomap(op->node, 0);
> +	if (!regs)
> +		return -ENODEV;
> +
> +	/* initialize the device */
> +	out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);

spaces around operator.

> +	out_8(regs + SPI_CTRL2, 0x0);
> +	out_8(regs + SPI_DATADIR, 0xe);	/* Set output pins */
> +	out_8(regs + SPI_PORTDATA, 0x8);	/* Deassert /SS signal */
> +
> +	/* Clear the status register and re-read it to check for a MODF
> +	 * failure.  This driver cannot currently handle multiple masters
> +	 * on the SPI bus.  This fault will also occur if the SPI signals
> +	 * are not connected to any pins (port_config setting) */
> +	in_8(regs + SPI_STATUS);
> +	in_8(regs + SPI_DATA);
> +	if (in_8(regs + SPI_STATUS) & SPI_STATUS_MODF) {
> +		dev_err(&op->dev, "mode fault; is port_config correct?\n");
> +		rc = -EIO;
> +		goto err_init;
> +	}
> +
> +	dev_dbg(&op->dev, "allocating spi_master struct\n");
> +	master = spi_alloc_master(&op->dev, sizeof *ms);
> +	if (!master) {
> +		rc = -ENOMEM;
> +		goto err_alloc;
> +	}
> +	master->bus_num = -1;
> +	master->num_chipselect = 1;
> +	master->setup = mpc52xx_spi_setup;
> +	master->transfer = mpc52xx_spi_transfer;
> +	dev_set_drvdata(&op->dev, master);
> +
> +	ms = spi_master_get_devdata(master);
> +	ms->master = master;
> +	ms->regs = regs;
> +	ms->irq0 = irq_of_parse_and_map(op->node, 0);
> +	ms->irq1 = irq_of_parse_and_map(op->node, 1);
> +	ms->state = mpc52xx_spi_fsmstate_idle;
> +	ms->ipb_freq = mpc5xxx_get_bus_frequency(op->node);
> +	spin_lock_init(&ms->lock);
> +	INIT_LIST_HEAD(&ms->queue);
> +	INIT_WORK(&ms->work, mpc52xx_spi_wq);
> +
> +	/* Decide if interrupts can be used */
> +	if (ms->irq0 && ms->irq1) {
> +		rc = request_irq(ms->irq0, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> +				  "mpc5200-spi-modf", ms);
> +		rc |= request_irq(ms->irq1, mpc52xx_spi_irq, IRQF_SAMPLE_RANDOM,
> +				  "mpc5200-spi-spiF", ms);

Capital 'F' wanted?

> +		if (rc) {
> +			free_irq(ms->irq0, ms);
> +			free_irq(ms->irq1, ms);
> +			ms->irq0 = ms->irq1 = 0;
> +		}
> +	} else {
> +		/* operate in polled mode */
> +		ms->irq0 = ms->irq1 = 0;
> +	}
> +
> +	if (!ms->irq0)
> +		dev_info(&op->dev, "using polled mode\n");
> +
> +	dev_dbg(&op->dev, "registering spi_master struct\n");
> +	rc = spi_register_master(master);
> +	if (rc)
> +		goto err_register;
> +
> +	of_register_spi_devices(master, op->node);
> +	dev_info(&ms->master->dev, "registered MPC5200 SPI bus\n");
> +
> +	return rc;
> +
> + err_register:
> +	dev_err(&ms->master->dev, "initialization failed\n");
> +	spi_master_put(master);
> + err_alloc:
> + err_init:
> +	iounmap(regs);
> +	return rc;
> +}
> +
> +static int __devexit mpc52xx_spi_remove(struct of_device *op)
> +{
> +	struct spi_master *master = dev_get_drvdata(&op->dev);
> +	struct mpc52xx_spi *ms = spi_master_get_devdata(master);
> +
> +	free_irq(ms->irq0, ms);
> +	free_irq(ms->irq1, ms);
> +
> +	spi_unregister_master(master);
> +	spi_master_put(master);
> +	iounmap(ms->regs);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id mpc52xx_spi_match[] __devinitdata = {
> +	{ .compatible = "fsl,mpc5200-spi", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mpc52xx_spi_match);
> +
> +static struct of_platform_driver mpc52xx_spi_of_driver = {
> +	.owner = THIS_MODULE,
> +	.name = "mpc52xx-spi",
> +	.match_table = mpc52xx_spi_match,
> +	.probe = mpc52xx_spi_probe,
> +	.remove = __exit_p(mpc52xx_spi_remove),
> +};
> +
> +static int __init mpc52xx_spi_init(void)
> +{
> +	return of_register_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_init(mpc52xx_spi_init);
> +
> +static void __exit mpc52xx_spi_exit(void)
> +{
> +	of_unregister_platform_driver(&mpc52xx_spi_of_driver);
> +}
> +module_exit(mpc52xx_spi_exit);
> +
> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
> new file mode 100644
> index 0000000..d1004cf
> --- /dev/null
> +++ b/include/linux/spi/mpc52xx_spi.h
> @@ -0,0 +1,10 @@
> +
> +#ifndef INCLUDE_MPC5200_SPI_H
> +#define INCLUDE_MPC5200_SPI_H
> +
> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
> +					    void (*hook)(struct spi_message *m,
> +							 void *context),
> +					    void *hook_context);
> +
> +#endif

This can be dropped for now and reintroduced when needed, I think.

> 
> 
> ------------------------------------------------------------------------------
> Crystal Reports - New Free Runtime and 30 Day Trial
> Check out the new simplified licensing option that enables unlimited
> royalty-free distribution of the report engine for externally facing 
> server and web deployment.
> http://p.sf.net/sfu/businessobjects
> _______________________________________________
> spi-devel-general mailing list
> spi-devel-general@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/spi-devel-general

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-06-18  6:58 ` [spi-devel-general] " Wolfram Sang
@ 2009-06-18 13:35   ` Grant Likely
  2009-06-18 14:26     ` Wolfram Sang
  0 siblings, 1 reply; 8+ messages in thread
From: Grant Likely @ 2009-06-18 13:35 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general

On Thu, Jun 18, 2009 at 12:58 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
> Hi Grant,
>
> some comments below:

Hi Wolfram.  Thanks for the review.

> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> +#include <linux/spi/mpc52xx_spi.h>
>
> Is this still needed? See last comment...

No, not needed at all.  Will remove.

>> +#include <linux/of_spi.h>
>> +#include <linux/io.h>
>> +#include <asm/time.h>
>
> Really asm?

Yes, really asm.  Needed for get_tlb()

>> +#include <asm/mpc52xx.h>
>> +
>> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
>> +MODULE_DESCRIPTION("MPC52xx SPI (non-PSC) Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> +/* Register offsets */
>> +#define SPI_CTRL1    0x00
>> +#define SPI_CTRL1_SPIE               (1 << 7)
>> +#define SPI_CTRL1_SPE                (1 << 6)
>> +#define SPI_CTRL1_MSTR               (1 << 4)
>> +#define SPI_CTRL1_CPOL               (1 << 3)
>> +#define SPI_CTRL1_CPHA               (1 << 2)
>> +#define SPI_CTRL1_SSOE               (1 << 1)
>> +#define SPI_CTRL1_LSBFE              (1 << 0)
>> +
>> +#define SPI_CTRL2    0x01
>> +#define SPI_BRR              0x04
>> +
>> +#define SPI_STATUS   0x05
>> +#define SPI_STATUS_SPIF              (1 << 7)
>> +#define SPI_STATUS_WCOL              (1 << 6)
>> +#define SPI_STATUS_MODF              (1 << 4)
>> +
>> +#define SPI_DATA     0x09
>> +#define SPI_PORTDATA 0x0d
>> +#define SPI_DATADIR  0x10
>> +
>> +/* FSM state return values */
>> +#define FSM_STOP     0       /* Nothing more for the state machine to */
>> +                             /* do.  If something interesting happens */
>> +                             /* then and IRQ will be received */
>
> s/and/an/? Throughout the comments, there is sometimes a double space.

The double spaces at the end of sentences are intentional.  It is
perhaps a bit quaint and old fashioned, but it is my writing style.

>
>> +#define FSM_POLL     1       /* need to poll for completion, an IRQ is */
>> +                             /* not expected */
>> +#define FSM_CONTINUE 2       /* Keep iterating the state machine */
>> +
>> +/* Driver internal data */
>> +struct mpc52xx_spi {
>> +     struct spi_master *master;
>> +     u32 sysclk;
>
> unused?

yes, fixed.

>> +     void __iomem *regs;
>> +     int irq0;       /* MODF irq */
>> +     int irq1;       /* SPIF irq */
>> +     int ipb_freq;
>
> unsigned int will suit it better IMHO.

right.

>> +
>> +     /* Statistics */
>> +     int msg_count;
>> +     int wcol_count;
>> +     int wcol_ticks;
>> +     u32 wcol_tx_timestamp;
>> +     int modf_count;
>> +     int byte_count;
>
> Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> them will be ugly. Well, I can't make up if this is just overhead or useful
> debugging aid, so no real objection :)

There used to be a sysfs interface for dumping these, but it was an
ugly misuse.  I'd like to leave these in.  I still have the sysfs bits
in a private patch and I'm going to rework them for debugfs.

> But I wonder more about the usage of the SS pin and if this chipsel is needed
> at all (sadly I cannot test as I don't have any board with SPI connected to
> that device). You define the SS-pin as output, but do not set the SSOE-bit.
> More, you use the MODF-feature, so the SS-pin should be defined as input?
> According to Table 17.3 in the PM, you have that pin defined as generic purpose
> output.

That's right.  The SS handling by the SPI device is completely
useless, so this driver uses it as a GPIO and asserts it manually.
The MODF irq is probably irrelevant, but I'd like to leave it in for
completeness.

>> +     /* initialize the device */
>> +     out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
>
> spaces around operator.

Intentional to keep from spilling past column 80; but I'll move the
missing spaces to around the | operators.  I think it is easier to
read that way.

>> diff --git a/include/linux/spi/mpc52xx_spi.h b/include/linux/spi/mpc52xx_spi.h
>> new file mode 100644
>> index 0000000..d1004cf
>> --- /dev/null
>> +++ b/include/linux/spi/mpc52xx_spi.h
>> @@ -0,0 +1,10 @@
>> +
>> +#ifndef INCLUDE_MPC5200_SPI_H
>> +#define INCLUDE_MPC5200_SPI_H
>> +
>> +extern void mpc52xx_spi_set_premessage_hook(struct spi_master *master,
>> +                                         void (*hook)(struct spi_message *m,
>> +                                                      void *context),
>> +                                         void *hook_context);
>> +
>> +#endif
>
> This can be dropped for now and reintroduced when needed, I think.

Yeah, this is cruft.  Removed.

Thanks!
g.

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

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

* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-06-18 13:35   ` Grant Likely
@ 2009-06-18 14:26     ` Wolfram Sang
  2009-07-03  7:01       ` Grant Likely
  0 siblings, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2009-06-18 14:26 UTC (permalink / raw)
  To: Grant Likely
  Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general


[-- Attachment #1.1: Type: text/plain, Size: 2272 bytes --]

Hi Grant,

> The double spaces at the end of sentences are intentional.  It is
> perhaps a bit quaint and old fashioned, but it is my writing style.

Ah, okay.

> >> +
> >> +     /* Statistics */
> >> +     int msg_count;
> >> +     int wcol_count;
> >> +     int wcol_ticks;
> >> +     u32 wcol_tx_timestamp;
> >> +     int modf_count;
> >> +     int byte_count;
> >
> > Hmm, there isn't even a debug-printout for those. Putting #ifdef DEBUG around
> > them will be ugly. Well, I can't make up if this is just overhead or useful
> > debugging aid, so no real objection :)
> 
> There used to be a sysfs interface for dumping these, but it was an
> ugly misuse.  I'd like to leave these in.  I still have the sysfs bits
> in a private patch and I'm going to rework them for debugfs.

Okay. Maybe a comment stating the future use will be nice.

> 
> > But I wonder more about the usage of the SS pin and if this chipsel is needed
> > at all (sadly I cannot test as I don't have any board with SPI connected to
> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
> > More, you use the MODF-feature, so the SS-pin should be defined as input?
> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
> > output.
> 
> That's right.  The SS handling by the SPI device is completely
> useless, so this driver uses it as a GPIO and asserts it manually.

That definately needs a comment :D (perhaps with some more details if you know them).

> The MODF irq is probably irrelevant, but I'd like to leave it in for
> completeness.

But it won't work if the pin is set to output, no? Are you sure there are no
side-effects?


> >> +     /* initialize the device */
> >> +     out_8(regs+SPI_CTRL1, SPI_CTRL1_SPIE | SPI_CTRL1_SPE | SPI_CTRL1_MSTR);
> >
> > spaces around operator.
> 
> Intentional to keep from spilling past column 80; but I'll move the
> missing spaces to around the | operators.  I think it is easier to
> read that way.

Ah, I remember, we had this topic a while ago :D

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

* Re: [spi-devel-general] [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-06-18 14:26     ` Wolfram Sang
@ 2009-07-03  7:01       ` Grant Likely
  0 siblings, 0 replies; 8+ messages in thread
From: Grant Likely @ 2009-07-03  7:01 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linuxppc-dev, David Brownell, spi-devel-general, linux-kernel, jonsmirl

On Thu, Jun 18, 2009 at 8:26 AM, Wolfram Sang<w.sang@pengutronix.de> wrote:
>> There used to be a sysfs interface for dumping these, but it was an
>> ugly misuse.  I'd like to leave these in.  I still have the sysfs bits
>> in a private patch and I'm going to rework them for debugfs.
>
> Okay. Maybe a comment stating the future use will be nice.

okay

>> > But I wonder more about the usage of the SS pin and if this chipsel is needed
>> > at all (sadly I cannot test as I don't have any board with SPI connected to
>> > that device). You define the SS-pin as output, but do not set the SSOE-bit.
>> > More, you use the MODF-feature, so the SS-pin should be defined as input?
>> > According to Table 17.3 in the PM, you have that pin defined as generic purpose
>> > output.
>>
>> That's right.  The SS handling by the SPI device is completely
>> useless, so this driver uses it as a GPIO and asserts it manually.
>
> That definately needs a comment :D (perhaps with some more details if you know them).
>
>> The MODF irq is probably irrelevant, but I'd like to leave it in for
>> completeness.
>
> But it won't work if the pin is set to output, no?

yes

> Are you sure there are no side-effects?

I'm sure.

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

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

* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-06-18  2:55 [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver Grant Likely
  2009-06-18  6:58 ` [spi-devel-general] " Wolfram Sang
@ 2009-10-21 13:17 ` Wolfram Sang
  2009-10-21 14:45   ` Grant Likely
  1 sibling, 1 reply; 8+ messages in thread
From: Wolfram Sang @ 2009-10-21 13:17 UTC (permalink / raw)
  To: Grant Likely
  Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general


[-- Attachment #1.1: Type: text/plain, Size: 535 bytes --]

Hi Grant,

On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> 
> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
> SoC.
> 
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>

do you have an updated version to share? Or is V4 still 'status quo'?

Genki de ;)

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-10-21 13:17 ` Wolfram Sang
@ 2009-10-21 14:45   ` Grant Likely
  2009-10-21 17:06     ` Wolfram Sang
  0 siblings, 1 reply; 8+ messages in thread
From: Grant Likely @ 2009-10-21 14:45 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general

On Wed, Oct 21, 2009 at 10:17 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Grant,
>
> On Wed, Jun 17, 2009 at 08:55:01PM -0600, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
>>
>> Adds support for the dedicated SPI device on the Freescale MPC5200(b)
>> SoC.
>>
>> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
>
> do you have an updated version to share? Or is V4 still 'status quo'?

V4 was supposed to go in during the merge window.  But I haven't heard
anything from David (and I didn't pursue it either).  Unless David
objects, I'll put it into either my -merge or my -next tree; depending
on what Ben and Linus prefer.

Cheers,
g.

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

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

* Re: [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver
  2009-10-21 14:45   ` Grant Likely
@ 2009-10-21 17:06     ` Wolfram Sang
  0 siblings, 0 replies; 8+ messages in thread
From: Wolfram Sang @ 2009-10-21 17:06 UTC (permalink / raw)
  To: Grant Likely
  Cc: linuxppc-dev, David Brownell, linux-kernel, spi-devel-general


[-- Attachment #1.1: Type: text/plain, Size: 560 bytes --]


> V4 was supposed to go in during the merge window.  But I haven't heard
> anything from David (and I didn't pursue it either).  Unless David
> objects, I'll put it into either my -merge or my -next tree; depending
> on what Ben and Linus prefer.

I wondered if there was going to be a V5 as you said you fixed a few things
according to my review and couldn't find an updated patch for that.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

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

end of thread, other threads:[~2009-10-21 17:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-06-18  2:55 [PATCH v4] powerpc/5200: Add mpc5200-spi (non-PSC) device driver Grant Likely
2009-06-18  6:58 ` [spi-devel-general] " Wolfram Sang
2009-06-18 13:35   ` Grant Likely
2009-06-18 14:26     ` Wolfram Sang
2009-07-03  7:01       ` Grant Likely
2009-10-21 13:17 ` Wolfram Sang
2009-10-21 14:45   ` Grant Likely
2009-10-21 17:06     ` Wolfram Sang

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).