[RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Jaya Kumar]

Hi friends,

This is v4 of batch support for gpiolib. Thanks to David Brownell, Eric Miao,
David Hylands, Robin, Ben, Jamie and others for prior feedback. The post for
v3 summarized the previous discussion. Since then the changes I've made are:
- split the patches into generic and arch specific
- optimizing the empty mask case
- adjusting the API to add error returns
- documenting this optional API in gpio.txt
- cleanup of the commenting

Please let me know your thoughts and feedback.

Thanks,
jaya

Cc: David Brownell <david-b@pacbell.net>
Cc: Eric Miao <eric.miao@marvell.com>
Cc: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: linux-arm-kernel@lists.arm.linux.org.uk
Cc: linux-fbdev-devel@lists.sourceforge.net
Cc: linux-kernel@vger.kernel.org
Cc: linux-embedded@vger.kernel.org
Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com>
---
 Documentation/gpio.txt     |   60 ++++++++
 drivers/gpio/Kconfig       |    5 +
 drivers/gpio/gpiolib.c     |  323 ++++++++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h |   19 +++-
 4 files changed, 406 insertions(+), 1 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index b1b9887..d7e5fe9 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -185,6 +185,66 @@ and not to need spinlocks.  Such optimized calls can make bitbanging
 applications a lot more efficient (in both space and time) than spending
 dozens of instructions on subroutine calls.
 
+[OPTIONAL] Spinlock-Safe GPIO Batch access
+------------------------------------------
+The original GPIO API implements single bit access to GPIO pins. Some
+drivers for devices that treat GPIO as a mechanism for bulk data transfer
+may find the performance of the single bit API to be inadequate. In this
+scenario, the user can consider enabling the batch access API. This API is
+as follows:
+
+	/* BATCH GPIO INPUT */
+int gpio_get_batch(unsigned gpio, u32 bitmask, int maskwidth, u32 *result);
+
+The following examples help explain how this function is to be used.
+ Q: How to get gpio pins 0 through 7? (8 bits)
+ A: gpio_get_batch(gpio=0, bitmask=0xFF, width=8, &result) result=0xnn
+ Q: How to get gpio pins 58 through 73? (16 bits)
+ A: gpio_get_batch(gpio=58, bitmask=0xFFFF, width=16, &result) result=0xnnnn
+ Q: How to get gpio pins 16 through 47? (32 bits)
+ A: gpio_get_batch(offset=16, bitmask=0xFFFFFFFF, width=32, &result)
+ A: result=0xnnnnnnnn
+
+	/* BATCH GPIO OUTPUT */
+int gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int maskwidth);
+
+The following examples help explain how this function is to be used.
+ Q: How to set gpio pins 0 through 7 to all 0? (8 bits)
+ A: gpio_set_batch(gpio=0, values=0x0, bitmask=0xFF, width=8);
+ Q: How to set gpio pins 58 through 73 to all 1? (16 bits)
+ A: gpio_set_batch(gpio=58, values=0xFFFF, bitmask=0xFFFF, width=16);
+ Q: How to set gpio pins 16 through 47 to 0xCAFEC001? (32 bits)
+ A: gpio_set_batch(gpio=16, values=0xCAFEC001, bitmask=0xFFFFFFFF, width=32);
+
+The following example shows the use of the batch API and a comparison with
+the original single bit API:
+
+Original input method which loops through a set of pins:
+-       for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++)
+-               res |= (gpio_get_value(DB0_GPIO_PIN + i)) ? (1 << i) : 0;
+
+Batch input method:
++       u16 val;
++       err = gpio_get_batch(DB0_GPIO_PIN, 0xFFFF, 16, &val);
+
+Original output method:
+-       for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++)
+-               gpio_set_value(DB0_GPIO_PIN + i, (data >> i) & 0x01);
+Batch output method:
++       int err;
++       err = gpio_set_batch(DB0_GPIO_PIN, data, 0xFFFF, 16);
+
+Using these calls for GPIOs that can't safely be accessed without sleeping
+(see below) is an error.
+
+Platform-specific implementations are encouraged to optimize the two
+calls by checking if the batch get/set requested can be achieved within the
+platform's fast path access to gpio registers. For example, if the starting
+gpio and width of bits to be written is contained within a single register
+then the platform specific implementation may choose to execute it. If the
+request is more complex, then the platform specific implementation can
+choose to call upon the platform independent __gpio_get/set_batch functions
+which are able to cross gpio_chip boundaries.
 
 GPIO access that may sleep
 --------------------------
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3d25654..474070b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -37,6 +37,11 @@ menuconfig GPIOLIB
 
 if GPIOLIB
 
+config GPIOLIB_BATCH
+	bool "Batch GPIO support"
+	help
+	  Say Y here to add the capability to batch set/get GPIOs.
+
 config DEBUG_GPIO
 	bool "Debug GPIO calls"
 	depends on DEBUG_KERNEL
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 35e7aea..a9cf75e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -643,6 +643,323 @@ static inline void gpiochip_unexport(struct gpio_chip *chip)
 
 #endif /* CONFIG_GPIO_SYSFS */
 
