Re: [PATCH] SPI: MIPS: lantiq: add FALC-ON spi driver

[Wolfram Sang <w.sang@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 8950 bytes --]

On Wed, Nov 16, 2011 at 03:37:17PM +0100, John Crispin wrote:
> The external bus unit (EBU) found on the FALC-ON SoC has spi emulation that is
> designed for serial flash access. This driver has only been tested with m25p80
> type chips. The hardware has no support for other types of spi peripherals.
> 
> Signed-off-by: Thomas Langer <thomas.langer@lantiq.com>
> Signed-off-by: John Crispin <blogic@openwrt.org>
> Cc: spi-devel-general@lists.sourceforge.net

Rough review.

> ---
> 
>  arch/mips/lantiq/falcon/devices.c        |   13 +
>  arch/mips/lantiq/falcon/devices.h        |    4 +
>  arch/mips/lantiq/falcon/mach-easy98000.c |   27 ++
>  drivers/spi/Kconfig                      |    4 +
>  drivers/spi/Makefile                     |    1 +
>  drivers/spi/spi-falcon.c                 |  483 ++++++++++++++++++++++++++++++
>  6 files changed, 532 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/spi/spi-falcon.c

You need to split up the arch-specific changes and driver-specific
changes into seperate patches.

> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index a1fd73d..f244553 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -180,6 +180,10 @@ config SPI_MPC52xx
>  	  This drivers supports the MPC52xx SPI controller in master SPI
>  	  mode.
>  
> +config SPI_FALCON
> +	tristate "Falcon SPI controller support"
> +	depends on SOC_FALCON
> +

Kconfig is alphabetically sorted. Please keep that.

>  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 61c3261..570894c 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_SPI_DW_MMIO)		+= spi-dw-mmio.o
>  obj-$(CONFIG_SPI_DW_PCI)		+= spi-dw-midpci.o
>  spi-dw-midpci-objs			:= spi-dw-pci.o spi-dw-mid.o
>  obj-$(CONFIG_SPI_EP93XX)		+= spi-ep93xx.o
> +obj-$(CONFIG_SPI_FALCON)		+= spi-falcon.o
>  obj-$(CONFIG_SPI_FSL_LIB)		+= spi-fsl-lib.o
>  obj-$(CONFIG_SPI_FSL_ESPI)		+= spi-fsl-espi.o
>  obj-$(CONFIG_SPI_FSL_SPI)		+= spi-fsl-spi.o
> diff --git a/drivers/spi/spi-falcon.c b/drivers/spi/spi-falcon.c
> new file mode 100644
> index 0000000..8b81aa2
> --- /dev/null
> +++ b/drivers/spi/spi-falcon.c
> @@ -0,0 +1,483 @@
> +/*
> + *  This program is free software; you can redistribute it and/or modify it
> + *  under the terms of the GNU General Public License version 2 as published
> + *  by the Free Software Foundation.
> + *
> + *  Copyright (C) 2010 Thomas Langer <thomas.langer@lantiq.com>
> + */
> +

...

> +int
> +falcon_spi_xfer(struct spi_device *spi,
> +		    struct spi_transfer *t,
> +		    unsigned long flags)

I'd prefer to squash the lines, dunno Grant's preference.

