[PATCH 2/2] SPI: make the moved loopback test work

[PATCH 2/2] SPI: make the moved loopback test work

[Linus Walleij]

This completes the move of the SPI test chip out of the U300
architecture by modifying the loopback test to register itself on
every PL022 master if it is compiled in. Also updates some minor
things like naming to make the code stricter.

Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>
---
 arch/arm/mach-u300/Kconfig        |   12 ----
 arch/arm/mach-u300/Makefile       |    1 -
 arch/arm/mach-u300/spi.c          |   61 -------------------
 drivers/spi/Kconfig               |   13 ++++
 drivers/spi/Makefile              |    1 +
 drivers/spi/amba-pl022-looptest.c |  120 +++++++++++++++++++++++++++++-------
 drivers/spi/amba-pl022-looptest.h |   11 ++++
 drivers/spi/amba-pl022.c          |    6 ++
 8 files changed, 127 insertions(+), 98 deletions(-)
 create mode 100644 drivers/spi/amba-pl022-looptest.h

diff --git a/arch/arm/mach-u300/Kconfig b/arch/arm/mach-u300/Kconfig
index 801b21e..337b9aa 100644
--- a/arch/arm/mach-u300/Kconfig
+++ b/arch/arm/mach-u300/Kconfig
@@ -81,18 +81,6 @@ config MACH_U300_SEMI_IS_SHARED
 		Memory Interface) from both from access and application
 		side.
 
-config MACH_U300_SPIDUMMY
-	bool "SSP/SPI dummy chip"
-	select SPI
-	select SPI_MASTER
-	select SPI_PL022
-	help
-		This creates a small kernel module that creates a dummy
-		SPI device to be used for loopback tests. Regularly used
-		to test reference designs. If you're not testing SPI,
-		you don't need it. Selecting this will activate the
-		SPI framework and ARM PL022 support.
-
 comment "All the settings below must match the bootloader's settings"
 
 config MACH_U300_ACCESS_MEM_SIZE
diff --git a/arch/arm/mach-u300/Makefile b/arch/arm/mach-u300/Makefile
index fab46fe..1f0f4b8 100644
--- a/arch/arm/mach-u300/Makefile
+++ b/arch/arm/mach-u300/Makefile
@@ -10,6 +10,5 @@ obj-		:=
 obj-$(CONFIG_ARCH_U300)	          += u300.o
 obj-$(CONFIG_MMC)                 += mmc.o
 obj-$(CONFIG_SPI_PL022)           += spi.o
-obj-$(CONFIG_MACH_U300_SPIDUMMY)  += dummyspichip.o
 obj-$(CONFIG_I2C_STU300)          += i2c.o
 obj-$(CONFIG_REGULATOR_AB3100)    += regulator.o
diff --git a/arch/arm/mach-u300/spi.c b/arch/arm/mach-u300/spi.c
index f0e887b..0790cce 100644
--- a/arch/arm/mach-u300/spi.c
+++ b/arch/arm/mach-u300/spi.c
@@ -16,68 +16,8 @@
 /*
  * The following is for the actual devices on the SSP/SPI bus
  */