+#ifdef CONFIG_GPIOLIB_BATCH
+/**
+ * __generic_gpio_set_batch() - Set batch of gpio pins in provided gpio_chip.
+ * @chip: gpio_chip containing this set of pins
+ * @offset: starting gpio pin
+ * @values: values to assign including masked bits
+ * @bitmask: the bitmask to be applied to values
+ * @width: the width of the bitmask
+ * Context: any
+ *
+ * This provides a generic platform independent set_batch capability.
+ * It invokes the associated gpio_chip.set() method to actually set the
+ * value. gpio_chip-s that don't implement their own optimized
+ * set_batch function are assigned this function as a default.
+ *
+ * The following examples help explain how this function works.
+ * Q: How to set gpio pins at offset 0 through 7 to all 0? (8 bits)
+ * A: offset=0, values=0x0, bitmask=0xFF, width=8
+ * Q: How to set gpio pins at offset 26 through 31 to all 1? (6 bits)
+ * A: offset=26, values=0x3F, bitmask=0x3F, width=6
+ * Q: How to set gpio pins at offset 16 through 31 to 0xCAFE? (16 bits)
+ * A: offset=16, values=0xCAFE, bitmask=0xFFFF, width=16
+ * Q: Why isn't width calculated from the mask here?
+ * A: This function is intended to be called repetitively and doing the
+ * bit shift looping to calculate the width here would be an undesirable
+ * penalty. Instead, the width is provided by the caller, thus avoiding
+ * the performance penalty since it is a fixed value known by the caller.
+ *
+ * Returns a negative errno if the caller supplied bad data, such as
+ * offset or width in excess of this chips gpio. Otherwise, we return zero
+ * as a success code.
+ */
+static int __generic_gpio_set_batch(struct gpio_chip *chip, unsigned offset,
+					u32 values, u32 bitmask, int width)
+{
+	int i;
+	int value;
+	u32 mask;
+
+	/*
+	 * If the caller attempted to exceed the number of gpios that
+	 * are in this chip, then we flag that as an invalid value for
+	 * either the offset or the width supplied.
+	 */
+	if (offset + width >  chip->ngpio)
+		return -EINVAL;
+
+	/*
+	 * We start the loop and continue till we reach the width
+	 * of the bitmask that is provided to us.
+	 */
+	for (i = 0; i < width; i++) {
+		/*
+		 * If this bit is enabled by the bitmask then
+		 * we perform the set. If it is disabled we leave
+		 * it alone.
+		 */
+		mask = 1 << i;
+		if (bitmask & mask) {
+			value = values & mask;
+			chip->set(chip, offset + i, value);
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * __generic_gpio_get_batch() - Get batch of gpio pins in provided gpio_chip.
+ * @chip: gpio_chip containing this set of pins
+ * @offset: starting gpio pin
+ * @values: values to assign including masked bits
+ * @bitmask: the bitmask to be applied to values
+ * @width: the width of the bitmask
+ * @result: the result to be returned
+ * Context: any
+ *
+ * This provides a generic platform independent get_batch capability.
+ * It invokes the associated gpio_chip.get() method to actually set the
+ * value. gpio_chip-s that don't implement their own optimized
+ * get_batch function are assigned this function as a default.
+ *
+ * The following examples help explain how this function works.
+ * Q: How to get gpio pins at offset 0 through 7? (8 bits)
+ * A: offset=0, bitmask=0xFF, width=8, result=0xnn
+ * Q: How to get gpio pins at offset 26 through 31? (6 bits)
+ * A: offset=26, bitmask=0x3F, width=6, result=0xnn
+ * Q: How to get gpio pins at offset 16 through 31? (16 bits)
+ * A: offset=16, bitmask=0xFFFF, width=16, result=0xnnnn
+ * Q: Why isn't width calculated from the mask here?
+ * A: This function is intended to be called repetitively and doing the
+ * bit shift looping to calculate the width here would be an undesirable
+ * penalty. Instead, the width is provided by the caller, thus avoiding
+ * the performance penalty since it is a fixed value known by the caller.
+ *
+ * Returns a negative errno if the caller supplied bad data, such as
+ * offset or width in excess of this chips gpio. Otherwise, we return zero
+ * as a success code.
+ * Context: any
+ */
+static int __generic_gpio_get_batch(struct gpio_chip *chip, unsigned offset,
+					u32 bitmask, int width, u32 *result)
+{
+	int i;
+	u32 mask;
+	u32 values = 0;
+
+	/*
+	 * If the caller attempted to exceed the number of gpios that
+	 * are in this chip, then we flag that as an invalid value for
+	 * either the offset or the width supplied.
+	 */
+	if (offset + width >  chip->ngpio)
+		return -EINVAL;
+
+	/*
+	 * We start the loop and continue till we reach the width
+	 * of the bitmask that is provided to us.
+	 */
+	for (i = 0; i < width; i++) {
+		/*
+		 * If this bit is enabled by the bitmask then
+		 * we perform the get. If it is disabled we leave
+		 * it alone thus leaving it as 0 because we initialized
+		 * values.
+		 */
+		mask = 1 << i;
+		if (bitmask & mask) {
+			if (chip->get(chip, offset + i))
+				values |= mask;
+		}
+	}
+
+	*result = values;
+	return 0;
+}
+
+/**
+ * __gpio_set_batch() - set batch of gpio pins across multiple gpio_chip-s
+ * @gpio: starting gpio pin
+ * @values: values to assign including masked bits
+ * @bitmask: the bitmask to be applied to values
+ * @bitwidth: the width of the bitmask
+ * Context: any
+ *
+ * This function is platform independent and uses the starting gpio and
+ * bitwidth information to iterate through the set of required gpio_chips
+ * to use their set_batch capability in order to set a batch of gpio pins.
+ * This function handles going across gpio_chip boundaries. It is intended
+ * to be called from arch/mach specific code if they detect that the caller
+ * requires functionality outside the fast-path.
+ *
+ * The following examples help explain how this function is to be used.
+ * Q: How to set gpio pins 0 through 7 to all 0? (8 bits)
+ * A: gpio=0, values=0x0, bitmask=0xFF, width=8
+ * Q: How to set gpio pins 58 through 73 to all 1? (16 bits)
+ * A: gpio=58, values=0xFFFF, bitmask=0xFFFF, width=16
+ * Q: How to set gpio pins 16 through 47 to 0xCAFEC001? (32 bits)
+ * A: offset=16, values=0xCAFEC001, bitmask=0xFFFFFFFF, width=32
+ * Q: Why isn't width calculated from the mask here?
+ * A: This function is intended to be called repetitively and doing the
+ * bit shift looping to calculate the width here would be an undesirable
+ * penalty. Instead, the width is provided by the caller, thus avoiding
+ * the performance penalty since it is a fixed value known by the caller.
+ *
+ * Returns a negative errno if the caller supplied bad data, such as
+ * gpio or width in excess of the platforms max gpio. Otherwise, we return
+ * zero as a success code.
+ *
+ */
+int __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth)
+{
+	struct gpio_chip *chip;
+	int i = 0;
+	int value, width, remwidth;
+	u32 mask;
+	int ret;
+
+	/*
+	 * If the caller attempted to exceed the number of gpios that
+	 * are available here, then we flag that as an invalid value for
+	 * either the gpio or the width supplied.
+	 */
+	if ((bitwidth > 32) || (gpio + bitwidth >  ARCH_NR_GPIOS))
+		return -EINVAL;
+
+	do {
+		chip = gpio_to_chip(gpio + i);
+		WARN_ON(extra_checks && chip->can_sleep);
+
+		value = values >> i; /* shift off the used data bits */
+
+		/* Work out the remaining width in this chip. */
+		remwidth = ((chip->base + (int) chip->ngpio) -
+					((int) gpio + i));
+
+		/*
+		 * Check if the remaining bits to be handled are less than
+		 * the remaining width in this chip.
+		 */
+		width = min(bitwidth, remwidth);
+
+		/* Shift off the used mask bits. */
+		mask = bitmask >> i;
+
+		/* Now adjust mask by width of this set. */
+		mask &= ((1 << width) - 1);
+
+		/* If the mask is empty, then we can skip this chip. */
+		if (mask) {
+			ret = chip->set_batch(chip, gpio + i - chip->base,
+						value, mask, width);
+			if (ret)
+				return ret;
+		}
+
+		/* deduct the used bits from our todolist */
+		i += width;
+		bitwidth -= width;
+	} while (bitwidth);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__gpio_set_batch);
+
+/**
+ * __gpio_get_batch() - get batch of gpio pins across multiple gpio_chip-s
+ * @gpio: starting gpio pin
+ * @bitmask: the bitmask to be applied to values
+ * @bitwidth: the width of the bitmask
+ * @result: returned values
+ * Context: any
+ *
+ * This function is platform independent and uses the starting gpio and
+ * bitwidth information to iterate through the set of required gpio_chips
+ * to use their get_batch capability in order to get a batch of gpio pins.
+ * This function handles going across gpio_chip boundaries. It is intended
+ * to be called from arch/mach specific code if they detect that the caller
+ * requires functionality outside the fast-path.
+ *
+ * The following examples help explain how this function is to be used.
+ * Q: How to get gpio pins 0 through 7? (8 bits)
+ * A: gpio=0, bitmask=0xFF, width=8, result=0xnn
+ * Q: How to get gpio pins 58 through 73? (16 bits)
+ * A: gpio=58, bitmask=0xFFFF, width=16, result=0xnnnn
+ * Q: How to get gpio pins 16 through 47? (32 bits)
+ * A: offset=16, bitmask=0xFFFFFFFF, width=32, result=0xnnnnnnnn
+ * Q: Why isn't width calculated from the mask here?
+ * A: This function is intended to be called repetitively and doing the
+ * bit shift looping to calculate the width here would be an undesirable
+ * penalty. Instead, the width is provided by the caller, thus avoiding
+ * the performance penalty since it is a fixed value known by the caller.
+ *
+ * Returns a negative errno if the caller supplied bad data, such as
+ * gpio or width in excess of the platforms max gpio. Otherwise, we return
+ * zero as a success code
+ *
+ */
+int __gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth, u32 *result)
+{
+	struct gpio_chip *chip;
+	int i = 0;
+	int width, remwidth;
+	u32 mask;
+	u32 values = 0;
+	u32 value;
+	int ret;
+
+	/*
+	 * If the caller attempted to exceed the number of gpios that
+	 * are available here, then we flag that as an invalid value for
+	 * either the gpio or the width supplied.
+	 */
+	if ((bitwidth > 32) || (gpio + bitwidth >  ARCH_NR_GPIOS))
+		return -EINVAL;
+
+	do {
+		chip = gpio_to_chip(gpio + i);
+		WARN_ON(extra_checks && chip->can_sleep);
+
+		/* Work out the remaining width in this chip. */
+		remwidth = ((chip->base + (int) chip->ngpio) -
+					((int) gpio + i));
+
+		/*
+		 * Check if the remaining bits to be handled are less than
+		 * the remaining width in this chip.
+		 */
+		width = min(bitwidth, remwidth);
+
+		/* shift off the used mask bits */
+		mask = bitmask >> i;
+		/* now adjust mask by width of get */
+		mask &= ((1 << width) - 1);
+
+		/* If the mask is empty, then we can skip this chip. */
+		if (mask) {
+			ret = chip->get_batch(chip, gpio + i - chip->base,
+						mask, width, &value);
+			if (ret)
+				return ret;
+
+			/* shift result back into correct position */
+			values |= value << i;
+		}
+
+		/* deduct the used bits from our todolist */
+		i += width;
+		bitwidth -= width;
+	} while (bitwidth);
+
+	*result = values;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__gpio_get_batch);
+#endif
+
 /**
  * gpiochip_add() - register a gpio_chip
  * @chip: the chip to register, with chip->base initialized
@@ -683,6 +1000,12 @@ int gpiochip_add(struct gpio_chip *chip)
 		}
 		chip->base = base;
 	}
+#ifdef CONFIG_GPIOLIB_BATCH
+	if (!chip->set_batch)
+		chip->set_batch = __generic_gpio_set_batch;
+	if (!chip->get_batch)
+		chip->get_batch = __generic_gpio_get_batch;
+#endif
 
 	/* these GPIO numbers must not be managed by another gpio_chip */
 	for (id = base; id < base + chip->ngpio; id++) {
diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
index 81797ec..29478d2 100644
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -44,6 +44,10 @@ struct module;
  *	returns either the value actually sensed, or zero
  * @direction_output: configures signal "offset" as output, or returns error
  * @set: assigns output value for signal "offset"
+ * @set_batch: batch assigns output values for signals starting at
+ *	"offset" with mask in "bitmask" all within this gpio_chip
+ * @get_batch: batch fetches values for consecutive signals starting at
+ *	"offset" with mask in "bitmask" all within this gpio_chip
  * @to_irq: optional hook supporting non-static gpio_to_irq() mappings;
  *	implementation may not sleep
  * @dbg_show: optional routine to show contents in debugfs; default code
@@ -84,7 +88,14 @@ struct gpio_chip {
 						unsigned offset, int value);
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
-
+#ifdef CONFIG_GPIOLIB_BATCH
+	int			(*set_batch)(struct gpio_chip *chip,
+						unsigned offset, u32 values,
+						u32 bitmask, int width);
+	int			(*get_batch)(struct gpio_chip *chip,
+						unsigned offset, u32 bitmask,
+						int width, u32 *result);
+#endif
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
 
@@ -124,6 +135,12 @@ extern void gpio_set_value_cansleep(unsigned gpio, int value);
  */
 extern int __gpio_get_value(unsigned gpio);
 extern void __gpio_set_value(unsigned gpio, int value);
+#ifdef CONFIG_GPIOLIB_BATCH
+extern int __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask,
+				int bitwidth);
+extern int __gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth,
+				u32 *result);
+#endif
 
 extern int __gpio_cansleep(unsigned gpio);
 
