Hello Mingkai,

There are mostly cosmetic comments down below.

On Thu, Sep 30, 2010 at 04:00:42PM +0800, Mingkai Hu wrote:
[...]

+/*
+ * Freescale eSPI controller driver.
+ *
+ * Copyright 2010 Freescale Semiconductor, Inc.
+ *
+ * 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.
+ */
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/irq.h>
+#include <linux/spi/spi.h>
+#include <linux/platform_device.h>
+#include <linux/fsl_devices.h>
+#include <linux/mm.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_spi.h>
+#include <sysdev/fsl_soc.h>

Please move the sysdev/ include after linux/.
Will make it a bit prettier. :-)

+#include <linux/interrupt.h>
+#include <linux/err.h>
+
+#include "spi_fsl_lib.h"

[...]

+static void fsl_espi_change_mode(struct spi_device *spi)
+{
+	struct mpc8xxx_spi *mspi = spi_master_get_devdata(spi->master);
+	struct spi_mpc8xxx_cs *cs = spi->controller_state;
+	struct fsl_espi_reg *reg_base = (struct fsl_espi_reg *)mspi->reg_base;

No need for the type cast. The same for the rest of the code.

+	__be32 __iomem *mode;
+	__be32 __iomem *espi_mode = NULL;
+	u32 tmp;
+	unsigned long flags;
+
+	espi_mode = &reg_base->mode;
+	mode = &reg_base->csmode[spi->chip_select];

Could save a few lines by turning this into initializers.

+
+	/* Turn off IRQs locally to minimize time that SPI is disabled. */
+	local_irq_save(flags);
+
+	/* Turn off SPI unit prior changing mode */
+	tmp = mpc8xxx_spi_read_reg(espi_mode);
+	mpc8xxx_spi_write_reg(espi_mode, tmp & ~SPMODE_ENABLE);
+	mpc8xxx_spi_write_reg(mode, cs->hw_mode);
+	mpc8xxx_spi_write_reg(espi_mode, tmp);
+
+	local_irq_restore(flags);
+}
+
+static u32 fsl_espi_tx_buf_lsb(struct mpc8xxx_spi *mpc8xxx_spi)
+{
+	u32 data;
+	u16 data_h, data_l;

u16 data_h;
u16 data_l;

+

No need for this empty line.

+	const u32 *tx = mpc8xxx_spi->tx;

<- Instead, add an empty line here.

+	if (!tx)
+		return 0;
+
+	data = *tx++ << mpc8xxx_spi->tx_shift;
+	data_l = data & 0xffff;
+	data_h = (data >> 16) & 0xffff;
+	swab16s(&data_l);
+	swab16s(&data_h);
+	data = data_h | data_l;
+
+	mpc8xxx_spi->tx = tx;
+	return data;
+}
+
+static int fsl_espi_setup_transfer(struct spi_device *spi,
+					struct spi_transfer *t)
+{
+	struct mpc8xxx_spi *mpc8xxx_spi;
+	int bits_per_word = 0;
+	u8 pm;
+	u32 hz = 0;
+	struct spi_mpc8xxx_cs	*cs = spi->controller_state;

Stray tab.

+
+	mpc8xxx_spi = spi_master_get_devdata(spi->master);

Could move this to the initializer.

+
+	if (t) {
+		bits_per_word = t->bits_per_word;
+		hz = t->speed_hz;
+	}
+
+	/* spi_transfer level calls that work per-word */
+	if (!bits_per_word)
+		bits_per_word = spi->bits_per_word;
+
+	/* Make sure its a bit width we support [4..16] */
+	if ((bits_per_word < 4) || (bits_per_word > 16))
+		return -EINVAL;
+
+	if (!hz)
+		hz = spi->max_speed_hz;
+
+	cs->rx_shift = 0;
+	cs->tx_shift = 0;
+	cs->get_rx = mpc8xxx_spi_rx_buf_u32;
+	cs->get_tx = mpc8xxx_spi_tx_buf_u32;
+	if (bits_per_word <= 8) {
+		cs->rx_shift = 8 - bits_per_word;
+	} else if (bits_per_word <= 16) {
+		cs->rx_shift = 16 - bits_per_word;
+		if (spi->mode & SPI_LSB_FIRST)
+			cs->get_tx = fsl_espi_tx_buf_lsb;
+	} else
+		return -EINVAL;

} else {
}

