[PATCH] TXx9 SPI controller driver (take 2)

[PATCH] TXx9 SPI controller driver (take 2)

[Atsushi Nemoto]

This is a driver for SPI controller built into TXx9 MIPS SoCs.
This driver is derived from arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c.

Signed-off-by: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
---
Changes from previous version:

* Whitespace cleanup.
* spi->mode checking.
* Remove I/O barrier after gpio_set_value()
* Do not modify spi->max_speed_hz in _setup function.
* Deselect chip in _setup function.
* Check per-transfer parameters.
* Move all ndelay() into txx9spi_cs_func().
* Fix cs_change hint handling.
* Remove mapping hack, expecting ioremap() just works.
* Use the clock framework (clk_get()) instead of abusing resource framework.
* Initialize num_chipselect explicitly.
* Use platform_driver_probe() instead of platform_driver_register().

 drivers/spi/Kconfig    |    6 +
 drivers/spi/Makefile   |    1 +
 drivers/spi/spi_txx9.c |  459 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 466 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 5e3f748..36911b6 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -156,6 +156,12 @@ config SPI_S3C24XX_GPIO
 	  GPIO lines to provide the SPI bus. This can be used where
 	  the inbuilt hardware cannot provide the transfer mode, or
 	  where the board is using non hardware connected pins.
+
+config SPI_TXX9
+	tristate "Toshiba TXx9 SPI controller"
+	depends on SPI_MASTER && GENERIC_GPIO && CPU_TX49XX
+	help
+	  SPI driver for Toshiba TXx9 MIPS SoCs
 #
 # Add new SPI master controllers in alphabetical order above this line
 #
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 5788d86..7869eb5 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_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
+obj-$(CONFIG_SPI_TXX9)			+= spi_txx9.o
 # 	... add above this line ...
 
 # SPI protocol drivers (device/link on bus)