-- 
1.5.2.3

[RFC 2.6.28 2/2] mach-pxa: add and use batch set/get gpio

[Jaya Kumar]

This patch adds support for pxa specific batch set/get of gpio. This
is exported to gpiolib via the gpio_chip interface and then used within
am300epd.c

Cc: David Brownell <david-b@pacbell.net>
Cc: Eric Miao <eric.miao@marvell.com>
Cc: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: linux-arm-kernel@lists.arm.linux.org.uk
Cc: linux-fbdev-devel@lists.sourceforge.net
Cc: linux-kernel@vger.kernel.org
Cc: linux-embedded@vger.kernel.org
Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com>
---
 arch/arm/mach-pxa/Kconfig             |    1 +
 arch/arm/mach-pxa/am300epd.c          |   23 ++++++------
 arch/arm/mach-pxa/gpio.c              |   63 +++++++++++++++++++++++++++++++++
 arch/arm/mach-pxa/include/mach/gpio.h |   48 +++++++++++++++++++++++++
 4 files changed, 124 insertions(+), 11 deletions(-)

diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig
index f844425..1b3e631 100644
--- a/arch/arm/mach-pxa/Kconfig
+++ b/arch/arm/mach-pxa/Kconfig
@@ -41,6 +41,7 @@ config GUMSTIX_AM200EPD
 	bool "Enable AM200EPD board support"
 
 config GUMSTIX_AM300EPD