+
+	mpc8xxx_spi->rx_shift = cs->rx_shift;
+	mpc8xxx_spi->tx_shift = cs->tx_shift;
+	mpc8xxx_spi->get_rx = cs->get_rx;
+	mpc8xxx_spi->get_tx = cs->get_tx;
+
+	bits_per_word = bits_per_word - 1;
+
+	/* mask out bits we are going to set */
+	cs->hw_mode &= ~(CSMODE_LEN(0xF) | CSMODE_DIV16
+				  | CSMODE_PM(0xF));

No need to break this statement.

+
+	cs->hw_mode |= CSMODE_LEN(bits_per_word);
+
+	if ((mpc8xxx_spi->spibrg / hz) > 64) {
+		cs->hw_mode |= CSMODE_DIV16;
+		pm = (mpc8xxx_spi->spibrg - 1) / (hz * 64) + 1;
+
+		WARN_ONCE(pm > 16, "%s: Requested speed is too low: %d Hz. "
+			  "Will use %d Hz instead.\n", dev_name(&spi->dev),
+			  hz, mpc8xxx_spi->spibrg / 1024);
+		if (pm > 16)
+			pm = 16;
+	} else {
+		pm = (mpc8xxx_spi->spibrg - 1) / (hz * 4) + 1;
+	}
+	if (pm)
+		pm--;
+
+	cs->hw_mode |= CSMODE_PM(pm);
+
+	fsl_espi_change_mode(spi);
+	return 0;
+}
+
+int fsl_espi_cpu_bufs(struct mpc8xxx_spi *mspi, struct spi_transfer *t,
+		unsigned int len)

Does this need to be global?

+{
+	u32 word;
+	struct fsl_espi_reg *reg_base = (struct fsl_espi_reg *)mspi->reg_base;
+
+	mspi->count = len;
+
+	/* enable rx ints */
+	mpc8xxx_spi_write_reg(&reg_base->mask, SPIM_NE);
+
+	/* transmit word */
+	word = mspi->get_tx(mspi);
+	mpc8xxx_spi_write_reg(&reg_base->transmit, word);
+
+	return 0;
+}
+
+static int fsl_espi_bufs(struct spi_device *spi, struct spi_transfer *t,
+			    bool is_dma_mapped)

No need for the is_dma_mapped argument.

