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 = ®_base->mode;
> + mode = ®_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(®_base->mask, SPIM_NE);
> +
> + /* transmit word */
> + word = mspi->get_tx(mspi);
> + mpc8xxx_spi_write_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(®_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(®_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(®_base->event, events);
> +
> + mspi->count -= 1;
> + if (mspi->count) {
> + u32 word = mspi->get_tx(mspi);
> +
> + mpc8xxx_spi_write_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(®_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. ;-)
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/spi/spi_fsl_lib.h b/drivers/spi/spi_fsl_lib.h
> index 6ae8949..9c81498 100644
> --- a/drivers/spi/spi_fsl_lib.h
> +++ b/drivers/spi/spi_fsl_lib.h
> @@ -26,6 +26,7 @@ struct mpc8xxx_spi {
> /* rx & tx bufs from the spi_transfer */
> const void *tx;
> void *rx;
> + int len;
I'd place the #ifdef CONFIG_SPI_ESPI, for documentation purposes.
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