+	select GPIOLIB_BATCH
 	bool "Enable AM300EPD board support"
 
 endchoice
diff --git a/arch/arm/mach-pxa/am300epd.c b/arch/arm/mach-pxa/am300epd.c
index 4bd10a1..c065368 100644
--- a/arch/arm/mach-pxa/am300epd.c
+++ b/arch/arm/mach-pxa/am300epd.c
@@ -187,24 +187,25 @@ static void am300_cleanup(struct broadsheetfb_par *par)
 
 static u16 am300_get_hdb(struct broadsheetfb_par *par)
 {
-	u16 res = 0;
-	int i;
-
-	for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++)
-		res |= (gpio_get_value(DB0_GPIO_PIN + i)) ? (1 << i) : 0;
+	int err;
+	u32 val;
 
-	return res;
+	err = gpio_get_batch(DB0_GPIO_PIN, 0xFFFF, 16, &val);
+	if (err) {
+		dev_err(&am300_device->dev, "get failed: %d\n", err);
+		return 0;
+	}
+	return (u16) val;
 }
 
 static void am300_set_hdb(struct broadsheetfb_par *par, u16 data)
 {
-	int i;
-
-	for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++)
-		gpio_set_value(DB0_GPIO_PIN + i, (data >> i) & 0x01);
+	int err;
+	err = gpio_set_batch(DB0_GPIO_PIN, data, 0xFFFF, 16);
+	if (err)
+		dev_err(&am300_device->dev, "set failed: %d\n", err);
 }
 
