linux-spi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][RFC] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO
@ 2010-11-19 22:19 Ben Gardiner
  2010-12-09 20:34 ` [RFC][PATCH v2 0/3] slower spi-gpio Ben Gardiner
  0 siblings, 1 reply; 11+ messages in thread
From: Ben Gardiner @ 2010-11-19 22:19 UTC (permalink / raw)
  To: spi-devel-general, Grant Likely, David Brownell
  Cc: linux-kernel, Michael Buesch

Introduce a Kconfig option, SLOWER_SPI_GPIO, that enables a spidelay
implementation of ndelay when defined.

This is based off of the patch proposed by Michael Buesch [1]. Whereas that
patch required slave's set their spi_device.max_speed_hz and
spi_transfer.speed_hz to 0 to obtain 'as fast as we can transfers' this patch
keeps the default of 'as fast as we can' for any GPIO SPI master unless the
proposed Kconfig option is enabled.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Dan Sharon <dansharon@nanometrics.ca>
Reviewed-by: Chris Cordahi <chriscordahi@nanometrics.ca>
CC: Michael Buesch <mb@bu3sch.de>

[1] http://article.gmane.org/gmane.comp.embedded.openwrt.devel/2854

---

This patch was rebased to 6656b3fc8aba2eb7ca00c06c7fe4917938b0b652 of
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git

Testing was performed using the GPIOs of a da850evm. I registered an spi-gpio
instance and a slave device with a transfer speed of 500Hz (the slowest
possible speed on ARM since the udelay max is 2ms):

...
static struct spi_gpio_platform_data bb_spi_gpio_pdata = {
       .sck            = SPIG_CLK,
       .mosi           = SPIG_SIMO,
       .miso           = SPIG_SOMI,
       .num_chipselect = 1,
};

static struct platform_device bb_spi_gpio = {
       .name           = "spi_gpio",
       .id             = 0,
       .dev            = {
               .platform_data  = &bb_spi_gpio_pdata,
       },
};

static struct spi_board_info bb_spi_devices[] = {
       {
               .modalias       = "spidev",
               .max_speed_hz   = 500,
               .bus_num        = 0,
               .chip_select    = 0,
               .mode           = SPI_MODE_0,
       },
};
...

Without SLOWER_SPI_GPIO=y a spi transfer using 'echo a > /dev/spidev0.0'
resulted in a ~700kHz SCLK signal. 
With SLOWER_SPI_GPIO=y a spi transfer using 'echo a > /dev/spidev0.0'
resulted in a ~500Hz SCLK signal.

---

 drivers/spi/Kconfig    |   17 +++++++++++++++++
 drivers/spi/spi_gpio.c |    5 +++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 78f9fd0..0570dcd 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -143,6 +143,23 @@ config SPI_GPIO
 	  GPIO operations, you should be able to leverage that for better
 	  speed with a custom version of this driver; see the source code.
 
+config SLOWER_SPI_GPIO
+	bool "Enable delays in the GPIO-based bitbanging SPI Master"
+	default n
+	depends on SPI_GPIO
+	help
+	  The GPIO bitbanging SPI master driver will run without any delays if this
+	  option is not set. This option will enable the use of delays in the
+	  operation of the GPIO bitbanging SPI master implementation to honour the
+	  maximum speed of very slow devices.
+
+	  To configure slow speed devices your board-specific setup logic must also
+	  provide platform data assigning the speed for a device on a given chip
+	  select of the GPIO bitbanging SPI master.
+
+	  If your platform requires SPI buses driven at slow speeds select yes. If
+	  in doubt, select no.
+
 config SPI_IMX_VER_IMX1
 	def_bool y if SOC_IMX1
 
diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 63e51b0..b31fddd 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -19,6 +19,7 @@
  */
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
 
@@ -119,6 +120,9 @@ static inline int getmiso(const struct spi_device *spi)
 
 #undef pdata
 
+#if defined(CONFIG_SLOWER_SPI_GPIO)
+#define spidelay(nsecs) ndelay(nsecs)
+#else
 /*
  * NOTE:  this clocks "as fast as we can".  It "should" be a function of the
  * requested device clock.  Software overhead means we usually have trouble
@@ -126,6 +130,7 @@ static inline int getmiso(const struct spi_device *spi)
  * we'll just assume we never need additional per-bit slowdowns.
  */
 #define spidelay(nsecs)	do {} while (0)
+#endif
 
 #include "spi_bitbang_txrx.h"
 
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH v2 0/3] slower spi-gpio
  2010-11-19 22:19 [PATCH][RFC] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO Ben Gardiner
@ 2010-12-09 20:34 ` Ben Gardiner
  2010-12-09 20:34   ` [RFC][PATCH v2 1/3] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO Ben Gardiner
                     ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Ben Gardiner @ 2010-12-09 20:34 UTC (permalink / raw)
  To: spi-devel-general, Grant Likely, David Brownell
  Cc: linux-kernel, Michael Buesch

This a continuation of the previous RFC [1]; we are interested in running a
bitbanged SPI bus with a limited rate of transfer, including the delay between
CS assertion and the beginning of data transfer. We also would like to do this 
with the GPIOs on an IO expander.

The following three changes have been identified so far as being required to 
achieve this. spidelay() must be implemented, the delay specified in
controller_state should be honoured when a transfer begins and finally the 
_cansleep gpio set functions should be used so that GPIOs on an expander can
be used. 

This is alot of compile-time functionality switching though. I wonder if this
kind of patching of spi-gpio would be acceptable or not; given the recent
events in polled gpio-keys I feel that maybe a new 'spi-gpio-limited' driver 
might be preferred.

[1] http://article.gmane.org/gmane.linux.kernel/1065748

Ben Gardiner (3):
  [RFC] spi-gpio: implement spidelay() for platforms that configure
    SLOWER_SPI_GPIO
  [RFC] spi_bitbang : get nsecs delay from cs during transfer
  [RFC] spi-gpio: use _cansleep when CONFIG_SLOWER_SPI_GPIO is defined

 drivers/spi/Kconfig       |   17 +++++++++++++++++
 drivers/spi/spi_bitbang.c |   15 +++++++++------
 drivers/spi/spi_gpio.c    |   18 +++++++++++++++---
 3 files changed, 41 insertions(+), 9 deletions(-)

---

Changes since v1:
 * rebased to  2.6.37-rc5
 * introduced patch 2/3 and 3/3

This time around I tested the changes by registering a spi-gpio bus intance
on the user DIP switches of the da850evm baseboard which are connected to a 
pca953x-compatible I2C gpio expander. Then I did a few 
'echo hello > /dev/spidev0.0' commands and checked the behaviour of the bus
on a scope. I was unable to test the speed of this bus without the patchset
this time since using the GPIOs on the expander requires patch 3/3. I found 
that the SCLK speed was roughly 300Hz where 500Hz was requested.

Here is a snip of the board-setup code that registers the spi-gpio bus:

static struct spi_gpio_platform_data bb_spi_gpio_pdata = {
	.sck		= -1 /* assigned at runtime */,
	.mosi		= -1 /* assigned at runtime */,
	.miso		= -1 /* assigned at runtime */,
	.num_chipselect = 1,
};

static struct platform_device bb_spi_gpio = {
	.name		= "spi_gpio",
	.id		= 0,
	.dev		= {
		.platform_data	= &bb_spi_gpio_pdata,
	},
};

static struct spi_board_info bb_spi_devices[] = {
	{
		.modalias	= "spidev",
		.max_speed_hz	= 500,
		.bus_num	= 0,
		.chip_select	= 0,
		.mode		= SPI_MODE_0,
		.controller_data = (void *) -1 /* assigned at runtime */,
	},
};

static int bb_spi_gpio_init(unsigned gpio)
{
	int ret;

	bb_spi_gpio_pdata.sck = gpio + DA850_EVM_BB_EXP_USER_SW1;
	bb_spi_gpio_pdata.mosi = gpio + DA850_EVM_BB_EXP_USER_SW2;
	bb_spi_gpio_pdata.miso = -1 /* nothing connected ATM */;

	bb_spi_devices[0].controller_data = (void *) gpio +
						DA850_EVM_BB_EXP_USER_SW3;

	ret = spi_register_board_info(bb_spi_devices,
					ARRAY_SIZE(bb_spi_devices));
	if (ret) {
		pr_warning("could not register spi devices");
		goto spi_gpio_fail_devices;
	}

	ret = platform_device_register(&bb_spi_gpio);
	if (ret) {
		pr_warning("could not register spi-gpio driver instance");
		goto spi_gpio_fail_bus;
	}

	return 0;
spi_gpio_fail_bus:
	/* no unregister board info function available*/
spi_gpio_fail_devices:
	return ret;
}

static int da850_evm_bb_expander_setup(struct i2c_client *client,
						unsigned gpio, unsigned ngpio,
						void *c)
{
	int ret;

	/*
	 * Register the switches and pushbutton on the baseboard as a gpio-keys
	 * device.
	 */
	da850_evm_bb_keys_init(gpio);
	ret = platform_device_register(&da850_evm_bb_keys_device);
	if (ret) {
		pr_warning("Could not register baseboard GPIO expander keys");
		goto io_exp_setup_sw_fail;
	}

	da850_evm_bb_leds_init(gpio);
	ret = platform_device_register(&da850_evm_bb_leds_device);
	if (ret) {
		pr_warning("Could not register baseboard GPIO expander LEDS");
		goto io_exp_setup_leds_fail;
	}

	ret = bb_spi_gpio_init(gpio);
	if (ret) {
		pr_warning("Could not register spi-gpio bus");
		goto io_exp_setup_spi_fail;
	}

	return 0;

io_exp_setup_spi_fail:
	platform_device_unregister(&da850_evm_bb_leds_device);
io_exp_setup_leds_fail:
	platform_device_unregister(&da850_evm_bb_keys_device);
io_exp_setup_sw_fail:
	return ret;
}

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [RFC][PATCH v2 1/3] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO
  2010-12-09 20:34 ` [RFC][PATCH v2 0/3] slower spi-gpio Ben Gardiner