+{
+	struct mpc8xxx_spi *mpc8xxx_spi = spi_master_get_devdata(spi->master);
+	struct fsl_espi_reg *reg_base;
+	unsigned int len = t->len;
+	u8 bits_per_word;
+	int ret;
+
+	reg_base = (struct fsl_espi_reg *)mpc8xxx_spi->reg_base;

Better write this as an initializer, no need for the cast.

+
+	bits_per_word = spi->bits_per_word;
+	if (t->bits_per_word)
+		bits_per_word = t->bits_per_word;

[...]

+static int fsl_espi_setup(struct spi_device *spi)
+{
+	struct mpc8xxx_spi *mpc8xxx_spi;
+	struct fsl_espi_reg *reg_base;
+	int retval;
+	u32 hw_mode;
+	u32 loop_mode;
+	struct spi_mpc8xxx_cs	*cs = spi->controller_state;

Stray tab.

+
+	if (!spi->max_speed_hz)
+		return -EINVAL;
+
+	if (!cs) {
+		cs = kzalloc(sizeof *cs, GFP_KERNEL);
+		if (!cs)
+			return -ENOMEM;
+		spi->controller_state = cs;
+	}

[...]

+		rx_data = mpc8xxx_spi_read_reg(&reg_base->receive);
+
+		if (mspi->rx)
+			mspi->get_rx(rx_data, mspi);
+	}
+
+	if ((events & SPIE_NF) == 0)

if (!(events & bit)) is a bit more more natural. Also, the if
statement here needs braces.

+		/* spin until TX is done */
+		while (((events = mpc8xxx_spi_read_reg(&reg_base->event))
+					& SPIE_NF) == 0)
+			cpu_relax();

This is dangerous. There's a handy spin_event_timeout()
in asm/delay.h.

+
+	/* Clear the events */
+	mpc8xxx_spi_write_reg(&reg_base->event, events);
+
+	mspi->count -= 1;
+	if (mspi->count) {
+		u32 word = mspi->get_tx(mspi);
+
+		mpc8xxx_spi_write_reg(&reg_base->transmit, word);
+	} else {
+		complete(&mspi->done);
+	}
+}
+
+static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
+{
+	struct mpc8xxx_spi *mspi = context_data;
+	struct fsl_espi_reg *reg_base = (struct fsl_espi_reg *)mspi->reg_base;
+	irqreturn_t ret = IRQ_NONE;
+	u32 events;
+
+	/* Get interrupt events(tx/rx) */
+	events = mpc8xxx_spi_read_reg(&reg_base->event);
+	if (events)
+		ret = IRQ_HANDLED;
+
+	dev_dbg(mspi->dev, "%s: events %x\n", __func__, events);

dev_vdbg()

+
+	fsl_espi_cpu_irq(mspi, events);
+
+	return ret;
+}
+
+static void fsl_espi_remove(struct mpc8xxx_spi *mspi)
+{
+	iounmap(mspi->reg_base);
+}
+
+static struct spi_master * __devinit fsl_espi_probe(struct device *dev,
+		struct resource *mem, unsigned int irq)
+{
+	struct fsl_spi_platform_data *pdata = dev->platform_data;
+	struct spi_master *master;
+	struct mpc8xxx_spi *mpc8xxx_spi;
+	struct fsl_espi_reg *reg_base;
+	u32 regval;
+	int i, ret = 0;
+
+	master = spi_alloc_master(dev, sizeof(struct mpc8xxx_spi));
+	if (master == NULL) {

Sometimes you check for !allocated, and sometimes allocated == NULL.
Be consistent. (And !allocated is more natural.)

+		ret = -ENOMEM;
+		goto err;
+	}
+
+	dev_set_drvdata(dev, master);
+
+	ret = mpc8xxx_spi_probe(dev, mem, irq);
+	if (ret)
+		goto err_probe;
+
+	master->setup = fsl_espi_setup;
+
+	mpc8xxx_spi = spi_master_get_devdata(master);
+	mpc8xxx_spi->spi_do_one_msg = fsl_espi_do_one_msg;
+	mpc8xxx_spi->spi_remove = fsl_espi_remove;
+
+	mpc8xxx_spi->reg_base = ioremap(mem->start, resource_size(mem));
+	if (mpc8xxx_spi->reg_base == NULL) {

Ditto.

+		ret = -ENOMEM;
+		goto err_probe;
+	}
+
+	reg_base = (struct fsl_espi_reg *)mpc8xxx_spi->reg_base;
+
+	/* Register for SPI Interrupt */
+	ret = request_irq(mpc8xxx_spi->irq, fsl_espi_irq,
+			  0, "fsl_espi", mpc8xxx_spi);
+
+	if (ret != 0)

Every time someone writes 'if (rc != 0)', a kitty dies.
Simple 'if (rc)' saves kittens.

+		goto free_irq;
+
+	if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
+		mpc8xxx_spi->rx_shift = 16;
+		mpc8xxx_spi->tx_shift = 24;
+	}

[...]

+
+static int __devexit of_fsl_espi_remove(struct platform_device *dev)
+{
+	int ret;
+
+	ret = mpc8xxx_spi_remove(&dev->dev);
+	if (ret)
+		return ret;
+
+	return 0;

Just 'return mpc8xxx_spi_remove(&dev->dev);' is sufficient.

Also, I think there's no need for this wrapper nowadays (but
splitting OF and real probe() stuff is still appropriate).

+}
+
+static const struct of_device_id of_fsl_espi_match[] = {
+	{ .compatible = "fsl,mpc8536-espi" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_fsl_espi_match);
+
+static struct of_platform_driver fsl_espi_driver = {
+	.driver = {
+		.name = "fsl_espi",
+		.owner = THIS_MODULE,
+		.of_match_table = of_fsl_espi_match,
+	},
+	.probe		= of_fsl_espi_probe,
+	.remove		= __devexit_p(of_fsl_espi_remove),
+};
+
+static int __init fsl_espi_init(void)
+{
+	return of_register_platform_driver(&fsl_espi_driver);
+}
+module_init(fsl_espi_init);
+
+static void __exit fsl_espi_exit(void)
+{
+	of_unregister_platform_driver(&fsl_espi_driver);
+}
+module_exit(fsl_espi_exit);
+
+MODULE_AUTHOR("Mingkai Hu");
+MODULE_DESCRIPTION("Enhanced Freescale SPI Driver");

This sounds like that this is an enhanced version of the
Freescale SPI driver, which it is not. ;-)

I'd place the #ifdef CONFIG_SPI_ESPI, for documentation purposes.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2