-
 static void am300_set_ctl(struct broadsheetfb_par *par, unsigned char bit,
 				u8 state)
 {
diff --git a/arch/arm/mach-pxa/gpio.c b/arch/arm/mach-pxa/gpio.c
index 5fec1e4..46371a1 100644
--- a/arch/arm/mach-pxa/gpio.c
+++ b/arch/arm/mach-pxa/gpio.c
@@ -162,6 +162,67 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 		__raw_writel(mask, pxa->regbase + GPCR_OFFSET);
 }
 
+#ifdef CONFIG_GPIOLIB_BATCH
+/*
+ * Set output GPIO level in batches
+ */
+static int pxa_gpio_set_batch(struct gpio_chip *chip, unsigned offset,
+				u32 values, u32 bitmask, int width)
+{
+	struct pxa_gpio_chip *pxa;
+
+	/* we're guaranteed by the caller that offset + bitmask remains
+	 * in this chip.
+	 */
+	pxa = container_of(chip, struct pxa_gpio_chip, chip);
+
+	/* bitmask is used twice in shifted form */
+	bitmask <<= offset;
+
+	values = (values << offset) & bitmask;
+	if (values)
+		__raw_writel(values, pxa->regbase + GPSR_OFFSET);
+
+	values = ~values & bitmask;
+	if (values)
+		__raw_writel(values, pxa->regbase + GPCR_OFFSET);
+
+	return 0;
+}
+
+/*
+ * Get output GPIO level in batches
+ */
+static int pxa_gpio_get_batch(struct gpio_chip *chip, unsigned offset,
+				u32 bitmask, int width, u32 *result)
+{
+	u32 values;
+	struct pxa_gpio_chip *pxa;
+
+	/* we're guaranteed by the caller that offset + bitmask remains
+	 * in this chip.
+	 */
+	pxa = container_of(chip, struct pxa_gpio_chip, chip);
+
+	values = __raw_readl(pxa->regbase + GPLR_OFFSET);
+
+	/* shift the result back into original position */
+	*result = (values >> offset) & bitmask;
+	return 0;
+}
+#endif
+
+/* this is done this way so that we can insert an ifdef-able value
+ * into the following GPIO_CHIP macro
+ */
+#ifdef CONFIG_GPIOLIB_BATCH
+#define SET_BATCH_MACRO	.set_batch 	  = pxa_gpio_set_batch,
+#define GET_BATCH_MACRO	.get_batch	  = pxa_gpio_get_batch,
+#else
+#define SET_BATCH_MACRO
+#define GET_BATCH_MACRO
+#endif
+
 #define GPIO_CHIP(_n)							\
 	[_n] = {							\
 		.regbase = GPIO##_n##_BASE,				\
@@ -173,6 +234,8 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 			.set		  = pxa_gpio_set,		\
 			.base		  = (_n) * 32,			\
 			.ngpio		  = 32,				\
+			SET_BATCH_MACRO					\
+			GET_BATCH_MACRO					\
 		},							\
 	}
 
diff --git a/arch/arm/mach-pxa/include/mach/gpio.h b/arch/arm/mach-pxa/include/mach/gpio.h
index 2c538d8..46d9022 100644
--- a/arch/arm/mach-pxa/include/mach/gpio.h
+++ b/arch/arm/mach-pxa/include/mach/gpio.h
@@ -56,6 +56,54 @@ static inline void gpio_set_value(unsigned gpio, int value)
 	}
 }
 
+#ifdef CONFIG_GPIOLIB_BATCH
+static inline int gpio_set_batch(unsigned gpio, u32 values, u32 bitmask,
+					int bitwidth)
+{
+	int err = 0;
+	if (__builtin_constant_p(gpio) &&
+		(gpio + bitwidth < NR_BUILTIN_GPIO) &&
+		((gpio + bitwidth) <= roundup(gpio+1, 32))) {
+		int shift;
+
+		shift = gpio % 32;
+		bitmask <<= shift;
+
+		values = (values << shift) & bitmask;
+		if (values)
+			GPSR(gpio) = values;
+
+		values = ~values & bitmask;
+		if (values)
+			GPCR(gpio) = values;
+	} else {
+		err = __gpio_set_batch(gpio, values, bitmask, bitwidth);
+	}
+	return err;
+}
+
+static inline int gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth,
+					u32 *result)
+{
+	int ret = 0;
+	if (__builtin_constant_p(gpio) &&
+		(gpio + bitwidth < NR_BUILTIN_GPIO) &&
+		((gpio + bitwidth) <= roundup(gpio+1, 32))) {
+		int shift;
+		u32 values;
+
+		shift = gpio % 32;
+
+		values = GPLR(gpio);
+
+		*result = (values >> shift) & bitmask;
+	} else {
+		ret = __gpio_get_batch(gpio, bitmask, bitwidth, result);
+	}
+	return ret;
+}
+#endif
+
 #define gpio_cansleep __gpio_cansleep
 
 #define gpio_to_irq(gpio)	IRQ_GPIO(gpio)