@ 2010-12-09 20:34   ` Ben Gardiner
  2010-12-09 20:34   ` [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer Ben Gardiner
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Ben Gardiner @ 2010-12-09 20:34 UTC (permalink / raw)
  To: spi-devel-general, Grant Likely, David Brownell
  Cc: linux-kernel, Michael Buesch

Introduce a Kconfig option, SLOWER_SPI_GPIO, that enables a spidelay
implementation of ndelay when defined.

This is based off of the patch proposed by Michael Buesch [1]. Whereas that
patch required slave's set their spi_device.max_speed_hz and
spi_transfer.speed_hz to 0 to obtain 'as fast as we can transfers' this patch
keeps the default of 'as fast as we can' for any GPIO SPI master unless the
proposed Kconfig option is enabled.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
Reviewed-by: Dan Sharon <dansharon@nanometrics.ca>
Reviewed-by: Chris Cordahi <chriscordahi@nanometrics.ca>
CC: Michael Buesch <mb@bu3sch.de>

[1] http://article.gmane.org/gmane.comp.embedded.openwrt.devel/2854

---
changes since v1:
 * rebased to v2.6.37-rc5

 drivers/spi/Kconfig    |   17 +++++++++++++++++
 drivers/spi/spi_gpio.c |    5 +++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 78f9fd0..0570dcd 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -143,6 +143,23 @@ config SPI_GPIO
 	  GPIO operations, you should be able to leverage that for better
 	  speed with a custom version of this driver; see the source code.
 
+config SLOWER_SPI_GPIO
+	bool "Enable delays in the GPIO-based bitbanging SPI Master"
+	default n
+	depends on SPI_GPIO
+	help
+	  The GPIO bitbanging SPI master driver will run without any delays if this
+	  option is not set. This option will enable the use of delays in the
+	  operation of the GPIO bitbanging SPI master implementation to honour the
+	  maximum speed of very slow devices.
+
+	  To configure slow speed devices your board-specific setup logic must also
+	  provide platform data assigning the speed for a device on a given chip
+	  select of the GPIO bitbanging SPI master.
+
+	  If your platform requires SPI buses driven at slow speeds select yes. If
+	  in doubt, select no.
+
 config SPI_IMX_VER_IMX1
 	def_bool y if SOC_IMX1
 
diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index 63e51b0..b31fddd 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -19,6 +19,7 @@
  */
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/delay.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
 
@@ -119,6 +120,9 @@ static inline int getmiso(const struct spi_device *spi)
 
 #undef pdata
 
+#if defined(CONFIG_SLOWER_SPI_GPIO)
+#define spidelay(nsecs) ndelay(nsecs)
+#else
 /*
  * NOTE:  this clocks "as fast as we can".  It "should" be a function of the
  * requested device clock.  Software overhead means we usually have trouble
@@ -126,6 +130,7 @@ static inline int getmiso(const struct spi_device *spi)
  * we'll just assume we never need additional per-bit slowdowns.
  */
 #define spidelay(nsecs)	do {} while (0)
+#endif
 
 #include "spi_bitbang_txrx.h"
 
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer
  2010-12-09 20:34 ` [RFC][PATCH v2 0/3] slower spi-gpio Ben Gardiner
  2010-12-09 20:34   ` [RFC][PATCH v2 1/3] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO Ben Gardiner
@ 2010-12-09 20:34   ` Ben Gardiner
  2010-12-10 11:46     ` Wolfram Sang
  2010-12-09 20:34   ` [RFC][PATCH v2 3/3] spi-gpio: use _cansleep when CONFIG_SLOWER_SPI_GPIO is defined Ben Gardiner
  2010-12-10  4:38   ` [RFC][PATCH v2 0/3] slower spi-gpio David Brownell
  3 siblings, 1 reply; 11+ messages in thread
From: Ben Gardiner @ 2010-12-09 20:34 UTC (permalink / raw)
  To: spi-devel-general, Grant Likely, David Brownell
  Cc: linux-kernel, Michael Buesch

When users have elected to enable delays in the spi-gpio driver, use the
cs speed to govern the time between CS assertion and beginning of data
transfer on the bus

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>
---
changes since v1:
 * none; new in v2

 drivers/spi/spi_bitbang.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
index 8b55724..8707133 100644
--- a/drivers/spi/spi_bitbang.c
+++ b/drivers/spi/spi_bitbang.c
@@ -269,6 +269,7 @@ static void bitbang_work(struct work_struct *work)
 	while (!list_empty(&bitbang->queue)) {
 		struct spi_message	*m;
 		struct spi_device	*spi;
+		struct spi_bitbang_cs	*cs;
 		unsigned		nsecs;
 		struct spi_transfer	*t = NULL;
 		unsigned		tmp;
@@ -281,13 +282,15 @@ static void bitbang_work(struct work_struct *work)
 		list_del_init(&m->queue);
 		spin_unlock_irqrestore(&bitbang->lock, flags);
 
-		/* FIXME this is made-up ... the correct value is known to
-		 * word-at-a-time bitbang code, and presumably chipselect()
-		 * should enforce these requirements too?
-		 */
-		nsecs = 100;
-
 		spi = m->spi;
+		cs = spi->controller_state;
+		nsecs =
+#if defined(CONFIG_SLOWER_SPI_GPIO)
+				!cs->nsecs ? cs->nsecs : 100;
+#else
+		100;
+#endif
+
 		tmp = 0;
 		cs_change = 1;
 		status = 0;
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [RFC][PATCH v2 3/3] spi-gpio: use _cansleep when CONFIG_SLOWER_SPI_GPIO is defined
  2010-12-09 20:34 ` [RFC][PATCH v2 0/3] slower spi-gpio Ben Gardiner
  2010-12-09 20:34   ` [RFC][PATCH v2 1/3] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO Ben Gardiner
  2010-12-09 20:34   ` [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer Ben Gardiner
@ 2010-12-09 20:34   ` Ben Gardiner
  2010-12-10  4:38   ` [RFC][PATCH v2 0/3] slower spi-gpio David Brownell
  3 siblings, 0 replies; 11+ messages in thread
From: Ben Gardiner @ 2010-12-09 20:34 UTC (permalink / raw)
  To: spi-devel-general, Grant Likely, David Brownell
  Cc: linux-kernel, Michael Buesch

When users have elected to enable delays in the spi-gpio driver, use the
_cansleep variant of gpio_set_value. This allows the use of GPIOs on an
IO expander.

Signed-off-by: Ben Gardiner <bengardiner@nanometrics.ca>

---

changes since v1:
 * none; new in v2

I was testing the slowed-down spio-gpio driver on a I2C pca953x expander
when I noticed the WARNs that the spio-gpio driver was calling into the
I2C expander drivers gpio_set functions without using the _cansleep
variant. This patch removes all the warnings except for one on startup
when the spi-gpio instance is registered:

BUG: scheduling while atomic: swapper/1/0x00000002
Modules linked in:
[<c002f79c>] (unwind_backtrace+0x0/0xec) from [<c02859f8>] (schedule+0x74/0x408)
[<c02859f8>] (schedule+0x74/0x408) from [<c028653c>] (schedule_timeout+0x1d4/0x214)
[<c028653c>] (schedule_timeout+0x1d4/0x214) from [<c02861f4>] (wait_for_common+0xf0/0x1b8)
[<c02861f4>] (wait_for_common+0xf0/0x1b8) from [<c01e8c50>] (i2c_davinci_xfer+0x1ec/0x304)
[<c01e8c50>] (i2c_davinci_xfer+0x1ec/0x304) from [<c01e6e40>] (i2c_transfer+0xc4/0x120)
[<c01e6e40>] (i2c_transfer+0xc4/0x120) from [<c01e72dc>] (i2c_smbus_xfer+0x3ac/0x4ec)
[<c01e72dc>] (i2c_smbus_xfer+0x3ac/0x4ec) from [<c01e7610>] (i2c_smbus_write_word_data+0x38/0x40)
[<c01e7610>] (i2c_smbus_write_word_data+0x38/0x40) from [<c018a8ec>] (pca953x_write_reg+0x38/0x64)
[<c018a8ec>] (pca953x_write_reg+0x38/0x64) from [<c018a954>] (pca953x_gpio_set_value+0x3c/0x48)
[<c018a954>] (pca953x_gpio_set_value+0x3c/0x48) from [<c0189868>] (gpio_set_value_cansleep+0x30/0x38)
[<c0189868>] (gpio_set_value_cansleep+0x30/0x38) from [<c01d81d8>] (spi_bitbang_setup+0xb4/0x104)
[<c01d81d8>] (spi_bitbang_setup+0xb4/0x104) from [<c01d88d4>] (spi_gpio_setup+0x6c/0xa0)
[<c01d88d4>] (spi_gpio_setup+0x6c/0xa0) from [<c01d74d4>] (spi_setup+0x48/0x50)
[<c01d74d4>] (spi_setup+0x48/0x50) from [<c01d7754>] (spi_add_device+0xa0/0x118)
[<c01d7754>] (spi_add_device+0xa0/0x118) from [<c01d784c>] (spi_new_device+0x80/0xa4)
[<c01d784c>] (spi_new_device+0x80/0xa4) from [<c01d7890>] (spi_match_master_to_boardinfo+0x20/0x40)
[<c01d7890>] (spi_match_master_to_boardinfo+0x20/0x40) from [<c01d7974>] (spi_register_master+0xc4/0x10c)
[<c01d7974>] (spi_register_master+0xc4/0x10c) from [<c01d7d54>] (spi_bitbang_start+0x11c/0x154)
[<c01d7d54>] (spi_bitbang_start+0x11c/0x154) from [<c001b640>] (spi_gpio_probe+0x1ac/0x23c)
[<c001b640>] (spi_gpio_probe+0x1ac/0x23c) from [<c01be7d8>] (platform_drv_probe+0x18/0x1c)
[<c01be7d8>] (platform_drv_probe+0x18/0x1c) from [<c01bd950>] (driver_probe_device+0xb0/0x16c)
[<c01bd950>] (driver_probe_device+0xb0/0x16c) from [<c01bda6c>] (__driver_attach+0x60/0x84)
[<c01bda6c>] (__driver_attach+0x60/0x84) from [<c01bd194>] (bus_for_each_dev+0x44/0x74)
[<c01bd194>] (bus_for_each_dev+0x44/0x74) from [<c01bcae4>] (bus_add_driver+0xa8/0x228)
[<c01bcae4>] (bus_add_driver+0xa8/0x228) from [<c01bdd3c>] (driver_register+0xa8/0x134)
[<c01bdd3c>] (driver_register+0xa8/0x134) from [<c01bea44>] (platform_driver_probe+0x18/0x98)
[<c01bea44>] (platform_driver_probe+0x18/0x98) from [<c002a430>] (do_one_initcall+0xc8/0x1a4)
[<c002a430>] (do_one_initcall+0xc8/0x1a4) from [<c00083cc>] (kernel_init+0x94/0x14c)
[<c00083cc>] (kernel_init+0x94/0x14c) from [<c002b99c>] (kernel_thread_exit+0x0/0x8)
---
 drivers/spi/spi_gpio.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi_gpio.c b/drivers/spi/spi_gpio.c
index b31fddd..537ce13 100644
--- a/drivers/spi/spi_gpio.c
+++ b/drivers/spi/spi_gpio.c
@@ -78,6 +78,12 @@ struct spi_gpio {
 
 #define GENERIC_BITBANG	/* vs tight inlines */
 
+#if defined(CONFIG_SLOWER_SPI_GPIO)
+#define spi_gpio_gpio_set_value gpio_set_value_cansleep
+#else
+#define spi_gpio_gpio_set_value gpio_set_value
+#endif
+
 /* all functions referencing these symbols must define pdata */
 #define SPI_MISO_GPIO	((pdata)->miso)
 #define SPI_MOSI_GPIO	((pdata)->mosi)
@@ -105,12 +111,12 @@ spi_to_pdata(const struct spi_device *spi)
 
 static inline void setsck(const struct spi_device *spi, int is_on)
 {
-	gpio_set_value(SPI_SCK_GPIO, is_on);
+	spi_gpio_gpio_set_value(SPI_SCK_GPIO, is_on);
 }
 
 static inline void setmosi(const struct spi_device *spi, int is_on)
 {
-	gpio_set_value(SPI_MOSI_GPIO, is_on);
+	spi_gpio_gpio_set_value(SPI_MOSI_GPIO, is_on);
 }
 
 static inline int getmiso(const struct spi_device *spi)
@@ -222,7 +228,8 @@ static void spi_gpio_chipselect(struct spi_device *spi, int is_active)
 
 	if (cs != SPI_GPIO_NO_CHIPSELECT) {
 		/* SPI is normally active-low */
-		gpio_set_value(cs, (spi->mode & SPI_CS_HIGH) ? is_active : !is_active);
+		spi_gpio_gpio_set_value(cs, (spi->mode & SPI_CS_HIGH) ?
+							is_active : !is_active);
 	}
 }
 
-- 
1.7.0.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH v2 0/3] slower spi-gpio
  2010-12-09 20:34 ` [RFC][PATCH v2 0/3] slower spi-gpio Ben Gardiner
                     ` (2 preceding siblings ...)
  2010-12-09 20:34   ` [RFC][PATCH v2 3/3] spi-gpio: use _cansleep when CONFIG_SLOWER_SPI_GPIO is defined Ben Gardiner
@ 2010-12-10  4:38   ` David Brownell
  2010-12-14 16:58     ` Ben Gardiner
  3 siblings, 1 reply; 11+ messages in thread
From: David Brownell @ 2010-12-10  4:38 UTC (permalink / raw)
  To: spi-devel-general, Grant Likely, David Brownell, Ben Gardiner
  Cc: linux-kernel, Michael Buesch



--- On Thu, 12/9/10, Ben Gardiner <bengardiner@nanometrics.ca> wrote:


> This a continuation of the previous
> RFC [1]; we

You know, the spi-gpio code was written so it
would not need Kconfiguration, and I'd like
to see that continued.

Surely the minimal configuration you'd need
could be wrapped up in #defineslike the
actual GPIO numbers are wrapped up in.

Heck, nothing outside your patches needs your
:slow it way the heck down" option, so there's
no point to exposing it  via Kconfig;
such stuff is routinely embedded in C code.


 are interested in running a
> bitbanged SPI bus with a limited rate of transfer,

Would it make more sense to have a separate
slowed-down veresion of the driver, maybe just
custom defs for our hardware plus the current
driver body, as explained in the driver code
(last time I looked at it, anyway).

It bothers me to think of cluttering the
current driver with slowdowns

Or just have the "be slow" options private to
the driver instance, like "which GPIOs" is??

(I notice you didn't even check the GPIOs to see
if they are sleeping calls (e.g. over I2C), which
would have been preferable to a static always-slow
Kconfig option.  (But not to an always-slow object
vs the current default always-fast model.

I still need to be able to get multi-megabit
SPI clock rates out of the standard spi-gpio
code base.  (When I've had to use spi-gpio it
has never been a performance issue; the code
was written to facilitate inner bitbang loops
of about half a dozen instructions (ARM).

These recent patches look to be de-tuning badly,
or at least optimizing for slowness not speed,
which seems the wrong default.

- Dave


> including the delay between
> CS assertion and the beginning of data transfer. We also
> would like to do this 
> with the GPIOs on an IO expander.
> 
> The following three changes have been identified so far as
> being required to 
> achieve this. spidelay() must be implemented, the delay
> specified in
> controller_state should be honoured when a transfer begins
> and finally the 
> _cansleep gpio set functions should be used so that GPIOs
> on an expander can
> be used. 
> 
> This is alot of compile-time functionality switching
> though. I wonder if this
> kind of patching of spi-gpio would be acceptable or not;
> given the recent
> events in polled gpio-keys I feel that maybe a new
> 'spi-gpio-limited' driver 
> might be preferred.
> 
> [1] http://article.gmane.org/gmane.linux.kernel/1065748
> 
> Ben Gardiner (3):
>   [RFC] spi-gpio: implement spidelay() for platforms
> that configure
>     SLOWER_SPI_GPIO
>   [RFC] spi_bitbang : get nsecs delay from cs during
> transfer
>   [RFC] spi-gpio: use _cansleep when
> CONFIG_SLOWER_SPI_GPIO is defined
> 
>  drivers/spi/Kconfig   
>    |   17 +++++++++++++++++
>  drivers/spi/spi_bitbang.c |   15
> +++++++++------
>  drivers/spi/spi_gpio.c    |   18
> +++++++++++++++---
>  3 files changed, 41 insertions(+), 9 deletions(-)
> 
> ---
> 
> Changes since v1:
>  * rebased to  2.6.37-rc5
>  * introduced patch 2/3 and 3/3
> 
> This time around I tested the changes by registering a
> spi-gpio bus intance
> on the user DIP switches of the da850evm baseboard which
> are connected to a 
> pca953x-compatible I2C gpio expander. Then I did a few 
> 'echo hello > /dev/spidev0.0' commands and checked the
> behaviour of the bus
> on a scope. I was unable to test the speed of this bus
> without the patchset
> this time since using the GPIOs on the expander requires
> patch 3/3. I found 
> that the SCLK speed was roughly 300Hz where 500Hz was
> requested.
> 
> Here is a snip of the board-setup code that registers the
> spi-gpio bus:
> 
> static struct spi_gpio_platform_data bb_spi_gpio_pdata = {
>     .sck   
>     = -1 /* assigned at runtime */,
>     .mosi   
>     = -1 /* assigned at runtime */,
>     .miso   
>     = -1 /* assigned at runtime */,
>     .num_chipselect = 1,
> };
> 
> static struct platform_device bb_spi_gpio = {
>     .name   
>     = "spi_gpio",
>     .id       
> = 0,
>     .dev   
>     = {
>        
> .platform_data    = &bb_spi_gpio_pdata,
>     },
> };
> 
> static struct spi_board_info bb_spi_devices[] = {
>     {
>        
> .modalias    = "spidev",
>        
> .max_speed_hz    = 500,
>        
> .bus_num    = 0,
>        
> .chip_select    = 0,
>        
> .mode        = SPI_MODE_0,
>         .controller_data =
> (void *) -1 /* assigned at runtime */,
>     },
> };
> 
> static int bb_spi_gpio_init(unsigned gpio)
> {
>     int ret;
> 
>     bb_spi_gpio_pdata.sck = gpio +
> DA850_EVM_BB_EXP_USER_SW1;
>     bb_spi_gpio_pdata.mosi = gpio +
> DA850_EVM_BB_EXP_USER_SW2;
>     bb_spi_gpio_pdata.miso = -1 /* nothing
> connected ATM */;
> 
>     bb_spi_devices[0].controller_data =
> (void *) gpio +
>            
>            
> DA850_EVM_BB_EXP_USER_SW3;
> 
>     ret =
> spi_register_board_info(bb_spi_devices,
>            
>        
> ARRAY_SIZE(bb_spi_devices));
>     if (ret) {
>         pr_warning("could not
> register spi devices");
>         goto
> spi_gpio_fail_devices;
>     }
> 
>     ret =
> platform_device_register(&bb_spi_gpio);
>     if (ret) {
>         pr_warning("could not
> register spi-gpio driver instance");

>         goto
> spi_gpio_fail_bus;
>     }
> 
>     return 0;
> spi_gpio_fail_bus:
>     /* no unregister board info function
> available*/
> spi_gpio_fail_devices:
>     return ret;
> }
> 
> static int da850_evm_bb_expander_setup(struct i2c_client
> *client,
>            
>            
> unsigned gpio, unsigned ngpio,
>            
>            
> void *c)
> {
>     int ret;
> 
>     /*
>      * Register the switches and
> pushbutton on the baseboard as a gpio-keys
>      * device.
>      */
>     da850_evm_bb_keys_init(gpio);
>     ret =
> platform_device_register(&da850_evm_bb_keys_device);
>     if (ret) {
>         pr_warning("Could not
> register baseboard GPIO expander keys");
>         goto
> io_exp_setup_sw_fail;
>     }
> 
>     da850_evm_bb_leds_init(gpio);
>     ret =
> platform_device_register(&da850_evm_bb_leds_device);
>     if (ret) {
>         pr_warning("Could not
> register baseboard GPIO expander LEDS");
>         goto
> io_exp_setup_leds_fail;
>     }
> 
>     ret = bb_spi_gpio_init(gpio);
>     if (ret) {
>         pr_warning("Could not
> register spi-gpio bus");
>         goto
> io_exp_setup_spi_fail;
>     }
> 
>     return 0;
> 
> io_exp_setup_spi_fail:
>    
> platform_device_unregister(&da850_evm_bb_leds_device);
> io_exp_setup_leds_fail:
>    
> platform_device_unregister(&da850_evm_bb_keys_device);
> io_exp_setup_sw_fail:
>     return ret;
> }
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer
  2010-12-09 20:34   ` [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer Ben Gardiner
@ 2010-12-10 11:46     ` Wolfram Sang
  2010-12-14 16:58       ` Ben Gardiner
  0 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2010-12-10 11:46 UTC (permalink / raw)
  To: Ben Gardiner
  Cc: spi-devel-general, Grant Likely, David Brownell, linux-kernel,
	Michael Buesch

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

Hi,

thanks for the submission. It has some issues, though:

On Thu, Dec 09, 2010 at 03:34:55PM -0500, Ben Gardiner wrote:

> +		nsecs =
> +#if defined(CONFIG_SLOWER_SPI_GPIO)
> +				!cs->nsecs ? cs->nsecs : 100;
> +#else
> +		100;
> +#endif

This coding style is very hard to read and gains nothing for it. Also,
slower_spi should rather be a per-device than a config option.

Kind regards,

   Wolfram

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH v2 0/3] slower spi-gpio
  2010-12-10  4:38   ` [RFC][PATCH v2 0/3] slower spi-gpio David Brownell
@ 2010-12-14 16:58     ` Ben Gardiner
  0 siblings, 0 replies; 11+ messages in thread
From: Ben Gardiner @ 2010-12-14 16:58 UTC (permalink / raw)
  To: David Brownell, Wolfram Sang
  Cc: spi-devel-general, Grant Likely, David Brownell, linux-kernel,
	Michael Buesch

On Thu, Dec 9, 2010 at 11:38 PM, David Brownell <david-b@pacbell.net> wrote:
> You know, the spi-gpio code was written so it
> would not need Kconfiguration, and I'd like
> to see that continued.
>
> Surely the minimal configuration you'd need
> could be wrapped up in #defineslike the
> actual GPIO numbers are wrapped up in.
>
> Heck, nothing outside your patches needs your
> :slow it way the heck down" option, so there's
> no point to exposing it  via Kconfig;
> such stuff is routinely embedded in C code.

Hi David,

Thanks for your comments. I'm glad to get a better picture of what you
expect from future proposed changes to spi_gpio.

Ok. Kconfig is clearly not an acceptable way to keep spi_gpio fast for
those who want it.

> Would it make more sense to have a separate
> slowed-down veresion of the driver, maybe just
> custom defs for our hardware plus the current
> driver body, as explained in the driver code
> (last time I looked at it, anyway).

I'm hearing that a separate driver is.

> (I notice you didn't even check the GPIOs to see
> if they are sleeping calls (e.g. over I2C), which
> would have been preferable to a static always-slow
> Kconfig option.  (But not to an always-slow object
> vs the current default always-fast model.

I hadn't thought that we could check for sleeping calls, thanks for
that suggestion.

> I still need to be able to get multi-megabit
> SPI clock rates out of the standard spi-gpio
> code base.  (When I've had to use spi-gpio it
> has never been a performance issue; the code
> was written to facilitate inner bitbang loops
> of about half a dozen instructions (ARM).

Ok so current users of spi_gpio require it to operate 'as fast as it
can.' But compile-time switching in the driver is undesireable.

What I'm taking away from the discussion is that we should introduce a
second bitbanging SPI master driver that reuses as much code as
possible from the existing spi_gpio driver.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer
  2010-12-10 11:46     ` Wolfram Sang
@ 2010-12-14 16:58       ` Ben Gardiner
  2010-12-14 18:08         ` Wolfram Sang
  0 siblings, 1 reply; 11+ messages in thread
From: Ben Gardiner @ 2010-12-14 16:58 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: spi-devel-general, Grant Likely, David Brownell, linux-kernel,
	Michael Buesch

On Fri, Dec 10, 2010 at 6:46 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi,
>
> thanks for the submission. It has some issues, though:

Hi Wolfram,

Thanks for the comments, I appreciate your interest in the RFC.

> On Thu, Dec 09, 2010 at 03:34:55PM -0500, Ben Gardiner wrote:
>
>> +             nsecs =
>> +#if defined(CONFIG_SLOWER_SPI_GPIO)
>> +                             !cs->nsecs ? cs->nsecs : 100;
>> +#else
>> +             100;
>> +#endif
>
> This coding style is very hard to read and gains nothing for it. Also,
> slower_spi should rather be a per-device than a config option.

Yes, now that you mention it the implementation looks very clunky.

I think it is starting to sink-in that a 'slower' spi gpio _driver_ is
needed. I can think of a couple different ways to make the CS-to-data
delay assigned to 'nsecs' a per-device feature:
1) a flag or 2) a function pointer in struct spi_bitbang. Were you
thinking of something else entirely?

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer
  2010-12-14 16:58       ` Ben Gardiner
@ 2010-12-14 18:08         ` Wolfram Sang
  2010-12-14 18:17           ` Ben Gardiner
  0 siblings, 1 reply; 11+ messages in thread
From: Wolfram Sang @ 2010-12-14 18:08 UTC (permalink / raw)
  To: Ben Gardiner
  Cc: spi-devel-general, Grant Likely, David Brownell, linux-kernel,
	Michael Buesch

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

Hi Ben,

> I think it is starting to sink-in that a 'slower' spi gpio _driver_ is
> needed. I can think of a couple different ways to make the CS-to-data
> delay assigned to 'nsecs' a per-device feature:
> 1) a flag or 2) a function pointer in struct spi_bitbang. Were you
> thinking of something else entirely?

Nope, just saw (very high-level) that it should be per-device as it
might be feasible to have a fast and a slow bus at the same time. My
advice would be to start coding your favourite solution and check if it
feels right :)

Kind regards,

   Wolfram

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer
  2010-12-14 18:08         ` Wolfram Sang
@ 2010-12-14 18:17           ` Ben Gardiner
  0 siblings, 0 replies; 11+ messages in thread
From: Ben Gardiner @ 2010-12-14 18:17 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: spi-devel-general, Grant Likely, David Brownell, linux-kernel,
	Michael Buesch

On Tue, Dec 14, 2010 at 1:08 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Ben,
>
>> I think it is starting to sink-in that a 'slower' spi gpio _driver_ is
>> needed. I can think of a couple different ways to make the CS-to-data
>> delay assigned to 'nsecs' a per-device feature:
>> 1) a flag or 2) a function pointer in struct spi_bitbang. Were you
>> thinking of something else entirely?
>
> Nope, just saw (very high-level) that it should be per-device as it
> might be feasible to have a fast and a slow bus at the same time. My
> advice would be to start coding your favourite solution and check if it
> feels right :)

:) Sounds like good advice.

Thanks again, Wolfram.

Best Regards,
Ben Gardiner

---
Nanometrics Inc.
http://www.nanometrics.ca

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2010-12-14 18:17 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-19 22:19 [PATCH][RFC] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO Ben Gardiner
2010-12-09 20:34 ` [RFC][PATCH v2 0/3] slower spi-gpio Ben Gardiner
2010-12-09 20:34   ` [RFC][PATCH v2 1/3] spi-gpio: implement spidelay() for platforms that configure SLOWER_SPI_GPIO Ben Gardiner
2010-12-09 20:34   ` [RFC][PATCH v2 2/3] spi_bitbang : get nsecs delay from cs during transfer Ben Gardiner
2010-12-10 11:46     ` Wolfram Sang
2010-12-14 16:58       ` Ben Gardiner
2010-12-14 18:08         ` Wolfram Sang
2010-12-14 18:17           ` Ben Gardiner
2010-12-09 20:34   ` [RFC][PATCH v2 3/3] spi-gpio: use _cansleep when CONFIG_SLOWER_SPI_GPIO is defined Ben Gardiner
2010-12-10  4:38   ` [RFC][PATCH v2 0/3] slower spi-gpio David Brownell
2010-12-14 16:58     ` Ben Gardiner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).