diff --git a/drivers/spi/spi_txx9.c b/drivers/spi/spi_txx9.c
new file mode 100644
index 0000000..fd6a939
--- /dev/null
+++ b/drivers/spi/spi_txx9.c
@@ -0,0 +1,459 @@
+/*
+ * spi_txx9.c - TXx9 SPI controller driver.
+ *
+ * Based on linux/arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c
+ * Copyright (C) 2000-2001 Toshiba Corporation
+ *
+ * 2003-2005 (c) MontaVista Software, Inc. This file is licensed under the
+ * terms of the GNU General Public License version 2. This program is
+ * licensed "as is" without any warranty of any kind, whether express
+ * or implied.
+ *
+ * Support for TX4938 in 2.6 - Manish Lachwani (mlachwani-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org)
+ *
+ * Convert to generic SPI framework - Atsushi Nemoto (anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org)
+ */
+#include <linux/init.h>
+#include <linux/delay.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/workqueue.h>
+#include <linux/spi/spi.h>
+#include <linux/err.h>
+#include <linux/clk.h>
+#include <asm/gpio.h>
+
+
+#define SPI_FIFO_SIZE 4
+
+#define TXx9_SPMCR		0x00
+#define TXx9_SPCR0		0x04
+#define TXx9_SPCR1		0x08
+#define TXx9_SPFS		0x0c
+#define TXx9_SPSR		0x14
+#define TXx9_SPDR		0x18
+
+/* SPMCR : SPI Master Control */
+#define TXx9_SPMCR_OPMODE	0xc0
+#define TXx9_SPMCR_CONFIG	0x40
+#define TXx9_SPMCR_ACTIVE	0x80
+#define TXx9_SPMCR_SPSTP	0x02
+#define TXx9_SPMCR_BCLR		0x01
+
+/* SPCR0 : SPI Control 0 */
+#define TXx9_SPCR0_TXIFL_MASK	0xc000
+#define TXx9_SPCR0_RXIFL_MASK	0x3000
+#define TXx9_SPCR0_SIDIE	0x0800
+#define TXx9_SPCR0_SOEIE	0x0400
+#define TXx9_SPCR0_RBSIE	0x0200
+#define TXx9_SPCR0_TBSIE	0x0100
+#define TXx9_SPCR0_IFSPSE	0x0010
+#define TXx9_SPCR0_SBOS		0x0004
+#define TXx9_SPCR0_SPHA		0x0002
+#define TXx9_SPCR0_SPOL		0x0001
+
+/* SPSR : SPI Status */
+#define TXx9_SPSR_TBSI		0x8000
+#define TXx9_SPSR_RBSI		0x4000
+#define TXx9_SPSR_TBS_MASK	0x3800
+#define TXx9_SPSR_RBS_MASK	0x0700
+#define TXx9_SPSR_SPOE		0x0080
+#define TXx9_SPSR_IFSD		0x0008
+#define TXx9_SPSR_SIDLE		0x0004
+#define TXx9_SPSR_STRDY		0x0002
+#define TXx9_SPSR_SRRDY		0x0001
+
+
+struct txx9spi {
+	struct workqueue_struct	*workqueue;
+	struct work_struct work;
+	spinlock_t lock;	/* protect 'queue' */
+	struct list_head queue;
+	wait_queue_head_t waitq;
+	void __iomem *membase;
+	int irq;
+	int baseclk;
+	struct clk *clk;
+	u32 max_speed_hz, min_speed_hz;
+	int last_chipselect;
+	int last_chipselect_val;
+};
+
+static u32 txx9spi_rd(struct txx9spi *c, int reg)
+{
+	return __raw_readl(c->membase + reg);
+}
+static void txx9spi_wr(struct txx9spi *c, u32 val, int reg)
+{
+	__raw_writel(val, c->membase + reg);
+}
+
+static void txx9spi_cs_func(struct spi_device *spi, struct txx9spi *c,
+			    int on, unsigned int cs_delay)
+{
+	int val = (spi->mode & SPI_CS_HIGH) ? on : !on;
+	if (on) {
+		/* deselect the chip with cs_change hint in last transfer */
+		if (c->last_chipselect >= 0)
+			gpio_set_value(c->last_chipselect,
+				       !c->last_chipselect_val);
+		c->last_chipselect = spi->chip_select;
+		c->last_chipselect_val = val;
+	} else {
+		c->last_chipselect = -1;
+		ndelay(cs_delay);	/* CS Hold Time */
+	}
+	gpio_set_value(spi->chip_select, val);
+	ndelay(cs_delay);	/* CS Setup Time / CS Recovery Time */
+}
+
+/* the spi->mode bits understood by this driver: */
+#define MODEBITS	(SPI_CS_HIGH|SPI_CPOL|SPI_CPHA)
+
+static int txx9spi_setup(struct spi_device *spi)
+{
+	struct txx9spi *c = spi_master_get_devdata(spi->master);
+	u8 bits_per_word;
+
+	if (spi->mode & ~MODEBITS)
+		return -EINVAL;
+
+	if (!spi->max_speed_hz ||
+	    spi->max_speed_hz > c->max_speed_hz ||
+	    spi->max_speed_hz < c->min_speed_hz)
+		return -EINVAL;
+
+	bits_per_word = spi->bits_per_word ?: 8;
+	if (bits_per_word != 8 && bits_per_word != 16)
+		return -EINVAL;
+
+	if (gpio_direction_output(spi->chip_select,
+				  !(spi->mode & SPI_CS_HIGH))) {
+		dev_err(&spi->dev, "Cannot setup GPIO for chipselect.\n");
+		return -EINVAL;
+	}
+
+	/* deselect chip */
+	spin_lock(&c->lock);
+	txx9spi_cs_func(spi, c, 0, 1000000000 / 2 / spi->max_speed_hz);
+	spin_unlock(&c->lock);
+
+	return 0;
+}
+
+static irqreturn_t txx9spi_interrupt(int irq, void *dev_id)
+{
+	struct txx9spi *c = dev_id;
+
+	/* disable rx intr */
+	txx9spi_wr(c, txx9spi_rd(c, TXx9_SPCR0) & ~TXx9_SPCR0_RBSIE,
+		   TXx9_SPCR0);
+	wake_up(&c->waitq);
+	return IRQ_HANDLED;
+}
+
+static int txx9spi_work_one(struct txx9spi *c, struct spi_message *m)
+{
+	struct spi_device *spi = m->spi;
+	struct spi_transfer *t;
+	unsigned int cs_delay;
+	unsigned int cs_change;
+	int status;
+	u32 mcr;
+	u8 bits_per_word = spi->bits_per_word ?: 8;
+	u32 speed_hz = 0, n;
+
+	/* check each transter parameters */
+	list_for_each_entry (t, &m->transfers, transfer_list) {
+		if (!t->tx_buf && !t->rx_buf && t->len)
+			return -EINVAL;
+		if (t->bits_per_word && t->bits_per_word != bits_per_word)
+			return -EINVAL;
+		if (t->len & ((bits_per_word >> 3) - 1))
+			return -EINVAL;
+		if (!speed_hz)
+			speed_hz = t->speed_hz;
+		else if (speed_hz != t->speed_hz)
+			return -EINVAL;
+	}
+	if (!speed_hz)
+		speed_hz = spi->max_speed_hz;
+	if (!speed_hz || speed_hz > c->max_speed_hz)
+		speed_hz = c->max_speed_hz;
+	else if (speed_hz < c->min_speed_hz)
+		return -EINVAL;
+	n = (c->baseclk + speed_hz - 1) / speed_hz;
+	if (n < 1)
+		n = 1;
+	else if (n > 0xff)
+		n = 0xff;
+
+	mcr = txx9spi_rd(c, TXx9_SPMCR);
+	if (unlikely((mcr & TXx9_SPMCR_OPMODE) == TXx9_SPMCR_ACTIVE)) {
+		dev_err(&spi->dev, "Bad mode.\n");
+		return -EIO;
+	}
+
+	/* enter config mode */
+	mcr &= ~(TXx9_SPMCR_OPMODE | TXx9_SPMCR_SPSTP | TXx9_SPMCR_BCLR);
+	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
+	txx9spi_wr(c, TXx9_SPCR0_SBOS |
+		   ((spi->mode & SPI_CPOL) ? TXx9_SPCR0_SPOL : 0) |
+		   ((spi->mode & SPI_CPHA) ? TXx9_SPCR0_SPHA : 0) |
+		   0x08,
+		   TXx9_SPCR0);
+	txx9spi_wr(c, (n << 8) | bits_per_word, TXx9_SPCR1);
+	/* enter active mode */
+	txx9spi_wr(c, mcr | TXx9_SPMCR_ACTIVE, TXx9_SPMCR);
+
+	cs_change = 1;
+	status = 0;
+	/* CS setup/hold/recovery time in nsec */
+	cs_delay = 100 + 1000000000 / 2 / speed_hz;
+	list_for_each_entry (t, &m->transfers, transfer_list) {
+		const void *txbuf = t->tx_buf;
+		void *rxbuf = t->rx_buf;
+		u32 data;
+		unsigned int len = t->len;
+		unsigned int wsize = bits_per_word >> 3; /* in bytes */
+
+		if (cs_change)
+			txx9spi_cs_func(spi, c, 1, cs_delay);
+		cs_change = t->cs_change;
+		while (len) {
+			unsigned int count = SPI_FIFO_SIZE;
+			int i;
+			u32 cr0;
+			if (len < count * wsize)
+				count = len / wsize;
+			/* now tx must be idle... */
+			while (!(txx9spi_rd(c, TXx9_SPSR) & TXx9_SPSR_SIDLE))
+				;
+			cr0 = txx9spi_rd(c, TXx9_SPCR0);
+			cr0 &= ~TXx9_SPCR0_RXIFL_MASK;
+			cr0 |= (count - 1) << 12;
+			/* enable rx intr */
+			cr0 |= TXx9_SPCR0_RBSIE;
+			txx9spi_wr(c, cr0, TXx9_SPCR0);
+			/* send */
+			for (i = 0; i < count; i++) {
+				if (txbuf) {
+					data = wsize == 1 ?
+						*(const u8 *)txbuf :
+						*(const u16 *)txbuf;
+					txx9spi_wr(c, data, TXx9_SPDR);
+					txbuf += wsize;
+				} else
+					txx9spi_wr(c, 0, TXx9_SPDR);
+			}
+			/* wait all rx data */
+			wait_event(c->waitq,
+				   txx9spi_rd(c, TXx9_SPSR) & TXx9_SPSR_RBSI);
+			/* receive */
+			for (i = 0; i < count; i++) {
+				data = txx9spi_rd(c, TXx9_SPDR);
+				if (rxbuf) {
+					if (wsize == 1)
+						*(u8 *)rxbuf = data;
+					else
+						*(u16 *)rxbuf = data;
+					rxbuf += wsize;
+				}
+			}
+			len -= count * wsize;
+		}
+		m->actual_length += t->len;
+		if (t->delay_usecs)
+			udelay(t->delay_usecs);
+
+		if (!cs_change)
+			continue;
+		if (t->transfer_list.next == &m->transfers)
+			break;
+		/* sometimes a short mid-message deselect of the chip
+		 * may be needed to terminate a mode or command
+		 */
+		txx9spi_cs_func(spi, c, 0, cs_delay);
+	}
+
+	m->status = status;
+	m->complete(m->context);
+
+	/* normally deactivate chipselect ... unless no error and
+	 * cs_change has hinted that the next message will probably
+	 * be for this chip too.
+	 */
+	if (!(status == 0 && cs_change))
+		txx9spi_cs_func(spi, c, 0, cs_delay);
+
+	/* enter config mode */
+	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
+	return status;
+}
+
+static void txx9spi_work(struct work_struct *work)
+{
+	struct txx9spi *c = container_of(work, struct txx9spi, work);
+	unsigned long flags;
+
+	spin_lock_irqsave(&c->lock, flags);
+	while (!list_empty(&c->queue)) {
+		struct spi_message *m;
+
+		m = container_of(c->queue.next, struct spi_message, queue);
+		list_del_init(&m->queue);
+		spin_unlock_irqrestore(&c->lock, flags);
+
+		txx9spi_work_one(c, m);
+
+		spin_lock_irqsave(&c->lock, flags);
+	}
+	spin_unlock_irqrestore(&c->lock, flags);
+}
+
+static int txx9spi_transfer(struct spi_device *spi, struct spi_message *m)
+{
+	struct spi_master *master = spi->master;
+	struct txx9spi *c = spi_master_get_devdata(master);
+	unsigned long flags;
+
+	m->actual_length = 0;
+	spin_lock_irqsave(&c->lock, flags);
+	list_add_tail(&m->queue, &c->queue);
+	queue_work(c->workqueue, &c->work);
+	spin_unlock_irqrestore(&c->lock, flags);
+
+	return 0;
+}
+
+static int __init txx9spi_probe(struct platform_device *dev)
+{
+	struct spi_master *master;
+	struct txx9spi *c;
+	struct resource *res;
+	int ret = -ENODEV;
+	u32 mcr;
+
+	master = spi_alloc_master(&dev->dev, sizeof(*c));
+	if (!master)
+		return ret;
+	c = spi_master_get_devdata(master);
+	c->irq = -1;
+	platform_set_drvdata(dev, master);
+
+	INIT_WORK(&c->work, txx9spi_work);
+	spin_lock_init(&c->lock);
+	INIT_LIST_HEAD(&c->queue);
+	init_waitqueue_head(&c->waitq);
+
+	c->clk = clk_get(&dev->dev, "spi-baseclk");
+	if (IS_ERR(c->clk)) {
+		ret = PTR_ERR(c->clk);
+		c->clk = NULL;
+		goto exit;
+	}
+	if (clk_enable(c->clk)) {
+		clk_put(c->clk);
+		c->clk = NULL;
+		goto exit;
+	}
+	c->baseclk = clk_get_rate(c->clk);
+	c->min_speed_hz = (c->baseclk + 0xff - 1) / 0xff;
+	c->max_speed_hz = c->baseclk;
+
+	res = platform_get_resource(dev, IORESOURCE_MEM, 0);
+	if (!res)
+		goto exit;
+	c->membase = ioremap(res->start, res->end - res->start + 1);
+	if (!c->membase)
+		goto exit;
+
+	/* enter config mode */
+	mcr = txx9spi_rd(c, TXx9_SPMCR);
+	mcr &= ~(TXx9_SPMCR_OPMODE | TXx9_SPMCR_SPSTP | TXx9_SPMCR_BCLR);
+	txx9spi_wr(c, mcr | TXx9_SPMCR_CONFIG | TXx9_SPMCR_BCLR, TXx9_SPMCR);
+
+	c->irq = platform_get_irq(dev, 0);
+	if (c->irq < 0)
+		goto exit;
+	ret = request_irq(c->irq, txx9spi_interrupt, 0, dev->name, c);
+	if (ret) {
+		c->irq = -1;
+		goto exit;
+	}
+
+	c->workqueue = create_singlethread_workqueue(master->cdev.dev->bus_id);
+	if (!c->workqueue)
+		goto exit;
+	c->last_chipselect = -1;
+
+	dev_info(&dev->dev, "at %#llx, irq %d, %dMHz\n",
+		 (unsigned long long)res->start, c->irq,
+		 (c->baseclk + 500000) / 1000000);
+
+	master->bus_num = dev->id;
+	master->setup = txx9spi_setup;
+	master->transfer = txx9spi_transfer;
+	master->num_chipselect = 0;	/* unlimited: any GPIO numbers */
+
+	ret = spi_register_master(master);
+	if (ret)
+		goto exit;
+	return 0;
+exit:
+	if (c->workqueue)
+		destroy_workqueue(c->workqueue);
+	if (c->irq >= 0)
+		free_irq(c->irq, c);
+	if (c->membase)
+		iounmap(c->membase);
+	if (c->clk) {
+		clk_disable(c->clk);
+		clk_put(c->clk);
+	}
+	platform_set_drvdata(dev, NULL);
+	spi_master_put(master);
+	return ret;
+}
+
+static int __exit txx9spi_remove(struct platform_device *dev)
+{
+	struct spi_master *master = spi_master_get(platform_get_drvdata(dev));
+	struct txx9spi *c = spi_master_get_devdata(master);
+
+	spi_unregister_master(master);
+	platform_set_drvdata(dev, NULL);
+	destroy_workqueue(c->workqueue);
+	free_irq(c->irq, c);
+	iounmap(c->membase);
+	clk_disable(c->clk);
+	clk_put(c->clk);
+	spi_master_put(master);
+	return 0;
+}
+
+static struct platform_driver txx9spi_driver = {
+	.remove = __exit_p(txx9spi_remove),
+	.driver = {
+		.name = "txx9spi",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init txx9spi_init(void)
+{
+	return platform_driver_probe(&txx9spi_driver, txx9spi_probe);
+}
+subsys_initcall(txx9spi_init);
+
+static void __exit txx9spi_exit(void)
+{
+	platform_driver_unregister(&txx9spi_driver);
+}
+module_exit(txx9spi_exit);
+
+MODULE_DESCRIPTION("TXx9 SPI Driver");
+MODULE_LICENSE("GPL");

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver (take 2)

[David Brownell]

On Wednesday 27 June 2007, Atsushi Nemoto wrote:
> This is a driver for SPI controller built into TXx9 MIPS SoCs.
> This driver is derived from arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c.
> 
> Signed-off-by: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
> ---
> Changes from previous version:

Better, but still not there yet.
 
> * Whitespace cleanup.
> * spi->mode checking.
> * Remove I/O barrier after gpio_set_value()
> * Do not modify spi->max_speed_hz in _setup function.
> * Deselect chip in _setup function.
> * Check per-transfer parameters.

Checking these parameters is done at the wrong place though,
and is done incorrectly.  That's the main issue with this
patch.

> * Move all ndelay() into txx9spi_cs_func().
> * Fix cs_change hint handling.
> * Remove mapping hack, expecting ioremap() just works.
> * Use the clock framework (clk_get()) instead of abusing resource framework.
> * Initialize num_chipselect explicitly.

Yeah, but ... still not correctly!!


> * Use platform_driver_probe() instead of platform_driver_register().
> 
> ...
>
> +static int txx9spi_setup(struct spi_device *spi)
> +{
> +	...
> +
> +	/* deselect chip */
> +	spin_lock(&c->lock);
> +	txx9spi_cs_func(spi, c, 0, 1000000000 / 2 / spi->max_speed_hz);

You still use this confusing A/2/B syntax.  Please
rewrite that using one "/" and one "*".  (And there
is similar usage elsewhere.)


> +	spin_unlock(&c->lock);
> +
> +	return 0;
> +}
> +
> +...
> +
> +static int txx9spi_work_one(struct txx9spi *c, struct spi_message *m)
> +{
> +	struct spi_device *spi = m->spi;
> +	struct spi_transfer *t;
> +	unsigned int cs_delay;
> +	unsigned int cs_change;
> +	int status;
> +	u32 mcr;
> +	u8 bits_per_word = spi->bits_per_word ?: 8;
> +	u32 speed_hz = 0, n;
> +

These checks here should be in txx9_spi_transfer(), where
returning EINVAL will do some good.  The single caller to
this routine doesn't even look at its return value ... and
returning without issuing the message's completion callback
is just a bug.


> +	/* check each transter parameters */
> +	list_for_each_entry (t, &m->transfers, transfer_list) {
> +		if (!t->tx_buf && !t->rx_buf && t->len)
> +			return -EINVAL;
> +		if (t->bits_per_word && t->bits_per_word != bits_per_word)
> +			return -EINVAL;
> +		if (t->len & ((bits_per_word >> 3) - 1))
> +			return -EINVAL;
> +		if (!speed_hz)
> +			speed_hz = t->speed_hz;
> +		else if (speed_hz != t->speed_hz)

That speed check is wrong.  There's no reason two transfers
shouldn't have different speeds ... e.g. flash chips often
have speed limits in certain bulk reads, which don't apply
to other operations.


> +			return -EINVAL;
> +	}
> +	if (!speed_hz)
> +		speed_hz = spi->max_speed_hz;
> +	if (!speed_hz || speed_hz > c->max_speed_hz)
> +		speed_hz = c->max_speed_hz;
> +	else if (speed_hz < c->min_speed_hz)
> +		return -EINVAL;

Also, you can't replace per-transfer speed checks with one
for the overall message... each transfer could have a
very different speed.


> +	...
> +}
> +
> +...
> +
> +static int txx9spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> +	struct spi_master *master = spi->master;
> +	struct txx9spi *c = spi_master_get_devdata(master);
> +	unsigned long flags;
> +

Here's where the (corrected) checks for each spi_transfer in the
message belong:  if the message is invalid, don't even queue it,
just return -EINVAL.


> +	m->actual_length = 0;
> +	spin_lock_irqsave(&c->lock, flags);
> +	list_add_tail(&m->queue, &c->queue);
> +	queue_work(c->workqueue, &c->work);
> +	spin_unlock_irqrestore(&c->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int __init txx9spi_probe(struct platform_device *dev)
> +{
> +	struct spi_master *master;
> +	struct txx9spi *c;
> +	struct resource *res;
> +	int ret = -ENODEV;
> +	u32 mcr;
> +
> +	master = spi_alloc_master(&dev->dev, sizeof(*c));
> +	if (!master)
> +		return ret;
> +	c = spi_master_get_devdata(master);
> +	c->irq = -1;
> +	platform_set_drvdata(dev, master);
> +
> +	INIT_WORK(&c->work, txx9spi_work);
> +	spin_lock_init(&c->lock);
> +	INIT_LIST_HEAD(&c->queue);
> +	init_waitqueue_head(&c->waitq);
> +
> +	c->clk = clk_get(&dev->dev, "spi-baseclk");
> +	if (IS_ERR(c->clk)) {
> +		ret = PTR_ERR(c->clk);
> +		c->clk = NULL;
> +		goto exit;
> +	}
> +	if (clk_enable(c->clk)) {

Minor comment:  if power management is a concern, you might
consider leaving the clock disabled except when transfers
are active or you're accessing controller registers.  On
most chips, leaving a clock enabled all the time (like this)
means power is needlessly consumed.  (This isn't wrong, just
sub-optimal in terms of power reduction.)

> +	...
> +
> +	master->bus_num = dev->id;
> +	master->setup = txx9spi_setup;
> +	master->transfer = txx9spi_transfer;
> +	master->num_chipselect = 0;	/* unlimited: any GPIO numbers */

No, actually it means "no chipselects" as I said before;
the fact that this works right now is a bug that will be
fixed at some point.  INT_MAX would allow any GPIO.


... almost mergeable!

- Dave




-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver (take 2)

[Atsushi Nemoto]

On Sat, 30 Jun 2007 09:53:19 -0700, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > This is a driver for SPI controller built into TXx9 MIPS SoCs.
> > This driver is derived from arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c.
> > 
> > Signed-off-by: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
> > ---
> > Changes from previous version:
> 
> Better, but still not there yet.

Thanks!  I'll be back with take 3 patch.

> > +	txx9spi_cs_func(spi, c, 0, 1000000000 / 2 / spi->max_speed_hz);
> 
> You still use this confusing A/2/B syntax.  Please
> rewrite that using one "/" and one "*".  (And there
> is similar usage elsewhere.)

The compiler will optimize "1000000000 / 2 / spi->max_speed_hz" into
"500000000 / spi->max_speed_hz", so it can be treat as one "/", no?

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver (take 2)

[David Brownell]

On Saturday 30 June 2007, Atsushi Nemoto wrote:
> On Sat, 30 Jun 2007 09:53:19 -0700, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > > This is a driver for SPI controller built into TXx9 MIPS SoCs.
> > > This driver is derived from arch/mips/tx4938/toshiba_rbtx4938/spi_txx9.c.
> > > 
> > > Signed-off-by: Atsushi Nemoto <anemo-7JcRY8pycbNHfZP73Gtkiw@public.gmane.org>
> > > ---
> > > Changes from previous version:
> > 
> > Better, but still not there yet.
> 
> Thanks!  I'll be back with take 3 patch.
> 
> > > +	txx9spi_cs_func(spi, c, 0, 1000000000 / 2 / spi->max_speed_hz);
> > 
> > You still use this confusing A/2/B syntax.  Please
> > rewrite that using one "/" and one "*".  (And there
> > is similar usage elsewhere.)
> 
> The compiler will optimize "1000000000 / 2 / spi->max_speed_hz" into
> "500000000 / spi->max_speed_hz", so it can be treat as one "/", no?

Sure it's deterministic.  But that doesn't prevent me from
needing a double-take to figure what it does ... it's best
to avoid confusing idioms in code.  At the very least, put
parentheses there ...


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

Re: [PATCH] TXx9 SPI controller driver (take 2)

[Atsushi Nemoto]

On Sat, 30 Jun 2007 10:46:20 -0700, David Brownell <david-b@pacbell.net> wrote:
> > The compiler will optimize "1000000000 / 2 / spi->max_speed_hz" into
> > "500000000 / spi->max_speed_hz", so it can be treat as one "/", no?
> 
> Sure it's deterministic.  But that doesn't prevent me from
> needing a double-take to figure what it does ... it's best
> to avoid confusing idioms in code.  At the very least, put
> parentheses there ...

OK, I will use parentheses while I think 1000000000 is important part.
Well, maybe NSEC_PER_SEC would be better.
---
Atsushi Nemoto

Re: [PATCH] TXx9 SPI controller driver (take 2)

[Atsushi Nemoto]

On Sat, 30 Jun 2007 09:53:19 -0700, David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> wrote:
> > +	txx9spi_cs_func(spi, c, 0, 1000000000 / 2 / spi->max_speed_hz);
> 
> You still use this confusing A/2/B syntax.  Please
> rewrite that using one "/" and one "*".  (And there
> is similar usage elsewhere.)

Replaced with "(NSEC_PER_SEC / 2) / spi->max_speed_hz".

> These checks here should be in txx9_spi_transfer(), where
> returning EINVAL will do some good.  The single caller to
> this routine doesn't even look at its return value ... and
> returning without issuing the message's completion callback
> is just a bug.

Oh it was really a bug.  Fixed.

> That speed check is wrong.  There's no reason two transfers
> shouldn't have different speeds ... e.g. flash chips often
> have speed limits in certain bulk reads, which don't apply
> to other operations.

Done.

> Also, you can't replace per-transfer speed checks with one
> for the overall message... each transfer could have a
> very different speed.

Done.  Now per-transfer speed_hz and bits_per_word are really supported.

> Here's where the (corrected) checks for each spi_transfer in the
> message belong:  if the message is invalid, don't even queue it,
> just return -EINVAL.

Done.

> > +	if (clk_enable(c->clk)) {
> 
> Minor comment:  if power management is a concern, you might
> consider leaving the clock disabled except when transfers
> are active or you're accessing controller registers.  On
> most chips, leaving a clock enabled all the time (like this)
> means power is needlessly consumed.  (This isn't wrong, just
> sub-optimal in terms of power reduction.)

Well, I did not change this since we can not really stop the
"spi-baseclk" anyway.  But thank you for comment.

> > +	master->num_chipselect = 0;	/* unlimited: any GPIO numbers */
> 
> No, actually it means "no chipselects" as I said before;
> the fact that this works right now is a bug that will be
> fixed at some point.  INT_MAX would allow any GPIO.

Done.  I chose "(u16)INT_MAX" to silence a sparse warning.

> ... almost mergeable!

Let's go take3 !

---
Atsushi Nemoto

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/