-- 
1.5.2.3

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Ryan Mallon]

Jaya Kumar wrote:
> Hi friends,
> 

> +
> +	/* BATCH GPIO OUTPUT */
> +int gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int maskwidth);
> +
> +The following examples help explain how this function is to be used.
> + Q: How to set gpio pins 0 through 7 to all 0? (8 bits)
> + A: gpio_set_batch(gpio=0, values=0x0, bitmask=0xFF, width=8);
> + Q: How to set gpio pins 58 through 73 to all 1? (16 bits)
> + A: gpio_set_batch(gpio=58, values=0xFFFF, bitmask=0xFFFF, width=16);
> + Q: How to set gpio pins 16 through 47 to 0xCAFEC001? (32 bits)
> + A: gpio_set_batch(gpio=16, values=0xCAFEC001, bitmask=0xFFFFFFFF, width=32);
> +

Can the gpio_set_batch function be used to set non-consecutive gpios?
For example:

  gpio_set_batch(0, 0x0, 0x88, 8);

To clear gpios 3 and 7? It looks like the pxa implementation will
support this, but can it be guaranteed for other architectures? If so,
can we put an example in the documentation. If not, can we make it clear
that you shouldn't do this in the documentation. Also , in the latter
case is it necessary to pass the bitmask, since it will just be ((1 <<
bitwidth) - 1)?

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

       Ryan Mallon                              Unit 5, Amuri Park
       Phone: +64 3 3779127                     404 Barbadoes St
       Fax:   +64 3 3779135                     PO Box 13 889
       Email: ryan@bluewatersys.com             Christchurch, 8013
       Web:   http://www.bluewatersys.com       New Zealand
       Freecall Australia  1800 148 751         USA 1800 261 2934

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Jaya Kumar]

On Mon, Jan 19, 2009 at 4:05 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
> Jaya Kumar wrote:
>> Hi friends,
>>
>
>> +
>> +     /* BATCH GPIO OUTPUT */
>> +int gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int maskwidth);
>> +
>> +The following examples help explain how this function is to be used.
>> + Q: How to set gpio pins 0 through 7 to all 0? (8 bits)
>> + A: gpio_set_batch(gpio=0, values=0x0, bitmask=0xFF, width=8);
>> + Q: How to set gpio pins 58 through 73 to all 1? (16 bits)
>> + A: gpio_set_batch(gpio=58, values=0xFFFF, bitmask=0xFFFF, width=16);
>> + Q: How to set gpio pins 16 through 47 to 0xCAFEC001? (32 bits)
>> + A: gpio_set_batch(gpio=16, values=0xCAFEC001, bitmask=0xFFFFFFFF, width=32);
>> +
>
> Can the gpio_set_batch function be used to set non-consecutive gpios?
> For example:
>
>  gpio_set_batch(0, 0x0, 0x88, 8);
>
> To clear gpios 3 and 7? It looks like the pxa implementation will

Hi Ryan,

For the first part, yes, it can do non-consecutive gpios by using the
mask. Pins 3 and 7 are handled using a 5-bit mask. You'd  do
gpio_set_batch(3 <- starting pin is gpio 3, 0x0 <- clear, 0x1F <-
mask, 5 <- bit width of mask);

> support this, but can it be guaranteed for other architectures? If so,

That's a tough question. My basic answer would be yes because I've
provided the generic set_batch handler that just uses single bit sets
to achieve it. See __generic_gpio_set_batch() in patch). But if your
question is deeper, ie: can it be optimized for other architectures? ;
then I think I have to handwave a bit. I believe it can, as most of
the CPUs I've seen expose gpio via a set register and clear register
with variation between a context register to select which bank/module
of gpios is being accessed versus just having a pool of set and clear
registers (like pxa). If a CPU didn't have this and just had a blanket
gpio register then the implementation would have to first read the
previous register value (or cache it) in order to support doing the
mask.

> can we put an example in the documentation. If not, can we make it clear

Good point. I'll put it in the docs.

> that you shouldn't do this in the documentation. Also , in the latter
> case is it necessary to pass the bitmask, since it will just be ((1 <<
> bitwidth) - 1)?

Yes, it is as you have non-consecutive bits. In the above case the
mask width is 5 bits, but 3 bits are masked off, so we need the mask
so that the caller can tell us which bits are masked off.

Thanks,
jaya

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Jaya Kumar]