-#ifdef CONFIG_MACH_U300_SPIDUMMY
-static void select_dummy_chip(u32 chipselect)
-{
-	pr_debug("CORE: %s called with CS=0x%x (%s)\n",
-		 __func__,
-		 chipselect,
-		 chipselect ? "unselect chip" : "select chip");
-	/*
-	 * Here you would write the chip select value to the GPIO pins if
-	 * this was a real chip (but this is a loopback dummy).
-	 */
-}
-
-struct pl022_config_chip dummy_chip_info = {
-	/* Nominally this is LOOPBACK_DISABLED, but this is our dummy chip! */
-	.lbm = LOOPBACK_ENABLED,
-	/*
-	 * available POLLING_TRANSFER and INTERRUPT_TRANSFER,
-	 * DMA_TRANSFER does not work
-	 */
-	.com_mode = INTERRUPT_TRANSFER,
-	.iface = SSP_INTERFACE_MOTOROLA_SPI,
-	/* We can only act as master but SSP_SLAVE is possible in theory */
-	.hierarchy = SSP_MASTER,
-	/* 0 = drive TX even as slave, 1 = do not drive TX as slave */
-	.slave_tx_disable = 0,
-	/* LSB first */
-	.endian_tx = SSP_TX_LSB,
-	.endian_rx = SSP_RX_LSB,
-	.data_size = SSP_DATA_BITS_8, /* used to be 12 in some default */
-	.rx_lev_trig = SSP_RX_1_OR_MORE_ELEM,
-	.tx_lev_trig = SSP_TX_1_OR_MORE_EMPTY_LOC,
-	.clk_phase = SSP_CLK_SECOND_EDGE,
-	.clk_pol = SSP_CLK_POL_IDLE_LOW,
-	.ctrl_len = SSP_BITS_12,
-	.wait_state = SSP_MWIRE_WAIT_ZERO,
-	.duplex = SSP_MICROWIRE_CHANNEL_FULL_DUPLEX,
-	/*
-	 * This is where you insert a call to a function to enable CS
-	 * (usually GPIO) for a certain chip.
-	 */
-	.cs_control = select_dummy_chip,
-};
-#endif
 
 static struct spi_board_info u300_spi_devices[] = {
-#ifdef CONFIG_MACH_U300_SPIDUMMY
-	{
-		/* A dummy chip used for loopback tests */
-		.modalias       = "spi-dummy",
-		/* Really dummy, pass in additional chip config here */
-		.platform_data  = NULL,
-		/* This defines how the controller shall handle the device */
-		.controller_data = &dummy_chip_info,
-		/* .irq - no external IRQ routed from this device */
-		.max_speed_hz   = 1000000,
-		.bus_num        = 0, /* Only one bus on this chip */
-		.chip_select    = 0,
-		/* Means SPI_CS_HIGH, change if e.g low CS */
-		.mode           = 0,
-	},
-#endif
 };
 
 static struct pl022_ssp_controller ssp_platform_data = {
@@ -94,7 +34,6 @@ static struct pl022_ssp_controller ssp_platform_data = {
 	.num_chipselect = 3,
 };
 
-
 void __init u300_spi_init(struct amba_device *adev)
 {
 	struct pmx *pmx;
diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 91c2f4f..9e584dc 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -227,6 +227,19 @@ config SPI_PL022
 	  controller. If you have an embedded system with an AMBA(R)
 	  bus and a PL022 controller, say Y or M here.
 
+config SPI_PL022_LOOPTEST
+	bool "PL022 loopback chip"
+	depends on SPI_PL022
+	help
+	  The PL022 supports a loopback mode where the output is
+	  looped back to the input for testing.
+
+	  This creates a small kernel module that creates a dummy
+	  SPI device on each PL022 SPI bus to be used for loopback
+	  tests. You find the device in the /sys filesystem, e.g.
+	  /sys/bus/spi/devices/spi0.0/looptest and the test is run by
+	  issuing cat <looptest>.
+
 config SPI_PPC4xx
 	tristate "PPC4xx SPI Controller"
 	depends on PPC32 && 4xx && SPI_MASTER
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index e9cbd18..9ac8e95 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_SPI_OMAP24XX)		+= omap2_mcspi.o
 obj-$(CONFIG_SPI_OMAP_100K)		+= omap_spi_100k.o
 obj-$(CONFIG_SPI_ORION)			+= orion_spi.o
 obj-$(CONFIG_SPI_PL022)			+= amba-pl022.o
+obj-$(CONFIG_SPI_PL022_LOOPTEST)	+= amba-pl022-looptest.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= mpc512x_psc_spi.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= mpc52xx_psc_spi.o
 obj-$(CONFIG_SPI_MPC52xx)		+= mpc52xx_spi.o
diff --git a/drivers/spi/amba-pl022-looptest.c b/drivers/spi/amba-pl022-looptest.c
index 5f55012..561886a 100644
--- a/drivers/spi/amba-pl022-looptest.c
+++ b/drivers/spi/amba-pl022-looptest.c
@@ -1,7 +1,5 @@
 /*
- * arch/arm/mach-u300/dummyspichip.c
- *
- * Copyright (C) 2007-2009 ST-Ericsson AB
+ * Copyright (C) 2007-2010 ST-Ericsson SA
  * License terms: GNU General Public License (GPL) version 2
  * This is a dummy loopback SPI "chip" used for testing SPI.
  * Author: Linus Walleij <linus.walleij@stericsson.com>
@@ -25,19 +23,93 @@
  */
 #include <linux/amba/pl022.h>
 
-struct dummy {
+struct pl022_loopback {
 	struct device *dev;
 	struct mutex lock;
 };
 
 #define DMA_TEST_SIZE 2048
 
-/* When we cat /sys/bus/spi/devices/spi0.0/looptest this will be triggered */
-static ssize_t dummy_looptest(struct device *dev,
+/*
+ * The following code self-registers the PL022 loopback
+ * chip on each PL022 SPI bus.
+ */
+
+static void pl022_select_loopback_chip(u32 chipselect)
+{
+	pr_debug("PL022 loopback chip: %s called with CS=0x%x (%s)\n",
+		 __func__,
+		 chipselect,
+		 chipselect ? "unselect chip" : "select chip");
+	/*
+	 * Here you would write the chip select value to the GPIO pins if
+	 * this was a real chip (but this is a loopback dummy).
+	 */
+}
+
+struct pl022_config_chip pl022_loopback_chip_info = {
+	/* Nominally this is LOOPBACK_DISABLED, but this is our dummy chip! */
+	.lbm = LOOPBACK_ENABLED,
+	.com_mode = INTERRUPT_TRANSFER,
+	.iface = SSP_INTERFACE_MOTOROLA_SPI,
+	.hierarchy = SSP_MASTER,
+	.slave_tx_disable = 0,
+	/* LSB first */
+	.endian_tx = SSP_TX_LSB,
+	.endian_rx = SSP_RX_LSB,
+	.data_size = SSP_DATA_BITS_8,
+	.rx_lev_trig = SSP_RX_1_OR_MORE_ELEM,
+	.tx_lev_trig = SSP_TX_1_OR_MORE_EMPTY_LOC,
+	.clk_phase = SSP_CLK_SECOND_EDGE,
+	.clk_pol = SSP_CLK_POL_IDLE_LOW,
+	.ctrl_len = SSP_BITS_12,
+	.wait_state = SSP_MWIRE_WAIT_ZERO,
+	.duplex = SSP_MICROWIRE_CHANNEL_FULL_DUPLEX,
+	.cs_control = pl022_select_loopback_chip,
+};
+
+static struct spi_board_info pl022_loopback_device = {
+	/* A dummy chip used for loopback tests */
+	.modalias       = "pl022-loop",
+	/* Really dummy, pass in additional chip config here */
+	.platform_data  = NULL,
+	/* This defines how the controller shall handle the device */
+	.controller_data = &pl022_loopback_chip_info,
+	/* .irq - no external IRQ routed from this device */
+	.max_speed_hz   = 1000000,
+	.bus_num        = 0, /* Only one bus on this chip */
+	.chip_select    = 0,
+	/* Means SPI_CS_HIGH, change if e.g low CS */
+	.mode           = 0,
+};
+
+/*
+ * This is called from the PL022 driver to register a loopback
+ * on itself if this module is compiled in.
+ */
+void pl022_loopback_register(struct spi_master *master)
+{
+	struct spi_device *spidev;
+
+	spidev = spi_new_device(master, &pl022_loopback_device);
+	if (!spidev)
+		dev_err(&master->dev,
+			"could not register loopback test chip\n");
+	else
+		dev_err(&master->dev,
+			"registered loopback test chip\n");
+}
+
+/*
+ * This is where the tests begin. When we issue:
+ * cat /sys/bus/spi/devices/spi0.0/looptest
+ * this will be triggered.
+ */
+static ssize_t pl022_looptest(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct spi_device *spi = to_spi_device(dev);
-	struct dummy *p_dummy = dev_get_drvdata(&spi->dev);
+	struct pl022_loopback *p_loop = dev_get_drvdata(&spi->dev);
 
 	/*
 	 * WARNING! Do not dereference the chip-specific data in any normal
@@ -55,7 +127,7 @@ static ssize_t dummy_looptest(struct device *dev,
 	u8 *bigtxbuf_virtual;
 	u8 *bigrxbuf_virtual;
 
-	if (mutex_lock_interruptible(&p_dummy->lock))
+	if (mutex_lock_interruptible(&p_loop->lock))
 		return -ERESTARTSYS;
 
 	bigtxbuf_virtual = kmalloc(DMA_TEST_SIZE, GFP_KERNEL);
@@ -217,25 +289,25 @@ static ssize_t dummy_looptest(struct device *dev,
 	kfree(bigrxbuf_virtual);
 	kfree(bigtxbuf_virtual);
  out:
-	mutex_unlock(&p_dummy->lock);
+	mutex_unlock(&p_loop->lock);
 	return status;
 }
 
-static DEVICE_ATTR(looptest, S_IRUGO, dummy_looptest, NULL);
+static DEVICE_ATTR(looptest, S_IRUGO, pl022_looptest, NULL);
 
-static int __devinit pl022_dummy_probe(struct spi_device *spi)
+static int __devinit pl022_loopback_probe(struct spi_device *spi)
 {
-	struct dummy *p_dummy;
+	struct pl022_loopback *p_loop;
 	int status;
 
 	dev_info(&spi->dev, "probing dummy SPI device\n");
 
-	p_dummy = kzalloc(sizeof *p_dummy, GFP_KERNEL);
-	if (!p_dummy)
+	p_loop = kzalloc(sizeof *p_loop, GFP_KERNEL);
+	if (!p_loop)
 		return -ENOMEM;
 
-	dev_set_drvdata(&spi->dev, p_dummy);
-	mutex_init(&p_dummy->lock);
+	dev_set_drvdata(&spi->dev, p_loop);
+	mutex_init(&p_loop->lock);
 
 	/* sysfs hook */
 	status = device_create_file(&spi->dev, &dev_attr_looptest);
@@ -248,29 +320,29 @@ static int __devinit pl022_dummy_probe(struct spi_device *spi)
 
 out_dev_create_looptest_failed:
 	dev_set_drvdata(&spi->dev, NULL);
-	kfree(p_dummy);
+	kfree(p_loop);
 	return status;
 }
 
-static int __devexit pl022_dummy_remove(struct spi_device *spi)
+static int __devexit pl022_loopback_remove(struct spi_device *spi)
 {
-	struct dummy *p_dummy = dev_get_drvdata(&spi->dev);
+	struct pl022_loopback *p_loop = dev_get_drvdata(&spi->dev);
 
 	dev_info(&spi->dev, "removing dummy SPI device\n");
 	device_remove_file(&spi->dev, &dev_attr_looptest);
 	dev_set_drvdata(&spi->dev, NULL);
-	kfree(p_dummy);
+	kfree(p_loop);
 
 	return 0;
 }
 
 static struct spi_driver pl022_dummy_driver = {
 	.driver = {
-		.name	= "spi-dummy",
+		.name	= "pl022-loop",
 		.owner	= THIS_MODULE,
 	},
-	.probe	= pl022_dummy_probe,
-	.remove	= __devexit_p(pl022_dummy_remove),
+	.probe	= pl022_loopback_probe,
+	.remove	= __devexit_p(pl022_loopback_remove),
 };
 
 static int __init pl022_init_dummy(void)
@@ -287,5 +359,5 @@ module_init(pl022_init_dummy);
 module_exit(pl022_exit_dummy);
 
 MODULE_AUTHOR("Linus Walleij <linus.walleij@stericsson.com>");
-MODULE_DESCRIPTION("PL022 SSP/SPI DUMMY Linux driver");
+MODULE_DESCRIPTION("PL022 loopback test driver");
 MODULE_LICENSE("GPL");
diff --git a/drivers/spi/amba-pl022-looptest.h b/drivers/spi/amba-pl022-looptest.h
new file mode 100644
index 0000000..1bedcc6
--- /dev/null
+++ b/drivers/spi/amba-pl022-looptest.h
@@ -0,0 +1,11 @@
+/*
+ * Defines a single function for the master to create a loopback
+ * on itself.
+ */
+#ifdef CONFIG_SPI_PL022_LOOPTEST
+void pl022_loopback_register(struct spi_master *master);
+#else
+static void inline pl022_loopback_register(struct spi_master *master)
+{
+}
+#endif
diff --git a/drivers/spi/amba-pl022.c b/drivers/spi/amba-pl022.c
index f0a1418..6e4a657 100644
--- a/drivers/spi/amba-pl022.c
+++ b/drivers/spi/amba-pl022.c
@@ -46,6 +46,8 @@
 #include <linux/io.h>
 #include <linux/slab.h>
 
+#include "amba-pl022-looptest.h"
+
 /*
  * This macro is used to define some register default values.
  * reg is masked with mask, the OR:ed with an (again masked)
@@ -1818,6 +1820,10 @@ pl022_probe(struct amba_device *adev, struct amba_id *id)
 		goto err_spi_register;
 	}
 	dev_dbg(dev, "probe succeded\n");
+
+	/* Register a loopback test if desired */
+	pl022_loopback_register(master);
+
 	return 0;
 
  err_spi_register:
-- 
1.7.1

Re: [PATCH 2/2] SPI: make the moved loopback test work

[Grant Likely]

On Sat, Jul 10, 2010 at 6:43 PM, Linus Walleij
<linus.walleij@stericsson.com> wrote:
> This completes the move of the SPI test chip out of the U300
> architecture by modifying the loopback test to register itself on
> every PL022 master if it is compiled in. Also updates some minor
> things like naming to make the code stricter.
>
> Signed-off-by: Linus Walleij <linus.walleij@stericsson.com>

Having a test driver in common code that purposefully does bad things
is a sure fire way to get people to copy those bad practices when they
write their own drivers (despite all the comments in the code that say
don't do this).  I'm talking about things like the dereferencing of
controller data, and calling the setup() hook in something that looks
like a normal spi_device driver.  As-is, I'm not excited about merging
this patch (and it isn't bisectable either).

If you want this loopback functionality, It should be integrated into
the pl022 driver itself.  It should not pretend to be an spi_device.

> ---
>  arch/arm/mach-u300/Kconfig        |   12 ----
>  arch/arm/mach-u300/Makefile       |    1 -
>  arch/arm/mach-u300/spi.c          |   61 -------------------
>  drivers/spi/Kconfig               |   13 ++++
>  drivers/spi/Makefile              |    1 +
>  drivers/spi/amba-pl022-looptest.c |  120 +++++++++++++++++++++++++++++-------
>  drivers/spi/amba-pl022-looptest.h |   11 ++++
>  drivers/spi/amba-pl022.c          |    6 ++
>  8 files changed, 127 insertions(+), 98 deletions(-)
>  create mode 100644 drivers/spi/amba-pl022-looptest.h
>
> diff --git a/arch/arm/mach-u300/Kconfig b/arch/arm/mach-u300/Kconfig
> index 801b21e..337b9aa 100644
> --- a/arch/arm/mach-u300/Kconfig
> +++ b/arch/arm/mach-u300/Kconfig
> @@ -81,18 +81,6 @@ config MACH_U300_SEMI_IS_SHARED
>                Memory Interface) from both from access and application
>                side.
>
> -config MACH_U300_SPIDUMMY
> -       bool "SSP/SPI dummy chip"
> -       select SPI
> -       select SPI_MASTER
> -       select SPI_PL022
> -       help
> -               This creates a small kernel module that creates a dummy
> -               SPI device to be used for loopback tests. Regularly used
> -               to test reference designs. If you're not testing SPI,
> -               you don't need it. Selecting this will activate the
> -               SPI framework and ARM PL022 support.
> -
>  comment "All the settings below must match the bootloader's settings"
>
>  config MACH_U300_ACCESS_MEM_SIZE
> diff --git a/arch/arm/mach-u300/Makefile b/arch/arm/mach-u300/Makefile
> index fab46fe..1f0f4b8 100644
> --- a/arch/arm/mach-u300/Makefile
> +++ b/arch/arm/mach-u300/Makefile
> @@ -10,6 +10,5 @@ obj-          :=
>  obj-$(CONFIG_ARCH_U300)                  += u300.o
>  obj-$(CONFIG_MMC)                 += mmc.o
>  obj-$(CONFIG_SPI_PL022)           += spi.o
> -obj-$(CONFIG_MACH_U300_SPIDUMMY)  += dummyspichip.o
>  obj-$(CONFIG_I2C_STU300)          += i2c.o
>  obj-$(CONFIG_REGULATOR_AB3100)    += regulator.o
> diff --git a/arch/arm/mach-u300/spi.c b/arch/arm/mach-u300/spi.c
> index f0e887b..0790cce 100644
> --- a/arch/arm/mach-u300/spi.c
> +++ b/arch/arm/mach-u300/spi.c
> @@ -16,68 +16,8 @@
>  /*
>  * The following is for the actual devices on the SSP/SPI bus
>  */
> -#ifdef CONFIG_MACH_U300_SPIDUMMY
> -static void select_dummy_chip(u32 chipselect)
> -{
> -       pr_debug("CORE: %s called with CS=0x%x (%s)\n",
> -                __func__,
> -                chipselect,
> -                chipselect ? "unselect chip" : "select chip");
> -       /*
> -        * Here you would write the chip select value to the GPIO pins if
> -        * this was a real chip (but this is a loopback dummy).
> -        */
> -}
> -
> -struct pl022_config_chip dummy_chip_info = {
> -       /* Nominally this is LOOPBACK_DISABLED, but this is our dummy chip! */
> -       .lbm = LOOPBACK_ENABLED,
> -       /*
> -        * available POLLING_TRANSFER and INTERRUPT_TRANSFER,
> -        * DMA_TRANSFER does not work
> -        */
> -       .com_mode = INTERRUPT_TRANSFER,
> -       .iface = SSP_INTERFACE_MOTOROLA_SPI,
> -       /* We can only act as master but SSP_SLAVE is possible in theory */
> -       .hierarchy = SSP_MASTER,
> -       /* 0 = drive TX even as slave, 1 = do not drive TX as slave */
> -       .slave_tx_disable = 0,
> -       /* LSB first */
> -       .endian_tx = SSP_TX_LSB,
> -       .endian_rx = SSP_RX_LSB,
> -       .data_size = SSP_DATA_BITS_8, /* used to be 12 in some default */
> -       .rx_lev_trig = SSP_RX_1_OR_MORE_ELEM,
> -       .tx_lev_trig = SSP_TX_1_OR_MORE_EMPTY_LOC,
> -       .clk_phase = SSP_CLK_SECOND_EDGE,
> -       .clk_pol = SSP_CLK_POL_IDLE_LOW,
> -       .ctrl_len = SSP_BITS_12,
> -       .wait_state = SSP_MWIRE_WAIT_ZERO,
> -       .duplex = SSP_MICROWIRE_CHANNEL_FULL_DUPLEX,
> -       /*
> -        * This is where you insert a call to a function to enable CS
> -        * (usually GPIO) for a certain chip.
> -        */
> -       .cs_control = select_dummy_chip,
> -};
> -#endif
>
>  static struct spi_board_info u300_spi_devices[] = {
> -#ifdef CONFIG_MACH_U300_SPIDUMMY
> -       {
> -               /* A dummy chip used for loopback tests */
> -               .modalias       = "spi-dummy",
> -               /* Really dummy, pass in additional chip config here */
> -               .platform_data  = NULL,
> -               /* This defines how the controller shall handle the device */
> -               .controller_data = &dummy_chip_info,
> -               /* .irq - no external IRQ routed from this device */
> -               .max_speed_hz   = 1000000,
> -               .bus_num        = 0, /* Only one bus on this chip */
> -               .chip_select    = 0,
> -               /* Means SPI_CS_HIGH, change if e.g low CS */
> -               .mode           = 0,
> -       },
> -#endif
>  };
>
>  static struct pl022_ssp_controller ssp_platform_data = {
> @@ -94,7 +34,6 @@ static struct pl022_ssp_controller ssp_platform_data = {
>        .num_chipselect = 3,
>  };
>
> -
>  void __init u300_spi_init(struct amba_device *adev)
>  {
>        struct pmx *pmx;
> diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
> index 91c2f4f..9e584dc 100644
> --- a/drivers/spi/Kconfig
> +++ b/drivers/spi/Kconfig
> @@ -227,6 +227,19 @@ config SPI_PL022
>          controller. If you have an embedded system with an AMBA(R)
>          bus and a PL022 controller, say Y or M here.
>
> +config SPI_PL022_LOOPTEST
> +       bool "PL022 loopback chip"
> +       depends on SPI_PL022
> +       help
> +         The PL022 supports a loopback mode where the output is
> +         looped back to the input for testing.
> +
> +         This creates a small kernel module that creates a dummy
> +         SPI device on each PL022 SPI bus to be used for loopback
> +         tests. You find the device in the /sys filesystem, e.g.
> +         /sys/bus/spi/devices/spi0.0/looptest and the test is run by
> +         issuing cat <looptest>.

Questionable usage of sysfs.  sysfs files should contain single values
only, or a list of things-of-the-same-type, and by convention sysfs
files are supposed to be available immediately when the struct device
is registered.  It looks like debugfs should really be used instead.

> +
>  config SPI_PPC4xx
>        tristate "PPC4xx SPI Controller"
>        depends on PPC32 && 4xx && SPI_MASTER
> diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
> index e9cbd18..9ac8e95 100644
> --- a/drivers/spi/Makefile
> +++ b/drivers/spi/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_SPI_OMAP24XX)            += omap2_mcspi.o
>  obj-$(CONFIG_SPI_OMAP_100K)            += omap_spi_100k.o
>  obj-$(CONFIG_SPI_ORION)                        += orion_spi.o
>  obj-$(CONFIG_SPI_PL022)                        += amba-pl022.o
> +obj-$(CONFIG_SPI_PL022_LOOPTEST)       += amba-pl022-looptest.o
>  obj-$(CONFIG_SPI_MPC512x_PSC)          += mpc512x_psc_spi.o
>  obj-$(CONFIG_SPI_MPC52xx_PSC)          += mpc52xx_psc_spi.o
>  obj-$(CONFIG_SPI_MPC52xx)              += mpc52xx_spi.o
> diff --git a/drivers/spi/amba-pl022-looptest.c b/drivers/spi/amba-pl022-looptest.c
> index 5f55012..561886a 100644
> --- a/drivers/spi/amba-pl022-looptest.c
> +++ b/drivers/spi/amba-pl022-looptest.c
> @@ -1,7 +1,5 @@
>  /*
> - * arch/arm/mach-u300/dummyspichip.c
> - *
> - * Copyright (C) 2007-2009 ST-Ericsson AB
> + * Copyright (C) 2007-2010 ST-Ericsson SA
>  * License terms: GNU General Public License (GPL) version 2
>  * This is a dummy loopback SPI "chip" used for testing SPI.
>  * Author: Linus Walleij <linus.walleij@stericsson.com>
> @@ -25,19 +23,93 @@
>  */
>  #include <linux/amba/pl022.h>
>
> -struct dummy {
> +struct pl022_loopback {
>        struct device *dev;
>        struct mutex lock;
>  };
>
>  #define DMA_TEST_SIZE 2048
>
> -/* When we cat /sys/bus/spi/devices/spi0.0/looptest this will be triggered */
> -static ssize_t dummy_looptest(struct device *dev,
> +/*
> + * The following code self-registers the PL022 loopback
> + * chip on each PL022 SPI bus.
> + */
> +
> +static void pl022_select_loopback_chip(u32 chipselect)
> +{
> +       pr_debug("PL022 loopback chip: %s called with CS=0x%x (%s)\n",
> +                __func__,
> +                chipselect,
> +                chipselect ? "unselect chip" : "select chip");
> +       /*
> +        * Here you would write the chip select value to the GPIO pins if
> +        * this was a real chip (but this is a loopback dummy).
> +        */
> +}
> +
> +struct pl022_config_chip pl022_loopback_chip_info = {
> +       /* Nominally this is LOOPBACK_DISABLED, but this is our dummy chip! */
> +       .lbm = LOOPBACK_ENABLED,
> +       .com_mode = INTERRUPT_TRANSFER,
> +       .iface = SSP_INTERFACE_MOTOROLA_SPI,
> +       .hierarchy = SSP_MASTER,
> +       .slave_tx_disable = 0,
> +       /* LSB first */
> +       .endian_tx = SSP_TX_LSB,
> +       .endian_rx = SSP_RX_LSB,
> +       .data_size = SSP_DATA_BITS_8,
> +       .rx_lev_trig = SSP_RX_1_OR_MORE_ELEM,
> +       .tx_lev_trig = SSP_TX_1_OR_MORE_EMPTY_LOC,
> +       .clk_phase = SSP_CLK_SECOND_EDGE,
> +       .clk_pol = SSP_CLK_POL_IDLE_LOW,
> +       .ctrl_len = SSP_BITS_12,
> +       .wait_state = SSP_MWIRE_WAIT_ZERO,
> +       .duplex = SSP_MICROWIRE_CHANNEL_FULL_DUPLEX,
> +       .cs_control = pl022_select_loopback_chip,
> +};
> +
> +static struct spi_board_info pl022_loopback_device = {
> +       /* A dummy chip used for loopback tests */
> +       .modalias       = "pl022-loop",
> +       /* Really dummy, pass in additional chip config here */
> +       .platform_data  = NULL,
> +       /* This defines how the controller shall handle the device */
> +       .controller_data = &pl022_loopback_chip_info,
> +       /* .irq - no external IRQ routed from this device */
> +       .max_speed_hz   = 1000000,
> +       .bus_num        = 0, /* Only one bus on this chip */
> +       .chip_select    = 0,
> +       /* Means SPI_CS_HIGH, change if e.g low CS */
> +       .mode           = 0,
> +};
> +
> +/*
> + * This is called from the PL022 driver to register a loopback
> + * on itself if this module is compiled in.
> + */
> +void pl022_loopback_register(struct spi_master *master)
> +{
> +       struct spi_device *spidev;
> +
> +       spidev = spi_new_device(master, &pl022_loopback_device);
> +       if (!spidev)
> +               dev_err(&master->dev,
> +                       "could not register loopback test chip\n");
> +       else
> +               dev_err(&master->dev,
> +                       "registered loopback test chip\n");
> +}
> +
> +/*
> + * This is where the tests begin. When we issue:
> + * cat /sys/bus/spi/devices/spi0.0/looptest
> + * this will be triggered.
> + */
> +static ssize_t pl022_looptest(struct device *dev,
>                struct device_attribute *attr, char *buf)
>  {
>        struct spi_device *spi = to_spi_device(dev);
> -       struct dummy *p_dummy = dev_get_drvdata(&spi->dev);
> +       struct pl022_loopback *p_loop = dev_get_drvdata(&spi->dev);
>
>        /*
>         * WARNING! Do not dereference the chip-specific data in any normal
> @@ -55,7 +127,7 @@ static ssize_t dummy_looptest(struct device *dev,
>        u8 *bigtxbuf_virtual;
>        u8 *bigrxbuf_virtual;
>
> -       if (mutex_lock_interruptible(&p_dummy->lock))
> +       if (mutex_lock_interruptible(&p_loop->lock))
>                return -ERESTARTSYS;
>
>        bigtxbuf_virtual = kmalloc(DMA_TEST_SIZE, GFP_KERNEL);
> @@ -217,25 +289,25 @@ static ssize_t dummy_looptest(struct device *dev,
>        kfree(bigrxbuf_virtual);
>        kfree(bigtxbuf_virtual);
>  out:
> -       mutex_unlock(&p_dummy->lock);
> +       mutex_unlock(&p_loop->lock);
>        return status;
>  }
>
> -static DEVICE_ATTR(looptest, S_IRUGO, dummy_looptest, NULL);
> +static DEVICE_ATTR(looptest, S_IRUGO, pl022_looptest, NULL);
>
> -static int __devinit pl022_dummy_probe(struct spi_device *spi)
> +static int __devinit pl022_loopback_probe(struct spi_device *spi)
>  {
> -       struct dummy *p_dummy;
> +       struct pl022_loopback *p_loop;
>        int status;
>
>        dev_info(&spi->dev, "probing dummy SPI device\n");
>
> -       p_dummy = kzalloc(sizeof *p_dummy, GFP_KERNEL);
> -       if (!p_dummy)
> +       p_loop = kzalloc(sizeof *p_loop, GFP_KERNEL);
> +       if (!p_loop)
>                return -ENOMEM;
>
> -       dev_set_drvdata(&spi->dev, p_dummy);
> -       mutex_init(&p_dummy->lock);
> +       dev_set_drvdata(&spi->dev, p_loop);
> +       mutex_init(&p_loop->lock);
>
>        /* sysfs hook */
>        status = device_create_file(&spi->dev, &dev_attr_looptest);
> @@ -248,29 +320,29 @@ static int __devinit pl022_dummy_probe(struct spi_device *spi)
>
>  out_dev_create_looptest_failed:
>        dev_set_drvdata(&spi->dev, NULL);
> -       kfree(p_dummy);
> +       kfree(p_loop);
>        return status;
>  }
>
> -static int __devexit pl022_dummy_remove(struct spi_device *spi)
> +static int __devexit pl022_loopback_remove(struct spi_device *spi)
>  {
> -       struct dummy *p_dummy = dev_get_drvdata(&spi->dev);
> +       struct pl022_loopback *p_loop = dev_get_drvdata(&spi->dev);
>
>        dev_info(&spi->dev, "removing dummy SPI device\n");
>        device_remove_file(&spi->dev, &dev_attr_looptest);
>        dev_set_drvdata(&spi->dev, NULL);
> -       kfree(p_dummy);
> +       kfree(p_loop);
>
>        return 0;
>  }
>
>  static struct spi_driver pl022_dummy_driver = {
>        .driver = {
> -               .name   = "spi-dummy",
> +               .name   = "pl022-loop",
>                .owner  = THIS_MODULE,
>        },
> -       .probe  = pl022_dummy_probe,
> -       .remove = __devexit_p(pl022_dummy_remove),
> +       .probe  = pl022_loopback_probe,
> +       .remove = __devexit_p(pl022_loopback_remove),
>  };
>
>  static int __init pl022_init_dummy(void)
> @@ -287,5 +359,5 @@ module_init(pl022_init_dummy);
>  module_exit(pl022_exit_dummy);
>
>  MODULE_AUTHOR("Linus Walleij <linus.walleij@stericsson.com>");
> -MODULE_DESCRIPTION("PL022 SSP/SPI DUMMY Linux driver");
> +MODULE_DESCRIPTION("PL022 loopback test driver");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/spi/amba-pl022-looptest.h b/drivers/spi/amba-pl022-looptest.h
> new file mode 100644
> index 0000000..1bedcc6
> --- /dev/null
> +++ b/drivers/spi/amba-pl022-looptest.h
> @@ -0,0 +1,11 @@
> +/*
> + * Defines a single function for the master to create a loopback
> + * on itself.
> + */
> +#ifdef CONFIG_SPI_PL022_LOOPTEST
> +void pl022_loopback_register(struct spi_master *master);
> +#else
> +static void inline pl022_loopback_register(struct spi_master *master)
> +{
> +}
> +#endif
> diff --git a/drivers/spi/amba-pl022.c b/drivers/spi/amba-pl022.c
> index f0a1418..6e4a657 100644
> --- a/drivers/spi/amba-pl022.c
> +++ b/drivers/spi/amba-pl022.c
> @@ -46,6 +46,8 @@
>  #include <linux/io.h>
>  #include <linux/slab.h>
>
> +#include "amba-pl022-looptest.h"
> +
>  /*
>  * This macro is used to define some register default values.
>  * reg is masked with mask, the OR:ed with an (again masked)
> @@ -1818,6 +1820,10 @@ pl022_probe(struct amba_device *adev, struct amba_id *id)
>                goto err_spi_register;
>        }
>        dev_dbg(dev, "probe succeded\n");
> +
> +       /* Register a loopback test if desired */
> +       pl022_loopback_register(master);
> +
>        return 0;
>
>  err_spi_register:
> --
> 1.7.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>



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

Re: [spi-devel-general] [PATCH 2/2] SPI: make the moved loopback test work

[David Brownell]

A dedicated loopback test is the wrong model,
given that SPI_LOOP exists.  And is even used
in the sample userspace code ...

- Dave

Re: [PATCH 2/2] SPI: make the moved loopback test work

[Linus Walleij]

2010/7/19 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>:

> A dedicated loopback test is the wrong model,
> given that SPI_LOOP exists.  And is even used
> in the sample userspace code ...

Yeah I see, I never quite understood how SPI_LOOP
was intended really.

>From there to writing a *generic* loopback test is a bit
of road to cover though, let's see if I can figure it out.

The way I read the code I still have to provide some
kind of dummychip on the bus in order to perform a
loopback test on a bus with no devices connected,
am I right?

Yours,
Linus Walleij

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Re: [PATCH 2/2] SPI: make the moved loopback test work

[Linus Walleij]

2010/7/19 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>:

> A dedicated loopback test is the wrong model,
> given that SPI_LOOP exists.  And is even used
> in the sample userspace code ...

Hm looking over and grepping everywhere about this gives
me no clues as to how an in-kernel SPI loopback test could
use spi->mode |= SPI_LOOP. It can be set, and then it expects
the hardware to set the corresponding bit.

No matter how much I look at it, it seems like all use of this
require you to first register *some* device, whether real or
not, then you may play with it from kernelspace or userspace
alike.

So for a generic loopback in-kernel test I would need
something like:

loop over all masters
  for each master
    if it supports SPI_LOOP
      look for devices on master - make sure first device is a
      loopback dummy
      locate loopback dummy
      perform loopback test on the dummy

This means I still would have to register some dummychip
on the devices that should be able to run the test.

Anton, any hints on this, execept that you added it for
spidev?

I'll try to dream some use model up otherwise.

Yours,
Linus Walleij

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Re: [spi-devel-general] [PATCH 2/2] SPI: make the moved loopback test work

[David Brownell]

> No matter how much I look at it, it seems like
> all use of this require you to first register
> *some* device, 

OBVIOUSLY.  You can't use SPI without a device;
it's like any other driver framework in Linux.
What else should you expect???

Look at the sample code though, ISTR you'll see
that when the SPI master works correctly, putting
it into loopback mode just means that the writes
and reads (should) get the same data -- full duplex.
No matter what the device is (given SPI_LOOP).

- Dave

Re: [PATCH 2/2] SPI: make the moved loopback test work

[Linus Walleij]

2010/7/20 David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>:

>> No matter how much I look at it, it seems like
>> all use of this require you to first register
>> *some* device,
>
> OBVIOUSLY.  You can't use SPI without a device;
> it's like any other driver framework in Linux.
> What else should you expect???

I didn't expect anything else, it just makes it clear that
the part of my patch registering a dummy device is
not what is being disagreed upon, so this should be OK.

Yours,
Linus Walleij

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first

Re: [spi-devel-general] [PATCH 2/2] SPI: make the moved loopback test work

[Grant Likely]

On Tue, Jul 20, 2010 at 3:10 PM, Linus Walleij
<linus.ml.walleij@gmail.com> wrote:
> 2010/7/20 David Brownell <david-b@pacbell.net>:
>
>>> No matter how much I look at it, it seems like
>>> all use of this require you to first register
>>> *some* device,
>>
>> OBVIOUSLY.  You can't use SPI without a device;
>> it's like any other driver framework in Linux.
>> What else should you expect???
>
> I didn't expect anything else, it just makes it clear that
> the part of my patch registering a dummy device is
> not what is being disagreed upon, so this should be OK.

What's disagreed with is the dummy device knowing the intimate working
details of the SPI bus driver.

g.

Re: [PATCH 2/2] SPI: make the moved loopback test work

[Linus Walleij]

2010/7/25 Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>:

> What's disagreed with is the dummy device knowing the intimate working
> details of the SPI bus driver.

OK I'll send the other patch I've been holding back, it merges that part into
the driver itself so it sits in the apropriate containment, and solves
that specific issue.

But I understood Davids criticism like this: "you shouldn't make tests
using loopback for a specific driver, it should be generic, i.e. being
able to perform a loopback test on any driver supporting SPI_LOOP".

A generic loopback test is a lot harder for me to produce, that's
why I was troubled...

Yours,
Linus Walleij

------------------------------------------------------------------------------
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first