> +{
> +	struct device *dev = &spi->dev;
> +	struct falcon_spi *priv = spi_master_get_devdata(spi->master);
> +	const u8 *txp = t->tx_buf;
> +	u8 *rxp = t->rx_buf;
> +	unsigned int bytelen = ((8 * t->len + 7) / 8);
> +	unsigned int len, alen, dumlen;
> +	u32 val;
> +	enum {
> +		state_init,
> +		state_command_prepare,
> +		state_write,
> +		state_read,
> +		state_disable_cs,
> +		state_end
> +	} state = state_init;
> +
> +	do {
> +		switch (state) {
> +		case state_init: /* detect phase of upper layer sequence */
> +		{
> +			/* initial write ? */
> +			if (flags & FALCON_SPI_XFER_BEGIN) {
> +				if (!txp) {
> +					dev_err(dev,
> +						"BEGIN without tx data!\n");
> +					return -1;

-Esomething perhaps (for all occurences)?

> +				}
> +				/*
> +				 * Prepare the parts of the sfcmd register,
> +				 * which should not
> +				 * change during a sequence!
> +				 * Only exception are the length fields,
> +				 * especially alen and dumlen.
> +				 */

I think some lines could be squashed again.

> +
> +				priv->sfcmd = ((spi->chip_select
> +						<< SFCMD_CS_OFFSET)
> +					       & SFCMD_CS_MASK);
> +				priv->sfcmd |= SFCMD_KEEP_CS_KEEP_SELECTED;
> +				priv->sfcmd |= *txp;
> +				txp++;
> +				bytelen--;
> +				if (bytelen) {
> +					/*
> +					 * more data:
> +					 * maybe address and/or dummy
> +					 */
> +					state = state_command_prepare;
> +					break;
> +				} else {
> +					dev_dbg(dev, "write cmd %02X\n",
> +						priv->sfcmd & SFCMD_OPC_MASK);
> +				}
> +			}
> +			/* continued write ? */
> +			if (txp && bytelen) {
> +				state = state_write;
> +				break;
> +			}
> +			/* read data? */
> +			if (rxp && bytelen) {
> +				state = state_read;
> +				break;
> +			}
> +			/* end of sequence? */
> +			if (flags & FALCON_SPI_XFER_END)
> +				state = state_disable_cs;
> +			else
> +				state = state_end;
> +			break;
> +		}
> +		/* collect tx data for address and dummy phase */
> +		case state_command_prepare:
> +		{
> +			/* txp is valid, already checked */
> +			val = 0;
> +			alen = 0;
> +			dumlen = 0;
> +			while (bytelen > 0) {
> +				if (alen < 3) {
> +					val = (val<<8)|(*txp++);

Spaces around operators.

> +					alen++;
> +				} else if ((dumlen < 15) && (*txp == 0)) {
> +					/*
> +					 * assume dummy bytes are set to 0
> +					 * from upper layer
> +					 */
> +					dumlen++;
> +					txp++;
> +				} else
> +					break;
> +				bytelen--;
> +			}

...

> +static int
> +falcon_spi_setup(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	const u32 ebuclk = CLOCK_100M;
> +	unsigned int i;
> +	unsigned long flags;
> +
> +	dev_dbg(dev, "setup\n");

I'd think these debug outputs at the beginning of functions could simply
go.

> +
> +	if (spi->master->bus_num > 0 || spi->chip_select > 0)
> +		return -ENODEV;
> +

...

> +static int
> +falcon_spi_transfer(struct spi_device *spi, struct spi_message *m)
> +{
> +	struct falcon_spi *priv = spi_master_get_devdata(spi->master);
> +	struct spi_transfer *t;
> +	unsigned long spi_flags;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	priv->sfcmd = 0;
> +	m->actual_length = 0;
> +
> +	spi_flags = FALCON_SPI_XFER_BEGIN;
> +	list_for_each_entry(t, &m->transfers, transfer_list) {
> +		if (list_is_last(&t->transfer_list, &m->transfers))
> +			spi_flags |= FALCON_SPI_XFER_END;
> +
> +		spin_lock_irqsave(&ebu_lock, flags);
> +		ret = falcon_spi_xfer(spi, t, spi_flags);
> +		spin_unlock_irqrestore(&ebu_lock, flags);
> +
> +		if (ret)
> +			break;
> +
> +		m->actual_length += t->len;
> +
> +		if (t->delay_usecs || t->cs_change)
> +			BUG();

No garceful way of handling that?

> +
> +		spi_flags = 0;
> +	}
> +
> +	m->status = ret;
> +	m->complete(m->context);
> +
> +	return 0;
> +}
> +
> +static void
> +falcon_spi_cleanup(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +
> +	dev_dbg(dev, "cleanup\n");
> +}
> +
> +static int __devinit
> +falcon_spi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct falcon_spi *priv;
> +	struct spi_master *master;
> +	int ret;
> +
> +	dev_dbg(dev, "probing\n");
> +
> +	master = spi_alloc_master(&pdev->dev, sizeof(*priv));
> +	if (!master) {
> +		dev_err(dev, "no memory for spi_master\n");

I think -ENOMEM tells enough.

> +		return -ENOMEM;
> +	}
> +
> +	priv = spi_master_get_devdata(master);
> +	priv->master = master;
> +
> +	master->mode_bits = SPI_MODE_3;
> +	master->num_chipselect = 1;
> +	master->bus_num = 0;
> +
> +	master->setup = falcon_spi_setup;
> +	master->transfer = falcon_spi_transfer;
> +	master->cleanup = falcon_spi_cleanup;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = spi_register_master(master);
> +	if (ret)
> +		spi_master_put(master);
> +
> +	return ret;
> +}
> +
> +static int __devexit
> +falcon_spi_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct falcon_spi *priv = platform_get_drvdata(pdev);
> +
> +	dev_dbg(dev, "removed\n");
> +
> +	spi_unregister_master(priv->master);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver falcon_spi_driver = {
> +	.probe	= falcon_spi_probe,
> +	.remove	= __devexit_p(falcon_spi_remove),
> +	.driver = {
> +		.name	= DRV_NAME,
> +		.owner	= THIS_MODULE
> +	}
> +};

Use module_platform_driver here.

> +
> +static int __init
> +falcon_spi_init(void)
> +{
> +	return platform_driver_register(&falcon_spi_driver);
> +}
> +
> +static void __exit
> +falcon_spi_exit(void)
> +{
> +	platform_driver_unregister(&falcon_spi_driver);
> +}
> +
> +module_init(falcon_spi_init);
> +module_exit(falcon_spi_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Lantiq Falcon SPI controller driver");

Thanks,

   Wolfram

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]