On Mon, Jan 19, 2009 at 7:46 AM, Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> On Mon, Jan 19, 2009 at 4:05 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>> Jaya Kumar wrote:
>>> Hi friends,
>>>
>>
>>> +
>>> +     /* BATCH GPIO OUTPUT */
>>> +int gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int maskwidth);
>>> +
>>> +The following examples help explain how this function is to be used.
>>> + Q: How to set gpio pins 0 through 7 to all 0? (8 bits)
>>> + A: gpio_set_batch(gpio=0, values=0x0, bitmask=0xFF, width=8);
>>> + Q: How to set gpio pins 58 through 73 to all 1? (16 bits)
>>> + A: gpio_set_batch(gpio=58, values=0xFFFF, bitmask=0xFFFF, width=16);
>>> + Q: How to set gpio pins 16 through 47 to 0xCAFEC001? (32 bits)
>>> + A: gpio_set_batch(gpio=16, values=0xCAFEC001, bitmask=0xFFFFFFFF, width=32);
>>> +
>>
>> Can the gpio_set_batch function be used to set non-consecutive gpios?
>> For example:
>>
>>  gpio_set_batch(0, 0x0, 0x88, 8);
>>
>> To clear gpios 3 and 7? It looks like the pxa implementation will
>
> Hi Ryan,
>
> For the first part, yes, it can do non-consecutive gpios by using the
> mask. Pins 3 and 7 are handled using a 5-bit mask. You'd  do
> gpio_set_batch(3 <- starting pin is gpio 3, 0x0 <- clear, 0x1F <-
> mask, 5 <- bit width of mask);

Correction: I meant to write 0x11 for the mask above (instead of 0x1F)
since you only want to clear the starting pin 3 and the ending pin 7
in the 5 bits. If we used 0x1F here, then we would clear all 5 bits
rather than just 3 and 7.

Thanks,
jaya

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Ryan Mallon]

Jaya Kumar wrote:
> On Mon, Jan 19, 2009 at 4:05 AM, Ryan Mallon <ryan@bluewatersys.com> wrote:
>> Jaya Kumar wrote:
>>> Hi friends,
>>>
>>> +
>>> +     /* BATCH GPIO OUTPUT */
>>> +int gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int maskwidth);
>>> +
>>> +The following examples help explain how this function is to be used.
>>> + Q: How to set gpio pins 0 through 7 to all 0? (8 bits)
>>> + A: gpio_set_batch(gpio=0, values=0x0, bitmask=0xFF, width=8);
>>> + Q: How to set gpio pins 58 through 73 to all 1? (16 bits)
>>> + A: gpio_set_batch(gpio=58, values=0xFFFF, bitmask=0xFFFF, width=16);
>>> + Q: How to set gpio pins 16 through 47 to 0xCAFEC001? (32 bits)
>>> + A: gpio_set_batch(gpio=16, values=0xCAFEC001, bitmask=0xFFFFFFFF, width=32);
>>> +
>> Can the gpio_set_batch function be used to set non-consecutive gpios?
>> For example:
>>
>>  gpio_set_batch(0, 0x0, 0x88, 8);
>>
>> To clear gpios 3 and 7? It looks like the pxa implementation will
> 
> Hi Ryan,
> 
> For the first part, yes, it can do non-consecutive gpios by using the
> mask. Pins 3 and 7 are handled using a 5-bit mask. You'd  do
> gpio_set_batch(3 <- starting pin is gpio 3, 0x0 <- clear, 0x1F <-
> mask, 5 <- bit width of mask);
> 
>> support this, but can it be guaranteed for other architectures? If so,
> 
> That's a tough question. My basic answer would be yes because I've
> provided the generic set_batch handler that just uses single bit sets
> to achieve it. See __generic_gpio_set_batch() in patch). But if your
> question is deeper, ie: can it be optimized for other architectures? ;
> then I think I have to handwave a bit. I believe it can, as most of
> the CPUs I've seen expose gpio via a set register and clear register
> with variation between a context register to select which bank/module
> of gpios is being accessed versus just having a pool of set and clear
> registers (like pxa). If a CPU didn't have this and just had a blanket
> gpio register then the implementation would have to first read the
> previous register value (or cache it) in order to support doing the
> mask.

The EP93xx has a single register for reading, set and clearing the
states of gpios, so it would have to be read, masked and written. I'm
not sure if that would be quicker than doing a for-loop over the 8-bits
in the register (the EP93xx has 8 register banks of 8 gpios, each with
their own reigster set). I guess that even if it cannot be optimised for
a given architecture, then the standard for-loop method can be used. As
long as it is documented that non-consecutive setting should work so
that arch implementers can get it correct.

>> can we put an example in the documentation. If not, can we make it clear
> 
> Good point. I'll put it in the docs.

Cool, thanks.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

       Ryan Mallon                              Unit 5, Amuri Park
       Phone: +64 3 3779127                     404 Barbadoes St
       Fax:   +64 3 3779135                     PO Box 13 889
       Email: ryan@bluewatersys.com             Christchurch, 8013
       Web:   http://www.bluewatersys.com       New Zealand
       Freecall Australia  1800 148 751         USA 1800 261 2934

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Uwe Kleine-König]

Hi Jaya,

On Sat, Jan 17, 2009 at 05:57:17PM +0800, Jaya Kumar wrote:
> Hi friends,
> 
> This is v4 of batch support for gpiolib. Thanks to David Brownell, Eric Miao,
> David Hylands, Robin, Ben, Jamie and others for prior feedback. The post for
> v3 summarized the previous discussion. Since then the changes I've made are:
> - split the patches into generic and arch specific
IMHO this should be three patches: "gpiolib", "pxa" and "am300epd".
Well, ...

> +[OPTIONAL] Spinlock-Safe GPIO Batch access
Is it really spinlock safe in general?  Or only if gpio_cansleep(gpio)
if false for each gpio to get or set?

> +static int __generic_gpio_set_batch(struct gpio_chip *chip, unsigned offset,
> +					u32 values, u32 bitmask, int width)
IMHO a better name is __gpio_set_batch_generic (or
__gpiolib_set_batch_generic?), YMMV.

> +int __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth)
Sometimes your width parameter is called bitwidth, sometimes width.  I'd
like to have that consistant.

While talking about this parameter.  I don't really like it, because you
can calculate it from bitmask.  In an earlier mail you write:

	bitwidth (needed to iterate and map to chip ngpios) could be
	calculated from bitmask, but that involves iteratively counting
	bits from the mask, so we would have to do 800*600 bit counts.
	Unless, we do ugly things like cache the previous bitwidth/mask
	and compare against the current caller arguments. Given that the
	bitwidth would typically be a fixed value, I believe it is more
	intuitive for the caller to provide it, ...

I think it's easier than that.  bitwidth is just fls(bitmask) which
should be efficient enough not to bother the programmer.  If bitmask is
constant it's even the compiler that does the work here.

BTW, I wonder why the argument to fls has type int and not unsigned.

Best regards,
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |
Peiner Strasse 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686              | Fax:   +49-5121-206917-5555 |

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Jaya Kumar]

On Mon, Jan 19, 2009 at 6:03 PM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Hi Jaya,
>
> On Sat, Jan 17, 2009 at 05:57:17PM +0800, Jaya Kumar wrote:
>> Hi friends,
>>
>> This is v4 of batch support for gpiolib. Thanks to David Brownell, Eric Miao,
>> David Hylands, Robin, Ben, Jamie and others for prior feedback. The post for
>> v3 summarized the previous discussion. Since then the changes I've made are:
>> - split the patches into generic and arch specific
> IMHO this should be three patches: "gpiolib", "pxa" and "am300epd".
> Well, ...

Hi Uwe,

Ok, will do.

>
>> +[OPTIONAL] Spinlock-Safe GPIO Batch access
> Is it really spinlock safe in general?  Or only if gpio_cansleep(gpio)
> if false for each gpio to get or set?

You are correct to raise this issue. It is only spinlock safe if
chip->cansleep is false. Initially, I wasn't sure what to do. The
original gpio set/get_value() just does;
        WARN_ON(extra_checks && chip->can_sleep);
and it is documented as:

"
Spinlock-Safe GPIO access
-------------------------
<snip>
return zero.  Also, using these calls for GPIOs that can't safely be accessed
without sleeping (see below) is an error.
"

I will change this in the batch code to return an error if can_sleep
is detected on any involved gpio_chip.

>
>> +static int __generic_gpio_set_batch(struct gpio_chip *chip, unsigned offset,
>> +                                     u32 values, u32 bitmask, int width)
> IMHO a better name is __gpio_set_batch_generic (or
> __gpiolib_set_batch_generic?), YMMV.

Agreed. Will change.

>
>> +int __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth)
> Sometimes your width parameter is called bitwidth, sometimes width.  I'd
> like to have that consistant.

Ok, you're right. I'll fix this.

>
> While talking about this parameter.  I don't really like it, because you
> can calculate it from bitmask.  In an earlier mail you write:

Agreed.

>
>        bitwidth (needed to iterate and map to chip ngpios) could be
>        calculated from bitmask, but that involves iteratively counting
>        bits from the mask, so we would have to do 800*600 bit counts.
>        Unless, we do ugly things like cache the previous bitwidth/mask
>        and compare against the current caller arguments. Given that the
>        bitwidth would typically be a fixed value, I believe it is more
>        intuitive for the caller to provide it, ...
>
> I think it's easier than that.  bitwidth is just fls(bitmask) which
> should be efficient enough not to bother the programmer.  If bitmask is
> constant it's even the compiler that does the work here.
>

That is a good point. I agree that width is ugly in the main API. It
is just fls(mask) and I now realize that this is an inline so you're
right it would get taken care of by the compiler. fls is checked with
__constant_fls first. Beauty! Thanks Uwe! I'll make these changes.

Thanks,
jaya

Re: [RFC 2.6.28 1/2] gpiolib: add set/get batch v4

[Uwe Kleine-König]

Hi Jaya,

> >> +[OPTIONAL] Spinlock-Safe GPIO Batch access
> > Is it really spinlock safe in general?  Or only if gpio_cansleep(gpio)
> > if false for each gpio to get or set?
> 
> You are correct to raise this issue. It is only spinlock safe if
> chip->cansleep is false. Initially, I wasn't sure what to do. The
> original gpio set/get_value() just does;
>         WARN_ON(extra_checks && chip->can_sleep);
> and it is documented as:
> 
> "
> Spinlock-Safe GPIO access
> -------------------------
> <snip>
> return zero.  Also, using these calls for GPIOs that can't safely be accessed
> without sleeping (see below) is an error.
> "
> 
> I will change this in the batch code to return an error if can_sleep
> is detected on any involved gpio_chip.
Wait, I got it wrong.  I thought gpio_set_value might sleep if
chip->cansleep is true, but there are extra API functions for
cansleep-chips.  So I'd do it the same way as for the non-batch
functions and just WARN_ON(extra_checks && chip->cansleep) for each
involved chip.

Later it might make sense to add the _cansleep variants.

Best regards
Uwe
-- 
Pengutronix e.K.                              | Uwe Kleine-König            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |
Peiner Strasse 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686              | Fax:   +49-5121-206917-5555 |