[PATCH RFC 0/6 v3] gpio: Add block GPIO

[PATCH RFC 0/6 v3] gpio: Add block GPIO

[Roland Stigge]

This set of patches adds:

* Block GPIO API to gpiolib
* Sysfs support for GPIO API, to provide userland access
* Devicetree support to instantiate GPIO blocks via DT
* Example implementations in several gpio drivers since they need
  special accessor functions for block wise GPIO access

Signed-off-by: Roland Stigge <stigge@antcom.de>

--

Changes since v2:
* Added sysfs support
* Added devicetree support
* Added support for lpc32xx, generic
* Added functions for GPIO block registration
* Added more error checking
* Bit remapping bugfix

Roland Stigge (6):
 gpio: Add a block GPIO API to gpiolib
 gpio: Add sysfs support to block GPIO API
 gpio: Add device tree support to block GPIO API
 gpio-max730x: Add block GPIO API
 gpio-lpc32xx: Add block GPIO API
 gpio-generic: Add block GPIO API

 Documentation/ABI/testing/sysfs-gpio                  |    6 
 Documentation/devicetree/bindings/gpio/gpio-block.txt |   36 
 Documentation/gpio.txt                                |   45 +
 drivers/gpio/Makefile                                 |    1 
 drivers/gpio/gpio-generic.c                           |   56 +
 drivers/gpio/gpio-lpc32xx.c                           |   78 +
 drivers/gpio/gpio-max730x.c                           |   60 +
 drivers/gpio/gpioblock-of.c                           |   83 +
 drivers/gpio/gpiolib.c                                |  449 +++++++++-
 include/asm-generic/gpio.h                            |   25 
 include/linux/gpio.h                                  |   74 +
 11 files changed, 911 insertions(+), 2 deletions(-)

[PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib

[Roland Stigge]

The recurring task of providing simultaneous access to GPIO lines (especially
for bit banging protocols) needs an appropriate API.

This patch adds a kernel internal "Block GPIO" API that enables simultaneous
access to several GPIOs. This is done by abstracting GPIOs to an n-bit word:
Once requested, it provides access to a group of GPIOs which can range over
multiple GPIO chips.

Signed-off-by: Roland Stigge <stigge@antcom.de>
---

 Documentation/gpio.txt     |   45 +++++++++
 drivers/gpio/gpiolib.c     |  223 +++++++++++++++++++++++++++++++++++++++++++++
 include/asm-generic/gpio.h |   14 ++
 include/linux/gpio.h       |   61 ++++++++++++
 4 files changed, 343 insertions(+)

--- linux-2.6.orig/Documentation/gpio.txt
+++ linux-2.6/Documentation/gpio.txt
@@ -439,6 +439,51 @@ slower clock delays the rising edge of S
 signaling rate accordingly.
 
 
+Block GPIO
+----------
+
+The above described interface concentrates on handling single GPIOs.  However,
+in applications where it is critical to set several GPIOs at once, this
+interface doesn't work well, e.g. bit-banging protocols via grouped GPIO lines.
+Consider a GPIO controller that is connected via a slow I2C line. When
+switching two or more GPIOs one after another, there can be considerable time
+between those events. This is solved by an interface called Block GPIO:
+
+struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size);
+
+This creates a new block of GPIOs as a list of GPIO numbers with the specified
+size which are accessible via the returned struct gpio_block and the accessor
+functions described below. Please note that you need to request the GPIOs
+separately via gpio_request(). An arbitrary list of globally valid GPIOs can be
+specified, even ranging over several gpio_chips. Actual handling of I/O
+operations will be done on a best effort base, i.e. simultaneous I/O only where
+possible by hardware and implemented in the respective GPIO driver. The number
+of GPIOs in one block is limited to 32 on a 32 bit system, and 64 on a 64 bit
+system. However, several blocks can be defined at once.
+
+unsigned gpio_block_get(struct gpio_block *block);
+void gpio_block_set(struct gpio_block *block, unsigned value);
+
+With those accessor functions, setting and getting the GPIO values is possible,
+analogous to gpio_get_value() and gpio_set_value(). Each bit in the return
+value of gpio_block_get() and in the value argument of gpio_block_set()
+corresponds to a bit specified on gpio_block_create(). Block operations in
+hardware are only possible where the respective GPIO driver implements it,
+falling back to using single GPIO operations where the driver only implements
+single GPIO access.
+
+void gpio_block_free(struct gpio_block *block);
+
+After the GPIO block isn't used anymore, it should be free'd via
+gpio_block_free().
+
+int gpio_block_register(struct gpio_block *block);
+void gpio_block_unregister(struct gpio_block *block);
+
+These functions can be used to register a GPIO block. Blocks registered this
+way will be available via sysfs.
+
+
 What do these conventions omit?
 ===============================
 One of the biggest things these conventions omit is pin multiplexing, since
--- linux-2.6.orig/drivers/gpio/gpiolib.c
+++ linux-2.6/drivers/gpio/gpiolib.c
@@ -83,6 +83,10 @@ static inline void desc_set_label(struct
 #endif
 }
 
+#define NR_GPIO_BLOCKS	16
+
+static struct gpio_block *gpio_block[NR_GPIO_BLOCKS];
+
 /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
  * when setting direction, and otherwise illegal.  Until board setup code
  * and drivers use explicit requests everywhere (which won't happen when
@@ -1676,6 +1680,225 @@ void __gpio_set_value(unsigned gpio, int
 }
 EXPORT_SYMBOL_GPL(__gpio_set_value);
 
+static inline
+int gpio_block_chip_index(struct gpio_block *block, struct gpio_chip *gc)
+{
+	int i;
+
+	for (i = 0; i < block->nchip; i++) {
+		if (block->gbc[i].gc == gc)
+			return i;
+	}
+	return -1;
+}
+
+struct gpio_block *gpio_block_create(unsigned *gpios, size_t size,
+				     const char *name)
+{
+	struct gpio_block *block;
+	struct gpio_block_chip *gbc;
+	struct gpio_remap *remap;
+	int i;
+
+	if (size < 1 || size > sizeof(unsigned) * 8)
+		return ERR_PTR(-EINVAL);
+
+	block = kzalloc(sizeof(struct gpio_block), GFP_KERNEL);
+	if (!block)
+		return ERR_PTR(-ENOMEM);
+
+	block->name = name;
+	block->ngpio = size;
+	block->gpio = kzalloc(sizeof(*block->gpio) * size, GFP_KERNEL);
+	if (!block->gpio)
+		goto err1;
+
+	memcpy(block->gpio, gpios, sizeof(*block->gpio) * size);
+
+	for (i = 0; i < size; i++)
+		if (!gpio_is_valid(gpios[i]))
+			goto err2;
+
+	for (i = 0; i < size; i++) {
+		struct gpio_chip *gc = gpio_to_chip(gpios[i]);
+		int bit = gpios[i] - gc->base;
+		int index = gpio_block_chip_index(block, gc);
+
+		if (index < 0) {
+			block->nchip++;
+			block->gbc = krealloc(block->gbc,
+					      sizeof(struct gpio_block_chip) *
+					      block->nchip, GFP_KERNEL);
+			if (!block->gbc)
+				goto err2;
+			gbc = &block->gbc[block->nchip - 1];
+			gbc->gc = gc;
+			gbc->remap = NULL;
+			gbc->nremap = 0;
+			gbc->mask = 0;
+		} else {
+			gbc = &block->gbc[index];
+		}
+		/* represents the mask necessary on calls to the driver's
+		 * .get_block() and .set_block()
+		 */
+		gbc->mask |= BIT(bit);
+
+		/* collect gpios that are specified together, represented by
+		 * neighboring bits
+		 */
+		remap = &gbc->remap[gbc->nremap - 1];
+		if (!gbc->nremap || (bit - i != remap->offset)) {
+			gbc->nremap++;
+			gbc->remap = krealloc(gbc->remap,
+					      sizeof(struct gpio_remap) *
+					      gbc->nremap, GFP_KERNEL);
+			if (!gbc->remap)
+				goto err3;
+			remap = &gbc->remap[gbc->nremap - 1];
+			remap->offset = bit - i;
+			remap->mask = 0;
+		}
+
+		/* represents the mask necessary for bit reordering between
+		 * gpio_block (i.e. as specified on gpio_block_get() and
+		 * gpio_block_set()) and gpio_chip domain (i.e. as specified on
+		 * the driver's .set_block() and .get_block())
+		 */
+		remap->mask |= BIT(i);
+	}
+
+	return block;
+err3:
+	for (i = 0; i < block->nchip - 1; i++)
+		kfree(block->gbc[i].remap);
+	kfree(block->gbc);
+err2:
+	kfree(block->gpio);
+err1:
+	kfree(block);
+	return ERR_PTR(-ENOMEM);
+}
+EXPORT_SYMBOL_GPL(gpio_block_create);
+
+void gpio_block_free(struct gpio_block *block)
+{
+	int i;
+
+	for (i = 0; i < block->nchip; i++)
+		kfree(block->gbc[i].remap);
+	kfree(block->gpio);
+	kfree(block->gbc);
+	kfree(block);
+}
+EXPORT_SYMBOL_GPL(gpio_block_free);
+
+unsigned gpio_block_get(const struct gpio_block *block)
+{
+	struct gpio_block_chip *gbc;
+	int i, j;
+	unsigned values = 0;
+
+	for (i = 0; i < block->nchip; i++) {
+		unsigned remapped = 0;
+
+		gbc = &block->gbc[i];
+
+		if (gbc->gc->get_block) {
+			remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
+		} else { /* emulate */
+			unsigned bit = 1;
+
+			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
+				if (gbc->mask & bit)
+					remapped |= gbc->gc->get(gbc->gc,
+							gbc->gc->base + j) << j;
+			}
+		}
+
+		for (j = 0; j < gbc->nremap; j++) {
+			struct gpio_remap *gr = &gbc->remap[j];
+
+			values |= (remapped >> gr->offset) & gr->mask;
+		}
+	}
+
+	return values;
+}
+EXPORT_SYMBOL_GPL(gpio_block_get);
+
+void gpio_block_set(struct gpio_block *block, unsigned values)
+{
+	struct gpio_block_chip *gbc;
+	int i, j;
+
+	for (i = 0; i < block->nchip; i++) {
+		unsigned remapped = 0;
+
+		gbc = &block->gbc[i];
+
+		for (j = 0; j < gbc->nremap; j++) {
+			struct gpio_remap *gr = &gbc->remap[j];
+
+			remapped |= (values & gr->mask) << gr->offset;
+		}
+		if (gbc->gc->set_block) {
+			gbc->gc->set_block(gbc->gc, gbc->mask, remapped);
+		} else { /* emulate */
+			unsigned bit = 1;
+
+			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
+				if (gbc->mask & bit)
+					gbc->gc->set(gbc->gc, gbc->gc->base + j,
+						     (remapped >> j) & 1);
+			}
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(gpio_block_set);
+
+struct gpio_block *gpio_block_find_by_name(const char *name)
+{
+	int i;
+
+	for (i = 0; i < NR_GPIO_BLOCKS; i++) {
+		if (gpio_block[i] && !strcmp(gpio_block[i]->name, name))
+			return gpio_block[i];
+	}
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(gpio_block_find_by_name);
+
+int gpio_block_register(struct gpio_block *block)
+{
+	int i;
+
+	if (gpio_block_find_by_name(block->name))
+		return -EBUSY;
+
+	for (i = 0; i < NR_GPIO_BLOCKS; i++) {
+		if (!gpio_block[i]) {
+			gpio_block[i] = block;
+			break;
+		}
+	}
+	return i == NR_GPIO_BLOCKS ? -EBUSY : 0;
+}
+EXPORT_SYMBOL_GPL(gpio_block_register);
+
+void gpio_block_unregister(struct gpio_block *block)
+{
+	int i;
+
+	for (i = 0; i < NR_GPIO_BLOCKS; i++) {
+		if (gpio_block[i] == block) {
+			gpio_block[i] = NULL;
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(gpio_block_unregister);
+
 /**
  * __gpio_cansleep() - report whether gpio value access will sleep
  * @gpio: gpio in question
--- linux-2.6.orig/include/asm-generic/gpio.h
+++ linux-2.6/include/asm-generic/gpio.h
@@ -43,6 +43,7 @@ static inline bool gpio_is_valid(int num
 
 struct device;
 struct gpio;
+struct gpio_block;
 struct seq_file;
 struct module;
 struct device_node;
@@ -105,6 +106,8 @@ struct gpio_chip {
 						unsigned offset);
 	int			(*get)(struct gpio_chip *chip,
 						unsigned offset);
+	unsigned		(*get_block)(struct gpio_chip *chip,
+					     unsigned mask);
 	int			(*direction_output)(struct gpio_chip *chip,
 						unsigned offset, int value);
 	int			(*set_debounce)(struct gpio_chip *chip,
@@ -112,6 +115,8 @@ struct gpio_chip {
 
 	void			(*set)(struct gpio_chip *chip,
 						unsigned offset, int value);
+	void			(*set_block)(struct gpio_chip *chip,
+					     unsigned mask, unsigned values);
 
 	int			(*to_irq)(struct gpio_chip *chip,
 						unsigned offset);
@@ -171,6 +176,15 @@ extern void gpio_set_value_cansleep(unsi
 extern int __gpio_get_value(unsigned gpio);
 extern void __gpio_set_value(unsigned gpio, int value);
 
+extern struct gpio_block *gpio_block_create(unsigned *gpio, size_t size,
+					    const char *name);
+extern void gpio_block_free(struct gpio_block *block);
+extern unsigned gpio_block_get(const struct gpio_block *block);
+extern void gpio_block_set(struct gpio_block *block, unsigned values);
+extern struct gpio_block *gpio_block_find_by_name(const char *name);
+extern int gpio_block_register(struct gpio_block *block);
+extern void gpio_block_unregister(struct gpio_block *block);
+
 extern int __gpio_cansleep(unsigned gpio);
 
 extern int __gpio_to_irq(unsigned gpio);
--- linux-2.6.orig/include/linux/gpio.h
+++ linux-2.6/include/linux/gpio.h
@@ -2,6 +2,7 @@
 #define __LINUX_GPIO_H
 
 #include <linux/errno.h>
+#include <linux/types.h>
 
 /* see Documentation/gpio.txt */
 
@@ -39,6 +40,31 @@ struct gpio {
 	const char	*label;
 };
 
+struct gpio_remap {
+	int	mask;
+	int	offset;
+};
+
+struct gpio_block_chip {
+	struct gpio_chip	*gc;
+	struct gpio_remap	*remap;
+	int			nremap;
+	unsigned		mask;
+};
+
+/**
+ * struct gpio_block - a structure describing a list of GPIOs for simultaneous
+ *                     operations
+ */
+struct gpio_block {
+	struct gpio_block_chip	*gbc;
+	size_t			nchip;
+	const char		*name;
+
+	int			ngpio;
+	unsigned		*gpio;
+};
+
 #ifdef CONFIG_GENERIC_GPIO
 
 #ifdef CONFIG_ARCH_HAVE_CUSTOM_GPIO_H
@@ -169,6 +195,41 @@ static inline void gpio_set_value(unsign
 	WARN_ON(1);
 }
 
+static inline
+struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size,
+				     const char *name)
+{
+	WARN_ON(1);
+	return NULL;
+}
+
+static inline void gpio_block_free(struct gpio_block *block)
+{
+	WARN_ON(1);
+}
+
+static inline unsigned gpio_block_get(const struct gpio_block *block)
+{
+	WARN_ON(1);
+	return 0;
+}
+
+static inline void gpio_block_set(struct gpio_block *block, unsigned value)
+{
+	WARN_ON(1);
+}
+
+static inline int gpio_block_register(struct gpio_block *block)
+{
+	WARN_ON(1);
+	return 0;
+}
+
+static inline void gpio_block_unregister(struct gpio_block *block)
+{
+	WARN_ON(1);
+}
+
 static inline int gpio_cansleep(unsigned gpio)
 {
 	/* GPIO can never have been requested or set as {in,out}put */

[PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib

[Ryan Mallon]

On 13/10/12 06:11, Roland Stigge wrote:
> The recurring task of providing simultaneous access to GPIO lines (especially
> for bit banging protocols) needs an appropriate API.
> 
> This patch adds a kernel internal "Block GPIO" API that enables simultaneous
> access to several GPIOs. This is done by abstracting GPIOs to an n-bit word:
> Once requested, it provides access to a group of GPIOs which can range over
> multiple GPIO chips.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>

Hi Roland,

Some comments below.

~Ryan

> ---
> 
>  Documentation/gpio.txt     |   45 +++++++++
>  drivers/gpio/gpiolib.c     |  223 +++++++++++++++++++++++++++++++++++++++++++++
>  include/asm-generic/gpio.h |   14 ++
>  include/linux/gpio.h       |   61 ++++++++++++
>  4 files changed, 343 insertions(+)
> 
> --- linux-2.6.orig/Documentation/gpio.txt
> +++ linux-2.6/Documentation/gpio.txt
> @@ -439,6 +439,51 @@ slower clock delays the rising edge of S
>  signaling rate accordingly.
>  
>  
> +Block GPIO
> +----------
> +
> +The above described interface concentrates on handling single GPIOs.  However,
> +in applications where it is critical to set several GPIOs at once, this
> +interface doesn't work well, e.g. bit-banging protocols via grouped GPIO lines.
> +Consider a GPIO controller that is connected via a slow I2C line. When
> +switching two or more GPIOs one after another, there can be considerable time
> +between those events. This is solved by an interface called Block GPIO:

The emulate behaviour of gpio block switches gpios one after the other.
Is the problem only solved if the block_get/block_set callbacks can be
implemented?

> +struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size);
> +
> +This creates a new block of GPIOs as a list of GPIO numbers with the specified
> +size which are accessible via the returned struct gpio_block and the accessor
> +functions described below. Please note that you need to request the GPIOs
> +separately via gpio_request(). An arbitrary list of globally valid GPIOs can be
> +specified, even ranging over several gpio_chips. Actual handling of I/O
> +operations will be done on a best effort base, i.e. simultaneous I/O only where
> +possible by hardware and implemented in the respective GPIO driver. The number
> +of GPIOs in one block is limited to 32 on a 32 bit system, and 64 on a 64 bit
> +system. However, several blocks can be defined at once.
> +
> +unsigned gpio_block_get(struct gpio_block *block);
> +void gpio_block_set(struct gpio_block *block, unsigned value);
> +
> +With those accessor functions, setting and getting the GPIO values is possible,
> +analogous to gpio_get_value() and gpio_set_value(). Each bit in the return
> +value of gpio_block_get() and in the value argument of gpio_block_set()
> +corresponds to a bit specified on gpio_block_create(). Block operations in
> +hardware are only possible where the respective GPIO driver implements it,
> +falling back to using single GPIO operations where the driver only implements
> +single GPIO access.
> +
> +void gpio_block_free(struct gpio_block *block);
> +
> +After the GPIO block isn't used anymore, it should be free'd via
> +gpio_block_free().
> +
> +int gpio_block_register(struct gpio_block *block);
> +void gpio_block_unregister(struct gpio_block *block);
> +
> +These functions can be used to register a GPIO block. Blocks registered this
> +way will be available via sysfs.
> +
> +
>  What do these conventions omit?
>  ===============================
>  One of the biggest things these conventions omit is pin multiplexing, since
> --- linux-2.6.orig/drivers/gpio/gpiolib.c
> +++ linux-2.6/drivers/gpio/gpiolib.c
> @@ -83,6 +83,10 @@ static inline void desc_set_label(struct
>  #endif
>  }
>  
> +#define NR_GPIO_BLOCKS	16
> +
> +static struct gpio_block *gpio_block[NR_GPIO_BLOCKS];
> +
>  /* Warn when drivers omit gpio_request() calls -- legal but ill-advised
>   * when setting direction, and otherwise illegal.  Until board setup code
>   * and drivers use explicit requests everywhere (which won't happen when
> @@ -1676,6 +1680,225 @@ void __gpio_set_value(unsigned gpio, int
>  }
>  EXPORT_SYMBOL_GPL(__gpio_set_value);
>  
> +static inline

Nitpick - don't need the inline, the compiler will do so for you.

> +int gpio_block_chip_index(struct gpio_block *block, struct gpio_chip *gc)

Should be static?

> +{
> +	int i;
> +
> +	for (i = 0; i < block->nchip; i++) {
> +		if (block->gbc[i].gc == gc)
> +			return i;
> +	}
> +	return -1;
> +}
> +
> +struct gpio_block *gpio_block_create(unsigned *gpios, size_t size,
> +				     const char *name)
> +{
> +	struct gpio_block *block;
> +	struct gpio_block_chip *gbc;
> +	struct gpio_remap *remap;
> +	int i;
> +
> +	if (size < 1 || size > sizeof(unsigned) * 8)
> +		return ERR_PTR(-EINVAL);
> +
> +	block = kzalloc(sizeof(struct gpio_block), GFP_KERNEL);
> +	if (!block)
> +		return ERR_PTR(-ENOMEM);
> +
> +	block->name = name;
> +	block->ngpio = size;
> +	block->gpio = kzalloc(sizeof(*block->gpio) * size, GFP_KERNEL);
> +	if (!block->gpio)
> +		goto err1;
> +
> +	memcpy(block->gpio, gpios, sizeof(*block->gpio) * size);
> +
> +	for (i = 0; i < size; i++)
> +		if (!gpio_is_valid(gpios[i]))
> +			goto err2;

This loop should probably go at the start of the function, so you can
avoid doing the kzalloc/memcpy if it fails.

This function doesn't call gpio_request. Either it should, or it should
rely on the caller to have already done so, and call
gpio_ensure_requested here. There is also an implicit rule that any
gpios inside a block must not be freed as long as the block exists. The
code can't easily prevent this since gpios aren't refcounted. The rule
should be documented.

> +
> +	for (i = 0; i < size; i++) {
> +		struct gpio_chip *gc = gpio_to_chip(gpios[i]);
> +		int bit = gpios[i] - gc->base;
> +		int index = gpio_block_chip_index(block, gc);
> +
> +		if (index < 0) {
> +			block->nchip++;
> +			block->gbc = krealloc(block->gbc,
> +					      sizeof(struct gpio_block_chip) *
> +					      block->nchip, GFP_KERNEL);

krealloc is a bit nasty. Can't you add a struct list_head to struct
gpio_block_chip or something?

This also leaks memory if the krealloc fails, since the original pointer
is lost. You need to use a temporary for the re-allocation.

> +			if (!block->gbc)
> +				goto err2;
> +			gbc = &block->gbc[block->nchip - 1];
> +			gbc->gc = gc;
> +			gbc->remap = NULL;
> +			gbc->nremap = 0;
> +			gbc->mask = 0;
> +		} else {
> +			gbc = &block->gbc[index];
> +		}
> +		/* represents the mask necessary on calls to the driver's
> +		 * .get_block() and .set_block()
> +		 */

  /*
   * Nitpick - multi-line comment style looks like this.
   * However, other comments in this file use this style
   * so maybe keep for consistency.
   */

> +		gbc->mask |= BIT(bit);
> +
> +		/* collect gpios that are specified together, represented by
> +		 * neighboring bits
> +		 */
> +		remap = &gbc->remap[gbc->nremap - 1];

This looks broken. If gbc was re-alloced above (index < 0) then
gbc->remap == NULL and this will oops?

> +		if (!gbc->nremap || (bit - i != remap->offset)) {

gbc->nremap would have to be non-zero here, otherwise you have:

  gbc->remap[0 - 1]

above.

> +			gbc->nremap++;
> +			gbc->remap = krealloc(gbc->remap,
> +					      sizeof(struct gpio_remap) *
> +					      gbc->nremap, GFP_KERNEL);

Memory leak on failure. Also, is an alternative to krealloc possible.
Maybe a list?

> +			if (!gbc->remap)
> +				goto err3;
> +			remap = &gbc->remap[gbc->nremap - 1];
> +			remap->offset = bit - i;
> +			remap->mask = 0;
> +		}
> +
> +		/* represents the mask necessary for bit reordering between
> +		 * gpio_block (i.e. as specified on gpio_block_get() and
> +		 * gpio_block_set()) and gpio_chip domain (i.e. as specified on
> +		 * the driver's .set_block() and .get_block())
> +		 */
> +		remap->mask |= BIT(i);
> +	}

The remap functionality isn't very well explained (and looks like it
doesn't work properly anyway). Some comments explaining what the remap
does and how it works would be useful.

> +	return block;
> +err3:
> +	for (i = 0; i < block->nchip - 1; i++)
> +		kfree(block->gbc[i].remap);
> +	kfree(block->gbc);
> +err2:
> +	kfree(block->gpio);
> +err1:
> +	kfree(block);
> +	return ERR_PTR(-ENOMEM);
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_create);
> +
> +void gpio_block_free(struct gpio_block *block)
> +{
> +	int i;
> +
> +	for (i = 0; i < block->nchip; i++)
> +		kfree(block->gbc[i].remap);
> +	kfree(block->gpio);
> +	kfree(block->gbc);
> +	kfree(block);
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_free);
> +
> +unsigned gpio_block_get(const struct gpio_block *block)
> +{
> +	struct gpio_block_chip *gbc;
> +	int i, j;
> +	unsigned values = 0;
> +
> +	for (i = 0; i < block->nchip; i++) {
> +		unsigned remapped = 0;
> +
> +		gbc = &block->gbc[i];
> +
> +		if (gbc->gc->get_block) {
> +			remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
> +		} else { /* emulate */
> +			unsigned bit = 1;
> +
> +			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
> +				if (gbc->mask & bit)

A proper bitmask might be more ideal for this. It would remove the
sizeof(unsigned) restriction and allow you to use for_each_set_bit for
these loops.

> +					remapped |= gbc->gc->get(gbc->gc,
> +							gbc->gc->base + j) << j;
> +			}
> +		}
> +
> +		for (j = 0; j < gbc->nremap; j++) {
> +			struct gpio_remap *gr = &gbc->remap[j];
> +
> +			values |= (remapped >> gr->offset) & gr->mask;
> +		}
> +	}
> +
> +	return values;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_get);
> +
> +void gpio_block_set(struct gpio_block *block, unsigned values)
> +{
> +	struct gpio_block_chip *gbc;
> +	int i, j;
> +
> +	for (i = 0; i < block->nchip; i++) {
> +		unsigned remapped = 0;
> +
> +		gbc = &block->gbc[i];
> +
> +		for (j = 0; j < gbc->nremap; j++) {
> +			struct gpio_remap *gr = &gbc->remap[j];
> +
> +			remapped |= (values & gr->mask) << gr->offset;
> +		}
> +		if (gbc->gc->set_block) {
> +			gbc->gc->set_block(gbc->gc, gbc->mask, remapped);
> +		} else { /* emulate */

Nitpick - Put the comment on a line by itself.

> +			unsigned bit = 1;
> +
> +			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
> +				if (gbc->mask & bit)
> +					gbc->gc->set(gbc->gc, gbc->gc->base + j,
> +						     (remapped >> j) & 1);
> +			}

This doesn't clear pins which are set to zero? If you are using
gpio_block to bit-bang a bus then you probably want that to happen.
Probably you want three functions, gpio_block_set (set only),
gpio_block_clear (clear only) and gpio_block_drive (set/clear).

> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_set);
> +
> +struct gpio_block *gpio_block_find_by_name(const char *name)
> +{
> +	int i;
> +
> +	for (i = 0; i < NR_GPIO_BLOCKS; i++) {

A static limit is lame. Make it a list.

> +		if (gpio_block[i] && !strcmp(gpio_block[i]->name, name))
> +			return gpio_block[i];
> +	}
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_find_by_name);
> +
> +int gpio_block_register(struct gpio_block *block)
> +{
> +	int i;
> +
> +	if (gpio_block_find_by_name(block->name))
> +		return -EBUSY;
> +
> +	for (i = 0; i < NR_GPIO_BLOCKS; i++) {
> +		if (!gpio_block[i]) {
> +			gpio_block[i] = block;
> +			break;
> +		}
> +	}
> +	return i == NR_GPIO_BLOCKS ? -EBUSY : 0;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_register);
> +
> +void gpio_block_unregister(struct gpio_block *block)
> +{
> +	int i;
> +
> +	for (i = 0; i < NR_GPIO_BLOCKS; i++) {
> +		if (gpio_block[i] == block) {
> +			gpio_block[i] = NULL;
> +			break;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_unregister);
> +
>  /**
>   * __gpio_cansleep() - report whether gpio value access will sleep
>   * @gpio: gpio in question
> --- linux-2.6.orig/include/asm-generic/gpio.h
> +++ linux-2.6/include/asm-generic/gpio.h
> @@ -43,6 +43,7 @@ static inline bool gpio_is_valid(int num
>  
>  struct device;
>  struct gpio;
> +struct gpio_block;
>  struct seq_file;
>  struct module;
>  struct device_node;
> @@ -105,6 +106,8 @@ struct gpio_chip {
>  						unsigned offset);
>  	int			(*get)(struct gpio_chip *chip,
>  						unsigned offset);
> +	unsigned		(*get_block)(struct gpio_chip *chip,
> +					     unsigned mask);
>  	int			(*direction_output)(struct gpio_chip *chip,
>  						unsigned offset, int value);
>  	int			(*set_debounce)(struct gpio_chip *chip,
> @@ -112,6 +115,8 @@ struct gpio_chip {
>  
>  	void			(*set)(struct gpio_chip *chip,
>  						unsigned offset, int value);
> +	void			(*set_block)(struct gpio_chip *chip,
> +					     unsigned mask, unsigned values);
>  
>  	int			(*to_irq)(struct gpio_chip *chip,
>  						unsigned offset);
> @@ -171,6 +176,15 @@ extern void gpio_set_value_cansleep(unsi
>  extern int __gpio_get_value(unsigned gpio);
>  extern void __gpio_set_value(unsigned gpio, int value);
>  
> +extern struct gpio_block *gpio_block_create(unsigned *gpio, size_t size,
> +					    const char *name);
> +extern void gpio_block_free(struct gpio_block *block);
> +extern unsigned gpio_block_get(const struct gpio_block *block);
> +extern void gpio_block_set(struct gpio_block *block, unsigned values);
> +extern struct gpio_block *gpio_block_find_by_name(const char *name);
> +extern int gpio_block_register(struct gpio_block *block);
> +extern void gpio_block_unregister(struct gpio_block *block);
> +
>  extern int __gpio_cansleep(unsigned gpio);
>  
>  extern int __gpio_to_irq(unsigned gpio);
> --- linux-2.6.orig/include/linux/gpio.h
> +++ linux-2.6/include/linux/gpio.h
> @@ -2,6 +2,7 @@
>  #define __LINUX_GPIO_H
>  
>  #include <linux/errno.h>
> +#include <linux/types.h>
>  
>  /* see Documentation/gpio.txt */
>  
> @@ -39,6 +40,31 @@ struct gpio {
>  	const char	*label;
>  };
>  
> +struct gpio_remap {
> +	int	mask;
> +	int	offset;
> +};
> +
> +struct gpio_block_chip {
> +	struct gpio_chip	*gc;
> +	struct gpio_remap	*remap;
> +	int			nremap;
> +	unsigned		mask;
> +};
> +
> +/**
> + * struct gpio_block - a structure describing a list of GPIOs for simultaneous
> + *                     operations
> + */
> +struct gpio_block {
> +	struct gpio_block_chip	*gbc;
> +	size_t			nchip;
> +	const char		*name;
> +
> +	int			ngpio;
> +	unsigned		*gpio;
> +};
> +
>  #ifdef CONFIG_GENERIC_GPIO
>  
>  #ifdef CONFIG_ARCH_HAVE_CUSTOM_GPIO_H
> @@ -169,6 +195,41 @@ static inline void gpio_set_value(unsign
>  	WARN_ON(1);
>  }
>  
> +static inline
> +struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size,
> +				     const char *name)
> +{
> +	WARN_ON(1);
> +	return NULL;
> +}
> +
> +static inline void gpio_block_free(struct gpio_block *block)
> +{
> +	WARN_ON(1);
> +}
> +
> +static inline unsigned gpio_block_get(const struct gpio_block *block)
> +{
> +	WARN_ON(1);
> +	return 0;
> +}
> +
> +static inline void gpio_block_set(struct gpio_block *block, unsigned value)
> +{
> +	WARN_ON(1);
> +}
> +
> +static inline int gpio_block_register(struct gpio_block *block)
> +{
> +	WARN_ON(1);
> +	return 0;
> +}
> +
> +static inline void gpio_block_unregister(struct gpio_block *block)
> +{
> +	WARN_ON(1);
> +}
> +
>  static inline int gpio_cansleep(unsigned gpio)
>  {
>  	/* GPIO can never have been requested or set as {in,out}put */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib

[Roland Stigge]

Hi Ryan,

thank you for your feedback, I will include it, except for some points
noted below:

On 10/15/2012 07:20 AM, Ryan Mallon wrote:
>> +The above described interface concentrates on handling single GPIOs.  However,
>> +in applications where it is critical to set several GPIOs at once, this
>> +interface doesn't work well, e.g. bit-banging protocols via grouped GPIO lines.
>> +Consider a GPIO controller that is connected via a slow I2C line. When
>> +switching two or more GPIOs one after another, there can be considerable time
>> +between those events. This is solved by an interface called Block GPIO:
> 
> The emulate behaviour of gpio block switches gpios one after the other.
> Is the problem only solved if the block_get/block_set callbacks can be
> implemented?

Right, this is documented some lines away:

>> +struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size);
>> +
>> +This creates a new block of GPIOs as a list of GPIO numbers with the specified
>> +size which are accessible via the returned struct gpio_block and the accessor
>> +functions described below. Please note that you need to request the GPIOs
>> +separately via gpio_request(). An arbitrary list of globally valid GPIOs can be
>> +specified, even ranging over several gpio_chips. Actual handling of I/O
>> +operations will be done on a best effort base, i.e. simultaneous I/O only where
>> +possible by hardware and implemented in the respective GPIO driver. The number
>> +of GPIOs in one block is limited to 32 on a 32 bit system, and 64 on a 64 bit
>> +system. However, several blocks can be defined at once.
>> +
>> +unsigned gpio_block_get(struct gpio_block *block);
>> +void gpio_block_set(struct gpio_block *block, unsigned value);
>> +
>> +With those accessor functions, setting and getting the GPIO values is possible,
>> +analogous to gpio_get_value() and gpio_set_value(). Each bit in the return
>> +value of gpio_block_get() and in the value argument of gpio_block_set()
>> +corresponds to a bit specified on gpio_block_create(). Block operations in
>> +hardware are only possible where the respective GPIO driver implements it,
>> +falling back to using single GPIO operations where the driver only implements
>> +single GPIO access.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
... here.

>> +struct gpio_block *gpio_block_create(unsigned *gpios, size_t size,
>> +				     const char *name)
>> +{
>> +	struct gpio_block *block;
>> +	struct gpio_block_chip *gbc;
>> +	struct gpio_remap *remap;
>> +	int i;
>> +
>> +	if (size < 1 || size > sizeof(unsigned) * 8)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	block = kzalloc(sizeof(struct gpio_block), GFP_KERNEL);
>> +	if (!block)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	block->name = name;
>> +	block->ngpio = size;
>> +	block->gpio = kzalloc(sizeof(*block->gpio) * size, GFP_KERNEL);
>> +	if (!block->gpio)
>> +		goto err1;
>> +
>> +	memcpy(block->gpio, gpios, sizeof(*block->gpio) * size);
>> +
>> +	for (i = 0; i < size; i++)
>> +		if (!gpio_is_valid(gpios[i]))
>> +			goto err2;
> 
> This loop should probably go at the start of the function, so you can
> avoid doing the kzalloc/memcpy if it fails.

OK.

> This function doesn't call gpio_request. Either it should, or it should
> rely on the caller to have already done so, and call
> gpio_ensure_requested here. There is also an implicit rule that any
> gpios inside a block must not be freed as long as the block exists.

The idea is analogous to GPIOs itself. Just the existence of it doesn't
require it being requested. This way, it is possible to e.g. instantiate
a block via device tree and let the user itself take care for direction,
requesting and so on. Look at the sysfs binding: Only when someone
exports the actual block values to userspace (write 0/1 to "exported"),
the GPIOs in the block are requested by "sysfs".

Please note that this is already documented above (look for "Please note").

>> +
>> +	for (i = 0; i < size; i++) {
>> +		struct gpio_chip *gc = gpio_to_chip(gpios[i]);
>> +		int bit = gpios[i] - gc->base;
>> +		int index = gpio_block_chip_index(block, gc);
>> +
>> +		if (index < 0) {
>> +			block->nchip++;
>> +			block->gbc = krealloc(block->gbc,
>> +					      sizeof(struct gpio_block_chip) *
>> +					      block->nchip, GFP_KERNEL);
> 
> krealloc is a bit nasty. Can't you add a struct list_head to struct
> gpio_block_chip or something?

I also hesitated. But having it this way provides quite efficient access
later on.

> This also leaks memory if the krealloc fails, since the original pointer
> is lost. You need to use a temporary for the re-allocation.

OK.

>> +		/* represents the mask necessary on calls to the driver's
>> +		 * .get_block() and .set_block()
>> +		 */
> 
>   /*
>    * Nitpick - multi-line comment style looks like this.
>    * However, other comments in this file use this style
>    * so maybe keep for consistency.
>    */

Right, decided for the latter.

>> +		gbc->mask |= BIT(bit);
>> +
>> +		/* collect gpios that are specified together, represented by
>> +		 * neighboring bits
>> +		 */
>> +		remap = &gbc->remap[gbc->nremap - 1];
> 
> This looks broken. If gbc was re-alloced above (index < 0) then
> gbc->remap == NULL and this will oops?

No, because I took care that even though index can be < 0, the resulting
pointer is never dereferenced for -1.

> 
>> +		if (!gbc->nremap || (bit - i != remap->offset)) {
> 
> gbc->nremap would have to be non-zero here, otherwise you have:
> 
>   gbc->remap[0 - 1]
> 
> above.
> 
>> +			gbc->nremap++;
                        ^^^^^^^^^^^^^^
... here it is: in the gbc->nremap == 0 case, it gets incremented
immediately and remap is not dereferenced. Even in the above ( || ), the
right side is only evaluated if gbc->nremap > 0.

>> +			if (!gbc->remap)
>> +				goto err3;
>> +			remap = &gbc->remap[gbc->nremap - 1];
>> +			remap->offset = bit - i;
>> +			remap->mask = 0;
>> +		}
>> +
>> +		/* represents the mask necessary for bit reordering between
>> +		 * gpio_block (i.e. as specified on gpio_block_get() and
>> +		 * gpio_block_set()) and gpio_chip domain (i.e. as specified on
>> +		 * the driver's .set_block() and .get_block())
>> +		 */
>> +		remap->mask |= BIT(i);
>> +	}
> 
> The remap functionality isn't very well explained

Documenting now in gpio.h like this:

/*
 * struct gpio_remap - a structure for describing a bit mapping
 * @mask:       a bit mask
 * @offset:     how many bits to shift to the left (negative: to the
 *              right)
 *
 * When we are mapping bit values from one word to another (here: from
 * GPIO block domain to GPIO driver domain), we first mask them out
 * with mask and shift them as specified with offset. More complicated
 * mappings are done by grouping several of those structs and adding
 * the results together.
 */
struct gpio_remap {
        int     mask;
        int     offset;
};

> (and looks like it doesn't work properly anyway).

If you find an issue, please let me know. Works fine for me. Have you
tried? :-)

>> +unsigned gpio_block_get(const struct gpio_block *block)
>> +{
>> +	struct gpio_block_chip *gbc;
>> +	int i, j;
>> +	unsigned values = 0;
>> +
>> +	for (i = 0; i < block->nchip; i++) {
>> +		unsigned remapped = 0;
>> +
>> +		gbc = &block->gbc[i];
>> +
>> +		if (gbc->gc->get_block) {
>> +			remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
>> +		} else { /* emulate */
>> +			unsigned bit = 1;
>> +
>> +			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
>> +				if (gbc->mask & bit)
> 
> A proper bitmask might be more ideal for this. It would remove the
> sizeof(unsigned) restriction and allow you to use for_each_set_bit for
> these loops.

In a previous version of these patches, I actually had a generic bit
mask which was in turn awkward to handle, especially for the bit
remapping. Stijn brought me to the idea that for pragmatic reasons, 32
bit access is fully sufficient in most cases.

I also needed userland access (via sysfs), so there was no way of
accessing a block except via an int.

When there are GPIO drivers where we seriously need (and can handle
simultaneously) more than 32 bits, we can still extend the API. For now,
the cases where it is used is typically creating 8/16/32 bit busses with
GPIO lines, and on 64bit architectures even 64bit busses.

For this, the current API is working fine, even enabling userland access
via sysfs.

>> +			unsigned bit = 1;
>> +
>> +			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
>> +				if (gbc->mask & bit)
>> +					gbc->gc->set(gbc->gc, gbc->gc->base + j,
>> +						     (remapped >> j) & 1);
>> +			}
> 
> This doesn't clear pins which are set to zero?

It does. gbc->mask only masks which bits to set and clear. remapped
contains the actual bit values to set. 0 or 1.

Including all the other suggestions in the next update.

Thanks,

Roland

[PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib

[Ryan Mallon]

On 16/10/12 04:20, Roland Stigge wrote:
> Hi Ryan,
> 
> thank you for your feedback, I will include it, except for some points
> noted below:

>>> +		gbc->mask |= BIT(bit);
>>> +
>>> +		/* collect gpios that are specified together, represented by
>>> +		 * neighboring bits
>>> +		 */
>>> +		remap = &gbc->remap[gbc->nremap - 1];
>>
>> This looks broken. If gbc was re-alloced above (index < 0) then
>> gbc->remap == NULL and this will oops?
> 
> No, because I took care that even though index can be < 0, the resulting
> pointer is never dereferenced for -1.

Ah, I see. I think its a bit non-obvious and flaky though, since it
looks like you are both dereferencing a potentially NULL pointer, and
indexing an array with -1.

Even changing it to this I think makes it a bit more clear:

	if (gbc->remap == 0 ||
            bit - i != gbc->remap[gbc->nremap - 1].offset)
		gbc->nremap++;
		gbc->remap = krealloc(...);
		...

If you want to keep your way, at the very least I think it deserves a
comment, since it is easy to misread.

>> The remap functionality isn't very well explained
> 
> Documenting now in gpio.h like this:
> 
> /*
>  * struct gpio_remap - a structure for describing a bit mapping
>  * @mask:       a bit mask
>  * @offset:     how many bits to shift to the left (negative: to the
>  *              right)
>  *
>  * When we are mapping bit values from one word to another (here: from
>  * GPIO block domain to GPIO driver domain), we first mask them out
>  * with mask and shift them as specified with offset. More complicated
>  * mappings are done by grouping several of those structs and adding
>  * the results together.
>  */
> struct gpio_remap {
>         int     mask;
>         int     offset;
> };

Looks good. Thanks.

> If you find an issue, please let me know. Works fine for me. Have you
> tried? :-)

No, I was just looking at the code, and misread it.

>>> +unsigned gpio_block_get(const struct gpio_block *block)
>>> +{
>>> +	struct gpio_block_chip *gbc;
>>> +	int i, j;
>>> +	unsigned values = 0;
>>> +
>>> +	for (i = 0; i < block->nchip; i++) {
>>> +		unsigned remapped = 0;
>>> +
>>> +		gbc = &block->gbc[i];
>>> +
>>> +		if (gbc->gc->get_block) {
>>> +			remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
>>> +		} else { /* emulate */
>>> +			unsigned bit = 1;
>>> +
>>> +			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
>>> +				if (gbc->mask & bit)
>>
>> A proper bitmask might be more ideal for this. It would remove the
>> sizeof(unsigned) restriction and allow you to use for_each_set_bit for
>> these loops.
> 
> In a previous version of these patches, I actually had a generic bit
> mask which was in turn awkward to handle, especially for the bit
> remapping. Stijn brought me to the idea that for pragmatic reasons, 32
> bit access is fully sufficient in most cases.
> 
> I also needed userland access (via sysfs), so there was no way of
> accessing a block except via an int.
> 
> When there are GPIO drivers where we seriously need (and can handle
> simultaneously) more than 32 bits, we can still extend the API. For now,
> the cases where it is used is typically creating 8/16/32 bit busses with
> GPIO lines, and on 64bit architectures even 64bit busses.
> 
> For this, the current API is working fine, even enabling userland access
> via sysfs.

Fair enough. I didn't see the first round of patches. You probably can
still use for_each_set_bit though (maybe convert the mask to unsigned
long first to match the bitops API):

	for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
		...

>>> +			unsigned bit = 1;
>>> +
>>> +			for (j = 0; j < sizeof(unsigned) * 8; j++, bit <<= 1) {
>>> +				if (gbc->mask & bit)
>>> +					gbc->gc->set(gbc->gc, gbc->gc->base + j,
>>> +						     (remapped >> j) & 1);
>>> +			}
>>
>> This doesn't clear pins which are set to zero?
> 
> It does. gbc->mask only masks which bits to set and clear. remapped
> contains the actual bit values to set. 0 or 1.

Ugh, for some reason I was thinking that the gpio set function only
drove bits that were set in the mask (and had an analogous clear
function). Ignore me :-).

~Ryan

[PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib

[Roland Stigge]

Hi!

On 16/10/12 00:05, Ryan Mallon wrote:
>>> This looks broken. If gbc was re-alloced above (index < 0) then
>>> gbc->remap == NULL and this will oops?
>>
>> No, because I took care that even though index can be < 0, the resulting
>> pointer is never dereferenced for -1.
> 
> Ah, I see. I think its a bit non-obvious and flaky though, since it
> looks like you are both dereferencing a potentially NULL pointer, and
> indexing an array with -1.
> 
> Even changing it to this I think makes it a bit more clear:
> 
> 	if (gbc->remap == 0 ||
>             bit - i != gbc->remap[gbc->nremap - 1].offset)
> 		gbc->nremap++;
> 		gbc->remap = krealloc(...);
> 		...
> 
> If you want to keep your way, at the very least I think it deserves a
> comment, since it is easy to misread.

Yes, commenting it now.

Note that it's rather out-of-bounds than NULL pointer. (But with similar
results, though. ;-) )

>> For this, the current API is working fine, even enabling userland access
>> via sysfs.
> 
> Fair enough. I didn't see the first round of patches. You probably can
> still use for_each_set_bit though (maybe convert the mask to unsigned
> long first to match the bitops API):
> 
> 	for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
> 		...

Thanks for the hint! Useful.

Roland

[PATCH RFC 1/6 v3] gpio: Add a block GPIO API to gpiolib

[Linus Walleij]

On Fri, Oct 12, 2012 at 9:11 PM, Roland Stigge <stigge@antcom.de> wrote:

> +#define NR_GPIO_BLOCKS 16
> +
> +static struct gpio_block *gpio_block[NR_GPIO_BLOCKS];

16 looks quite arbitrary, as noted elsewhere please use a list.

Yours,
Linus Walleij

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Roland Stigge]

This patch adds sysfs support to the block GPIO API.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 Documentation/ABI/testing/sysfs-gpio |    6 
 drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
 include/asm-generic/gpio.h           |   11 +
 include/linux/gpio.h                 |   13 ++
 4 files changed, 254 insertions(+), 2 deletions(-)

--- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio
+++ linux-2.6/Documentation/ABI/testing/sysfs-gpio
@@ -24,4 +24,8 @@ Description:
 	    /base ... (r/o) same as N
 	    /label ... (r/o) descriptive, not necessarily unique
 	    /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
-
+	/blockN ... for each GPIO block #N
+	    /ngpio ... (r/o) number of GPIOs in this group
+	    /exported ... sysfs export state of this group (0, 1)
+	    /value ... current value as 32 or 64 bit integer in decimal
+                       (only available if /exported is 1)
--- linux-2.6.orig/drivers/gpio/gpiolib.c
+++ linux-2.6/drivers/gpio/gpiolib.c
@@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi
 				chip->label, status);
 }
 
+static ssize_t gpio_block_ngpio_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	const struct gpio_block	*block = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", block->ngpio);
+}
+static struct device_attribute
+dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL);
+
+static ssize_t gpio_block_value_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	const struct gpio_block *block = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", gpio_block_get(block));
+}
+
+static bool gpio_block_is_output(struct gpio_block *block)
+{
+	int i;
+
+	for (i = 0; i < block->ngpio; i++)
+		if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags))
+			return false;
+	return true;
+}
+
+static ssize_t gpio_block_value_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t size)
+{
+	ssize_t			status;
+	struct gpio_block	*block = dev_get_drvdata(dev);
+	unsigned long		value;
+
+	mutex_lock(&sysfs_lock);
+
+	status = kstrtoul(buf, 0, &value);
+	if (status == 0) {
+		if (gpio_block_is_output(block)) {
+			gpio_block_set(block, value);
+			status = size;
+		} else {
+			status = -EPERM;
+		}
+	}
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static struct device_attribute
+dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show,
+			      gpio_block_value_store);
+
+static int gpio_block_value_is_exported(struct gpio_block *block)
+{
+	struct device		*dev;
+	struct sysfs_dirent	*sd = NULL;
+
+	mutex_lock(&sysfs_lock);
+	dev = class_find_device(&gpio_class, NULL, block, match_export);
+	if (!dev)
+		goto out;
+
+	sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value");
+
+out:
+	mutex_unlock(&sysfs_lock);
+	return sd ? 1 : 0;
+}
+
+static ssize_t gpio_block_exported_show(struct device *dev,
+					struct device_attribute *attr,
+					char *buf)
+{
+	struct gpio_block	*block = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", gpio_block_value_is_exported(block));
+}
+
+static int gpio_block_value_export(struct gpio_block *block)
+{
+	struct device	*dev;
+	int		status;
+	int		i;
+
+	mutex_lock(&sysfs_lock);
+
+	for (i = 0; i < block->ngpio; i++) {
+		status = gpio_request(block->gpio[i], "sysfs");
+		if (status)
+			goto out;
+	}
+
+	dev = class_find_device(&gpio_class, NULL, block, match_export);
+	if (!dev) {
+		status = -ENODEV;
+		goto out;
+	}
+
+	status = device_create_file(dev, &dev_attr_block_value);
+	if (status)
+		goto out;
+
+	mutex_unlock(&sysfs_lock);
+	return 0;
+
+out:
+	while (i > 0) {
+		i--;
+		gpio_free(block->gpio[i]);
+	}
+
+	mutex_unlock(&sysfs_lock);
+	return status;
+}
+
+static int gpio_block_value_unexport(struct gpio_block *block)
+{
+	struct device	*dev;
+	int		i;
+
+	dev = class_find_device(&gpio_class, NULL, block, match_export);
+	if (!dev)
+		return -ENODEV;
+
+	for (i = 0; i < block->ngpio; i++)
+		gpio_free(block->gpio[i]);
+
+	device_remove_file(dev, &dev_attr_block_value);
+
+	return 0;
+}
+
+static ssize_t gpio_block_exported_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t size)
+{
+	long	value;
+	int	status;
+	struct	gpio_block *block = dev_get_drvdata(dev);
+	int	exported = gpio_block_value_is_exported(block);
+
+	status = kstrtoul(buf, 0, &value);
+	if (status < 0)
+		goto err;
+
+	if (value != exported) {
+		if (value)
+			status = gpio_block_value_export(block);
+		else
+			status = gpio_block_value_unexport(block);
+		if (!status)
+			status = size;
+	} else {
+		status = size;
+	}
+err:
+	return status;
+}
+
+static DEVICE_ATTR(exported, 0644, gpio_block_exported_show,
+		   gpio_block_exported_store);
+
+static const struct attribute *gpio_block_attrs[] = {
+	&dev_attr_block_ngpio.attr,
+	&dev_attr_exported.attr,
+	NULL,
+};
+
+static const struct attribute_group gpio_block_attr_group = {
+	.attrs = (struct attribute **) gpio_block_attrs,
+};
+
+int gpio_block_export(struct gpio_block *block)
+{
+	int		status;
+	struct device	*dev;
+
+	/* can't export until sysfs is available ... */
+	if (!gpio_class.p) {
+		pr_debug("%s: called too early!\n", __func__);
+		return -ENOENT;
+	}
+
+	mutex_lock(&sysfs_lock);
+	dev = device_create(&gpio_class, NULL, MKDEV(0, 0), block,
+			    block->name);
+	if (!IS_ERR(dev))
+		status = sysfs_create_group(&dev->kobj, &gpio_block_attr_group);
+	else
+		status = PTR_ERR(dev);
+	mutex_unlock(&sysfs_lock);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_block_export);
+
+void gpio_block_unexport(struct gpio_block *block)
+{
+	struct device *dev;
+
+	mutex_lock(&sysfs_lock);
+	dev = class_find_device(&gpio_class, NULL, block, match_export);
+	if (dev)
+		device_unregister(dev);
+	mutex_unlock(&sysfs_lock);
+}
+EXPORT_SYMBOL_GPL(gpio_block_unexport);
+
 static int __init gpiolib_sysfs_init(void)
 {
 	int		status;
@@ -1882,7 +2094,14 @@ int gpio_block_register(struct gpio_bloc
 			break;
 		}
 	}
-	return i == NR_GPIO_BLOCKS ? -EBUSY : 0;
+	if (i == NR_GPIO_BLOCKS)
+		goto err;
+
+	gpio_block_export(block);
+
+	return 0;
+err:
+	return -EBUSY;
 }
 EXPORT_SYMBOL_GPL(gpio_block_register);
 
@@ -1896,6 +2115,11 @@ void gpio_block_unregister(struct gpio_b
 			break;
 		}
 	}
+
+	if (i == NR_GPIO_BLOCKS)
+		return;
+
+	gpio_block_unexport(block);
 }
 EXPORT_SYMBOL_GPL(gpio_block_unregister);
 
--- linux-2.6.orig/include/asm-generic/gpio.h
+++ linux-2.6/include/asm-generic/gpio.h
@@ -210,6 +210,8 @@ extern int gpio_export_link(struct devic
 			unsigned gpio);
 extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
 extern void gpio_unexport(unsigned gpio);
+extern int gpio_block_export(struct gpio_block *block);
+extern void gpio_block_unexport(struct gpio_block *block);
 
 #endif	/* CONFIG_GPIO_SYSFS */
 
@@ -269,6 +271,15 @@ static inline int gpio_sysfs_set_active_
 static inline void gpio_unexport(unsigned gpio)
 {
 }
+
+static inline int gpio_block_export(struct gpio_block *block)
+{
+	return -ENOSYS;
+}
+
+static inline void gpio_block_unexport(struct gpio_block *block)
+{
+}
 #endif	/* CONFIG_GPIO_SYSFS */
 
 #endif /* _ASM_GENERIC_GPIO_H */
--- linux-2.6.orig/include/linux/gpio.h
+++ linux-2.6/include/linux/gpio.h
@@ -278,6 +278,19 @@ static inline void gpio_unexport(unsigne
 	WARN_ON(1);
 }
 
+static inline int gpio_block_export(struct gpio_block *block)
+{
+	/* GPIO block can never have been requested or set as {in,out}put */
+	WARN_ON(1);
+	return -EINVAL;
+}
+
+static inline void gpio_block_unexport(struct gpio_block *block)
+{
+	/* GPIO block can never have been exported */
+	WARN_ON(1);
+}
+
 static inline int gpio_to_irq(unsigned gpio)
 {
 	/* GPIO can never have been requested or set as input */

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Ryan Mallon]

On 13/10/12 06:11, Roland Stigge wrote:
> This patch adds sysfs support to the block GPIO API.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>

Hi Roland,

Some comments below,

~Ryan

> 
> ---
>  Documentation/ABI/testing/sysfs-gpio |    6 
>  drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
>  include/asm-generic/gpio.h           |   11 +
>  include/linux/gpio.h                 |   13 ++
>  4 files changed, 254 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio
> +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio
> @@ -24,4 +24,8 @@ Description:
>  	    /base ... (r/o) same as N
>  	    /label ... (r/o) descriptive, not necessarily unique
>  	    /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
> -
> +	/blockN ... for each GPIO block #N
> +	    /ngpio ... (r/o) number of GPIOs in this group
> +	    /exported ... sysfs export state of this group (0, 1)
> +	    /value ... current value as 32 or 64 bit integer in decimal
> +                       (only available if /exported is 1)
> --- linux-2.6.orig/drivers/gpio/gpiolib.c
> +++ linux-2.6/drivers/gpio/gpiolib.c
> @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi
>  				chip->label, status);
>  }
>  
> +static ssize_t gpio_block_ngpio_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	const struct gpio_block	*block = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", block->ngpio);
> +}
> +static struct device_attribute
> +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL);

static DEVICE_ATTR(ngpio, S_IRUGO, gpio_block_ngpio_show, NULL);

> +
> +static ssize_t gpio_block_value_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	const struct gpio_block *block = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", gpio_block_get(block));

Printing the value of a bunch of pins as a decimal is a bit odd. Hex, or
a bitmap would be more appropriate.

> +}
> +
> +static bool gpio_block_is_output(struct gpio_block *block)
> +{
> +	int i;
> +
> +	for (i = 0; i < block->ngpio; i++)
> +		if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags))
> +			return false;

Shouldn't a block force all of the pins to be the same direction? Or at
least have gpio_block_set skip pins which aren't outputs.

> +	return true;
> +}
> +
> +static ssize_t gpio_block_value_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t size)
> +{
> +	ssize_t			status;
> +	struct gpio_block	*block = dev_get_drvdata(dev);
> +	unsigned long		value;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	status = kstrtoul(buf, 0, &value);
> +	if (status == 0) {

You don't need to do the kstrtoul under the lock:

	err = kstrtoul(buf, 0, &value);
	if (err)
		return err;

	mutex_lock(&sysfs_lock);
	...

Global lock is a bit lame, it serialises all of your bitbanged busses
against each other. Why is it not part of the gpio_block structure?


> +		if (gpio_block_is_output(block)) {
> +			gpio_block_set(block, value);
> +			status = size;
> +		} else {
> +			status = -EPERM;
> +		}
> +	}
> +
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static struct device_attribute
> +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show,
> +			      gpio_block_value_store);

Use DEVICE_ATTR and S_IWUSR | S_IRUGO permission macros.

> +
> +static int gpio_block_value_is_exported(struct gpio_block *block)
> +{
> +	struct device		*dev;
> +	struct sysfs_dirent	*sd = NULL;
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (!dev)
> +		goto out;
> +
> +	sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value");
> +
> +out:
> +	mutex_unlock(&sysfs_lock);
> +	return sd ? 1 : 0;

  return sd;

or:

  return !!sd;

> +}
> +
> +static ssize_t gpio_block_exported_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct gpio_block	*block = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", gpio_block_value_is_exported(block));
> +}
> +
> +static int gpio_block_value_export(struct gpio_block *block)
> +{
> +	struct device	*dev;
> +	int		status;
> +	int		i;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	for (i = 0; i < block->ngpio; i++) {
> +		status = gpio_request(block->gpio[i], "sysfs");
> +		if (status)
> +			goto out;
> +	}
> +
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (!dev) {
> +		status = -ENODEV;
> +		goto out;
> +	}
> +
> +	status = device_create_file(dev, &dev_attr_block_value);
> +	if (status)
> +		goto out;
> +
> +	mutex_unlock(&sysfs_lock);
> +	return 0;
> +
> +out:
> +	while (i > 0) {
> +		i--;
> +		gpio_free(block->gpio[i]);
> +	}

Nitpick:

	while (--i >= 0)
		gpio_free(block->gpio[i]);

> +
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static int gpio_block_value_unexport(struct gpio_block *block)
> +{
> +	struct device	*dev;
> +	int		i;
> +
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	for (i = 0; i < block->ngpio; i++)
> +		gpio_free(block->gpio[i]);
> +
> +	device_remove_file(dev, &dev_attr_block_value);
> +
> +	return 0;
> +}
> +
> +static ssize_t gpio_block_exported_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t size)
> +{
> +	long	value;
> +	int	status;
> +	struct	gpio_block *block = dev_get_drvdata(dev);
> +	int	exported = gpio_block_value_is_exported(block);
> +
> +	status = kstrtoul(buf, 0, &value);
> +	if (status < 0)
> +		goto err;
> +
> +	if (value != exported) {
> +		if (value)
> +			status = gpio_block_value_export(block);
> +		else
> +			status = gpio_block_value_unexport(block);
> +		if (!status)
> +			status = size;
> +	} else {
> +		status = size;
> +	}
> +err:
> +	return status;
> +}
> +
> +static DEVICE_ATTR(exported, 0644, gpio_block_exported_show,
> +		   gpio_block_exported_store);
> +
> +static const struct attribute *gpio_block_attrs[] = {
> +	&dev_attr_block_ngpio.attr,
> +	&dev_attr_exported.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group gpio_block_attr_group = {
> +	.attrs = (struct attribute **) gpio_block_attrs,
> +};
> +
> +int gpio_block_export(struct gpio_block *block)
> +{
> +	int		status;
> +	struct device	*dev;
> +
> +	/* can't export until sysfs is available ... */
> +	if (!gpio_class.p) {
> +		pr_debug("%s: called too early!\n", __func__);
> +		return -ENOENT;
> +	}
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = device_create(&gpio_class, NULL, MKDEV(0, 0), block,
> +			    block->name);
> +	if (!IS_ERR(dev))
> +		status = sysfs_create_group(&dev->kobj, &gpio_block_attr_group);
> +	else
> +		status = PTR_ERR(dev);
> +	mutex_unlock(&sysfs_lock);
> +
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_export);
> +
> +void gpio_block_unexport(struct gpio_block *block)
> +{
> +	struct device *dev;
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (dev)
> +		device_unregister(dev);
> +	mutex_unlock(&sysfs_lock);
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_unexport);
> +
>  static int __init gpiolib_sysfs_init(void)
>  {
>  	int		status;
> @@ -1882,7 +2094,14 @@ int gpio_block_register(struct gpio_bloc
>  			break;
>  		}
>  	}
> -	return i == NR_GPIO_BLOCKS ? -EBUSY : 0;
> +	if (i == NR_GPIO_BLOCKS)
> +		goto err;
> +
> +	gpio_block_export(block);
> +
> +	return 0;
> +err:
> +	return -EBUSY;
>  }
>  EXPORT_SYMBOL_GPL(gpio_block_register);
>  
> @@ -1896,6 +2115,11 @@ void gpio_block_unregister(struct gpio_b
>  			break;
>  		}
>  	}
> +
> +	if (i == NR_GPIO_BLOCKS)
> +		return;
> +
> +	gpio_block_unexport(block);
>  }
>  EXPORT_SYMBOL_GPL(gpio_block_unregister);
>  
> --- linux-2.6.orig/include/asm-generic/gpio.h
> +++ linux-2.6/include/asm-generic/gpio.h
> @@ -210,6 +210,8 @@ extern int gpio_export_link(struct devic
>  			unsigned gpio);
>  extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
>  extern void gpio_unexport(unsigned gpio);
> +extern int gpio_block_export(struct gpio_block *block);
> +extern void gpio_block_unexport(struct gpio_block *block);
>  
>  #endif	/* CONFIG_GPIO_SYSFS */
>  
> @@ -269,6 +271,15 @@ static inline int gpio_sysfs_set_active_
>  static inline void gpio_unexport(unsigned gpio)
>  {
>  }
> +
> +static inline int gpio_block_export(struct gpio_block *block)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void gpio_block_unexport(struct gpio_block *block)
> +{
> +}
>  #endif	/* CONFIG_GPIO_SYSFS */
>  
>  #endif /* _ASM_GENERIC_GPIO_H */
> --- linux-2.6.orig/include/linux/gpio.h
> +++ linux-2.6/include/linux/gpio.h
> @@ -278,6 +278,19 @@ static inline void gpio_unexport(unsigne
>  	WARN_ON(1);
>  }
>  
> +static inline int gpio_block_export(struct gpio_block *block)
> +{
> +	/* GPIO block can never have been requested or set as {in,out}put */
> +	WARN_ON(1);
> +	return -EINVAL;
> +}
> +
> +static inline void gpio_block_unexport(struct gpio_block *block)
> +{
> +	/* GPIO block can never have been exported */
> +	WARN_ON(1);
> +}
> +
>  static inline int gpio_to_irq(unsigned gpio)
>  {
>  	/* GPIO can never have been requested or set as input */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Roland Stigge]

On 10/15/2012 07:35 AM, Ryan Mallon wrote:
>> --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio
>> +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio
>> @@ -24,4 +24,8 @@ Description:
>>  	    /base ... (r/o) same as N
>>  	    /label ... (r/o) descriptive, not necessarily unique
>>  	    /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
>> -
>> +	/blockN ... for each GPIO block #N
>> +	    /ngpio ... (r/o) number of GPIOs in this group
>> +	    /exported ... sysfs export state of this group (0, 1)
>> +	    /value ... current value as 32 or 64 bit integer in decimal
>> +                       (only available if /exported is 1)
>> --- linux-2.6.orig/drivers/gpio/gpiolib.c
>> +++ linux-2.6/drivers/gpio/gpiolib.c
>> @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi
>>  				chip->label, status);
>>  }
>>  
>> +static ssize_t gpio_block_ngpio_show(struct device *dev,
>> +				     struct device_attribute *attr, char *buf)
>> +{
>> +	const struct gpio_block	*block = dev_get_drvdata(dev);
>> +
>> +	return sprintf(buf, "%u\n", block->ngpio);
>> +}
>> +static struct device_attribute
>> +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL);
> 
> static DEVICE_ATTR(ngpio, S_IRUGO, gpio_block_ngpio_show, NULL);

Of course I would have used this. :-) But DEVICE_ATTR isn't flexible
enough here. There is already another "ngpio" in this file (resulting in
its name "dev_attr_ngpio") for gpio chips, colliding with this attribute
here (only on C source level - for sysfs it's fine).

Using S_IRUGO - thanks.

>> +}
>> +
>> +static bool gpio_block_is_output(struct gpio_block *block)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < block->ngpio; i++)
>> +		if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags))
>> +			return false;
> 
> Shouldn't a block force all of the pins to be the same direction? Or at
> least have gpio_block_set skip pins which aren't outputs.

It is again analogous to GPIOs themselves: The sysfs interface prevents
Oopses by checking for the direction with the above function. Otherwise,
the user is responsible for requesting and setting direction.

>> +static ssize_t gpio_block_value_store(struct device *dev,
>> +				      struct device_attribute *attr,
>> +				      const char *buf, size_t size)
>> +{
>> +	ssize_t			status;
>> +	struct gpio_block	*block = dev_get_drvdata(dev);
>> +	unsigned long		value;
>> +
>> +	mutex_lock(&sysfs_lock);
>> +
>> +	status = kstrtoul(buf, 0, &value);
>> +	if (status == 0) {
> 
> You don't need to do the kstrtoul under the lock:
> 
> 	err = kstrtoul(buf, 0, &value);
> 	if (err)
> 		return err;
> 
> 	mutex_lock(&sysfs_lock);
> 	...
> 
> Global lock is a bit lame, it serialises all of your bitbanged busses
> against each other. Why is it not part of the gpio_block structure?

It's the same strategy as for GPIO value get/set.

More importantly maybe: Consider 32 GPIO lines on a single GPIO
controller. Several defined, say, 8 bit buses defined on this single
hardware word actually need to be locked against each other.

So sticking with it for now.

>> +		if (gpio_block_is_output(block)) {
>> +			gpio_block_set(block, value);
>> +			status = size;
>> +		} else {
>> +			status = -EPERM;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&sysfs_lock);
>> +	return status;
>> +}
>> +
>> +static struct device_attribute
>> +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show,
>> +			      gpio_block_value_store);
> 
> Use DEVICE_ATTR and S_IWUSR | S_IRUGO permission macros.

Regarding DEVICE_ATTR as above. But adopting S_IWUSR | S_IRUGO - thanks.

Again, including all the other suggestions in the next update.

Thanks,

Roland

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Jean-Christophe PLAGNIOL-VILLARD]

On 21:11 Fri 12 Oct     , Roland Stigge wrote:
> This patch adds sysfs support to the block GPIO API.
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> ---
>  Documentation/ABI/testing/sysfs-gpio |    6 
>  drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
>  include/asm-generic/gpio.h           |   11 +
>  include/linux/gpio.h                 |   13 ++
>  4 files changed, 254 insertions(+), 2 deletions(-)
I really don't like this sysfs we need to add a specific device with ioctl

I put Greg in Cc

Best Regards,
J.
> 
> --- linux-2.6.orig/Documentation/ABI/testing/sysfs-gpio
> +++ linux-2.6/Documentation/ABI/testing/sysfs-gpio
> @@ -24,4 +24,8 @@ Description:
>  	    /base ... (r/o) same as N
>  	    /label ... (r/o) descriptive, not necessarily unique
>  	    /ngpio ... (r/o) number of GPIOs; numbered N to N + (ngpio - 1)
> -
> +	/blockN ... for each GPIO block #N
> +	    /ngpio ... (r/o) number of GPIOs in this group
> +	    /exported ... sysfs export state of this group (0, 1)
> +	    /value ... current value as 32 or 64 bit integer in decimal
> +                       (only available if /exported is 1)
> --- linux-2.6.orig/drivers/gpio/gpiolib.c
> +++ linux-2.6/drivers/gpio/gpiolib.c
> @@ -974,6 +974,218 @@ static void gpiochip_unexport(struct gpi
>  				chip->label, status);
>  }
>  
> +static ssize_t gpio_block_ngpio_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	const struct gpio_block	*block = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", block->ngpio);
> +}
> +static struct device_attribute
> +dev_attr_block_ngpio = __ATTR(ngpio, 0444, gpio_block_ngpio_show, NULL);
> +
> +static ssize_t gpio_block_value_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	const struct gpio_block *block = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", gpio_block_get(block));
> +}
> +
> +static bool gpio_block_is_output(struct gpio_block *block)
> +{
> +	int i;
> +
> +	for (i = 0; i < block->ngpio; i++)
> +		if (!test_bit(FLAG_IS_OUT, &gpio_desc[block->gpio[i]].flags))
> +			return false;
> +	return true;
> +}
> +
> +static ssize_t gpio_block_value_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t size)
> +{
> +	ssize_t			status;
> +	struct gpio_block	*block = dev_get_drvdata(dev);
> +	unsigned long		value;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	status = kstrtoul(buf, 0, &value);
> +	if (status == 0) {
> +		if (gpio_block_is_output(block)) {
> +			gpio_block_set(block, value);
> +			status = size;
> +		} else {
> +			status = -EPERM;
> +		}
> +	}
> +
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static struct device_attribute
> +dev_attr_block_value = __ATTR(value, 0644, gpio_block_value_show,
> +			      gpio_block_value_store);
> +
> +static int gpio_block_value_is_exported(struct gpio_block *block)
> +{
> +	struct device		*dev;
> +	struct sysfs_dirent	*sd = NULL;
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (!dev)
> +		goto out;
> +
> +	sd = sysfs_get_dirent(dev->kobj.sd, NULL, "value");
> +
> +out:
> +	mutex_unlock(&sysfs_lock);
> +	return sd ? 1 : 0;
> +}
> +
> +static ssize_t gpio_block_exported_show(struct device *dev,
> +					struct device_attribute *attr,
> +					char *buf)
> +{
> +	struct gpio_block	*block = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", gpio_block_value_is_exported(block));
> +}
> +
> +static int gpio_block_value_export(struct gpio_block *block)
> +{
> +	struct device	*dev;
> +	int		status;
> +	int		i;
> +
> +	mutex_lock(&sysfs_lock);
> +
> +	for (i = 0; i < block->ngpio; i++) {
> +		status = gpio_request(block->gpio[i], "sysfs");
> +		if (status)
> +			goto out;
> +	}
> +
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (!dev) {
> +		status = -ENODEV;
> +		goto out;
> +	}
> +
> +	status = device_create_file(dev, &dev_attr_block_value);
> +	if (status)
> +		goto out;
> +
> +	mutex_unlock(&sysfs_lock);
> +	return 0;
> +
> +out:
> +	while (i > 0) {
> +		i--;
> +		gpio_free(block->gpio[i]);
> +	}
> +
> +	mutex_unlock(&sysfs_lock);
> +	return status;
> +}
> +
> +static int gpio_block_value_unexport(struct gpio_block *block)
> +{
> +	struct device	*dev;
> +	int		i;
> +
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (!dev)
> +		return -ENODEV;
> +
> +	for (i = 0; i < block->ngpio; i++)
> +		gpio_free(block->gpio[i]);
> +
> +	device_remove_file(dev, &dev_attr_block_value);
> +
> +	return 0;
> +}
> +
> +static ssize_t gpio_block_exported_store(struct device *dev,
> +					 struct device_attribute *attr,
> +					 const char *buf, size_t size)
> +{
> +	long	value;
> +	int	status;
> +	struct	gpio_block *block = dev_get_drvdata(dev);
> +	int	exported = gpio_block_value_is_exported(block);
> +
> +	status = kstrtoul(buf, 0, &value);
> +	if (status < 0)
> +		goto err;
> +
> +	if (value != exported) {
> +		if (value)
> +			status = gpio_block_value_export(block);
> +		else
> +			status = gpio_block_value_unexport(block);
> +		if (!status)
> +			status = size;
> +	} else {
> +		status = size;
> +	}
> +err:
> +	return status;
> +}
> +
> +static DEVICE_ATTR(exported, 0644, gpio_block_exported_show,
> +		   gpio_block_exported_store);
> +
> +static const struct attribute *gpio_block_attrs[] = {
> +	&dev_attr_block_ngpio.attr,
> +	&dev_attr_exported.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group gpio_block_attr_group = {
> +	.attrs = (struct attribute **) gpio_block_attrs,
> +};
> +
> +int gpio_block_export(struct gpio_block *block)
> +{
> +	int		status;
> +	struct device	*dev;
> +
> +	/* can't export until sysfs is available ... */
> +	if (!gpio_class.p) {
> +		pr_debug("%s: called too early!\n", __func__);
> +		return -ENOENT;
> +	}
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = device_create(&gpio_class, NULL, MKDEV(0, 0), block,
> +			    block->name);
> +	if (!IS_ERR(dev))
> +		status = sysfs_create_group(&dev->kobj, &gpio_block_attr_group);
> +	else
> +		status = PTR_ERR(dev);
> +	mutex_unlock(&sysfs_lock);
> +
> +	return status;
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_export);
> +
> +void gpio_block_unexport(struct gpio_block *block)
> +{
> +	struct device *dev;
> +
> +	mutex_lock(&sysfs_lock);
> +	dev = class_find_device(&gpio_class, NULL, block, match_export);
> +	if (dev)
> +		device_unregister(dev);
> +	mutex_unlock(&sysfs_lock);
> +}
> +EXPORT_SYMBOL_GPL(gpio_block_unexport);
> +
>  static int __init gpiolib_sysfs_init(void)
>  {
>  	int		status;
> @@ -1882,7 +2094,14 @@ int gpio_block_register(struct gpio_bloc
>  			break;
>  		}
>  	}
> -	return i == NR_GPIO_BLOCKS ? -EBUSY : 0;
> +	if (i == NR_GPIO_BLOCKS)
> +		goto err;
> +
> +	gpio_block_export(block);
> +
> +	return 0;
> +err:
> +	return -EBUSY;
>  }
>  EXPORT_SYMBOL_GPL(gpio_block_register);
>  
> @@ -1896,6 +2115,11 @@ void gpio_block_unregister(struct gpio_b
>  			break;
>  		}
>  	}
> +
> +	if (i == NR_GPIO_BLOCKS)
> +		return;
> +
> +	gpio_block_unexport(block);
>  }
>  EXPORT_SYMBOL_GPL(gpio_block_unregister);
>  
> --- linux-2.6.orig/include/asm-generic/gpio.h
> +++ linux-2.6/include/asm-generic/gpio.h
> @@ -210,6 +210,8 @@ extern int gpio_export_link(struct devic
>  			unsigned gpio);
>  extern int gpio_sysfs_set_active_low(unsigned gpio, int value);
>  extern void gpio_unexport(unsigned gpio);
> +extern int gpio_block_export(struct gpio_block *block);
> +extern void gpio_block_unexport(struct gpio_block *block);
>  
>  #endif	/* CONFIG_GPIO_SYSFS */
>  
> @@ -269,6 +271,15 @@ static inline int gpio_sysfs_set_active_
>  static inline void gpio_unexport(unsigned gpio)
>  {
>  }
> +
> +static inline int gpio_block_export(struct gpio_block *block)
> +{
> +	return -ENOSYS;
> +}
> +
> +static inline void gpio_block_unexport(struct gpio_block *block)
> +{
> +}
>  #endif	/* CONFIG_GPIO_SYSFS */
>  
>  #endif /* _ASM_GENERIC_GPIO_H */
> --- linux-2.6.orig/include/linux/gpio.h
> +++ linux-2.6/include/linux/gpio.h
> @@ -278,6 +278,19 @@ static inline void gpio_unexport(unsigne
>  	WARN_ON(1);
>  }
>  
> +static inline int gpio_block_export(struct gpio_block *block)
> +{
> +	/* GPIO block can never have been requested or set as {in,out}put */
> +	WARN_ON(1);
> +	return -EINVAL;
> +}
> +
> +static inline void gpio_block_unexport(struct gpio_block *block)
> +{
> +	/* GPIO block can never have been exported */
> +	WARN_ON(1);
> +}
> +
>  static inline int gpio_to_irq(unsigned gpio)
>  {
>  	/* GPIO can never have been requested or set as input */

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Roland Stigge]

On 10/15/2012 08:07 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 21:11 Fri 12 Oct     , Roland Stigge wrote:
>> This patch adds sysfs support to the block GPIO API.
>>
>> Signed-off-by: Roland Stigge <stigge@antcom.de>
>>
>> ---
>>  Documentation/ABI/testing/sysfs-gpio |    6 
>>  drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
>>  include/asm-generic/gpio.h           |   11 +
>>  include/linux/gpio.h                 |   13 ++
>>  4 files changed, 254 insertions(+), 2 deletions(-)
> I really don't like this sysfs we need to add a specific device with ioctl

The good thing about the current style is that you have a single value
for the whole block in a single file just as for single GPIOs. You can
consider it as a generalized GPIO.

And I like it. :-)

Thanks for considering,

Roland

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Greg Kroah-Hartman]

On Mon, Oct 15, 2012 at 08:07:02PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 21:11 Fri 12 Oct     , Roland Stigge wrote:
> > This patch adds sysfs support to the block GPIO API.
> > 
> > Signed-off-by: Roland Stigge <stigge@antcom.de>
> > 
> > ---
> >  Documentation/ABI/testing/sysfs-gpio |    6 
> >  drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
> >  include/asm-generic/gpio.h           |   11 +
> >  include/linux/gpio.h                 |   13 ++
> >  4 files changed, 254 insertions(+), 2 deletions(-)
> I really don't like this sysfs we need to add a specific device with ioctl

Why?

> I put Greg in Cc

Again, why?

greg k-h

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Linus Walleij]

I really request Grant to comment on this...too.

On Mon, Oct 15, 2012 at 8:19 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Mon, Oct 15, 2012 at 08:07:02PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
>> On 21:11 Fri 12 Oct     , Roland Stigge wrote:
>> > This patch adds sysfs support to the block GPIO API.
>> >
>> > Signed-off-by: Roland Stigge <stigge@antcom.de>
>> >
>> > ---
>> >  Documentation/ABI/testing/sysfs-gpio |    6
>> >  drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
>> >  include/asm-generic/gpio.h           |   11 +
>> >  include/linux/gpio.h                 |   13 ++
>> >  4 files changed, 254 insertions(+), 2 deletions(-)
>> I really don't like this sysfs we need to add a specific device with ioctl
>
> Why?

I don't like it either, basically because the GPIO sysfs is not
entirely sound.

Another patch that is circulating concerns edge triggers and similar,
and it appear that some parts of the GPIO sysfs is for example
redefining and exporting IRQchip properties like trigger edge
in sysfs, while the settings of the irqchip actually used by the driver
is not reflected in the other direction. So you can *set* these things
by writing in the GPIO sysfs, but never trust what you *read* from
there. And you can set what edges an IRQ will trigger on a certain
GPIO, and the way to handle the IRQs from usespace is to poll
on a value. This is not really documented but well ...

Part of me just want to delete that, but I can't because it's now
an ABI.

The "devices" that the sysfs files are tied to are not real devices,
instead the code look like this: whenever a gpio is exported to
sysfs, the code calls (drivers/gpio/gpiolib.c):

    dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
                    desc, ioname ? ioname : "gpio%u", gpio);

Mock device just to get a sysfs opening. And once device for
every GPIO with no hierarchical correspondence to anything
in the system.

The thing is that struct gpio_chip is not a device at all, it's something
else.

This inconsistency in the GPIO sysfs implementation make me
fear adding new stuff to it. The other problems need fixing first.

The reason an ioctl() IMO is better suited to do the job is that
it can properly represent a multiple-value operation on several
GPIOs at the same time in a struct, and it can conversely inform
userspace about which GPIOs may be a block of signals that
can be fired simultaneously instead of going to string parsing
and binary values in sysfs which look like worse hacks to me.

The last thing I'm a bit softer on though. Mainly I fear of this
sysfs ABI growing into a beast.

It was all merged prior to Grant becoming maintainer and
me becoming co-maintainer of it, so this is legacy business.

Sadly the main creator of this ABI is David Brownell who is
not able to respond nor maintain it from where he is now. But
we need to think hard about what we shall do with this particular
piece of legacy. Some of the stuff was added by Daniel
Gl?ckner so requesting advice from him.

Daniel: are you interested in helping us fixing the GPIOlib
sysfs ABI and kernel internals? I'm a bit afraid of it.

Yours,
Linus Walleij

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Roland Stigge]

On 15/10/12 22:30, Linus Walleij wrote:
> I don't like it either, basically because the GPIO sysfs is not
> entirely sound.
> 
> Another patch that is circulating concerns edge triggers and similar,
> and it appear that some parts of the GPIO sysfs is for example
> redefining and exporting IRQchip properties like trigger edge
> in sysfs, while the settings of the irqchip actually used by the driver
> is not reflected in the other direction. So you can *set* these things
> by writing in the GPIO sysfs, but never trust what you *read* from
> there. And you can set what edges an IRQ will trigger on a certain
> GPIO, and the way to handle the IRQs from usespace is to poll
> on a value. This is not really documented but well ...

I'm not convinced this generally also applies to the block GPIO patches.
Or that it can't be fixed.

> The reason an ioctl() IMO is better suited to do the job is that
> it can properly represent a multiple-value operation on several
> GPIOs at the same time in a struct, and it can conversely inform
> userspace about which GPIOs may be a block of signals that
> can be fired simultaneously instead of going to string parsing
> and binary values in sysfs which look like worse hacks to me.

There is no binary values in the sysfs for the block GPIO patches, and
regarding string parsing - yes, that's sysfs. :-)

Roland

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Jean-Christophe PLAGNIOL-VILLARD]

On 22:30 Mon 15 Oct     , Linus Walleij wrote:
> I really request Grant to comment on this...too.
> 
> On Mon, Oct 15, 2012 at 8:19 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Mon, Oct 15, 2012 at 08:07:02PM +0200, Jean-Christophe PLAGNIOL-VILLARD wrote:
> >> On 21:11 Fri 12 Oct     , Roland Stigge wrote:
> >> > This patch adds sysfs support to the block GPIO API.
> >> >
> >> > Signed-off-by: Roland Stigge <stigge@antcom.de>
> >> >
> >> > ---
> >> >  Documentation/ABI/testing/sysfs-gpio |    6
> >> >  drivers/gpio/gpiolib.c               |  226 ++++++++++++++++++++++++++++++++++-
> >> >  include/asm-generic/gpio.h           |   11 +
> >> >  include/linux/gpio.h                 |   13 ++
> >> >  4 files changed, 254 insertions(+), 2 deletions(-)
> >> I really don't like this sysfs we need to add a specific device with ioctl
> >
> > Why?
> 
> I don't like it either, basically because the GPIO sysfs is not
> entirely sound.
> 
> Another patch that is circulating concerns edge triggers and similar,
> and it appear that some parts of the GPIO sysfs is for example
> redefining and exporting IRQchip properties like trigger edge
> in sysfs, while the settings of the irqchip actually used by the driver
> is not reflected in the other direction. So you can *set* these things
> by writing in the GPIO sysfs, but never trust what you *read* from
> there. And you can set what edges an IRQ will trigger on a certain
> GPIO, and the way to handle the IRQs from usespace is to poll
> on a value. This is not really documented but well ...
> 
> Part of me just want to delete that, but I can't because it's now
> an ABI.
> 
> The "devices" that the sysfs files are tied to are not real devices,
> instead the code look like this: whenever a gpio is exported to
> sysfs, the code calls (drivers/gpio/gpiolib.c):
> 
>     dev = device_create(&gpio_class, desc->chip->dev, MKDEV(0, 0),
>                     desc, ioname ? ioname : "gpio%u", gpio);
> 
> Mock device just to get a sysfs opening. And once device for
> every GPIO with no hierarchical correspondence to anything
> in the system.
> 
> The thing is that struct gpio_chip is not a device at all, it's something
> else.
> 
> This inconsistency in the GPIO sysfs implementation make me
> fear adding new stuff to it. The other problems need fixing first.
> 
> The reason an ioctl() IMO is better suited to do the job is that
> it can properly represent a multiple-value operation on several
> GPIOs at the same time in a struct, and it can conversely inform
> userspace about which GPIOs may be a block of signals that
> can be fired simultaneously instead of going to string parsing
> and binary values in sysfs which look like worse hacks to me.
> 
> The last thing I'm a bit softer on though. Mainly I fear of this
> sysfs ABI growing into a beast.
> 
> It was all merged prior to Grant becoming maintainer and
> me becoming co-maintainer of it, so this is legacy business.
> 
> Sadly the main creator of this ABI is David Brownell who is
> not able to respond nor maintain it from where he is now. But
> we need to think hard about what we shall do with this particular
> piece of legacy. Some of the stuff was added by Daniel
> Gl?ckner so requesting advice from him.
> 
> Daniel: are you interested in helping us fixing the GPIOlib
> sysfs ABI and kernel internals? I'm a bit afraid of it.
My 0.02$ too

Best Regards,
J.

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Daniel Glöckner]

Hi,
sorry for the late reply. I'm currently on vacation and it is no fun to
use SSH with a 1s latency while Entel Chile injects RST/ACK packets.
I'll read the remaining related mails that have accumulated in my inbox
when I'm back home.

On Mon, Oct 15, 2012 at 10:30:15PM +0200, Linus Walleij wrote:
> Another patch that is circulating concerns edge triggers and similar,
> and it appear that some parts of the GPIO sysfs is for example
> redefining and exporting IRQchip properties like trigger edge
> in sysfs, while the settings of the irqchip actually used by the driver
> is not reflected in the other direction. So you can *set* these things
> by writing in the GPIO sysfs, but never trust what you *read* from
> there. And you can set what edges an IRQ will trigger on a certain
> GPIO, and the way to handle the IRQs from usespace is to poll
> on a value. This is not really documented but well ...

Part of this sounds like you are not familiar with the GPIOlib sysfs
IRQ stuff. The trigger edge set in sysfs is only used when userspace
polls the GPIO via sysfs. Drivers that want to register a gpio IRQ
should IMHO always explicitly request the edge/level to trigger on and
they should request the gpio beforehand. This prevents the gpio from
being exported to userspace. Only IRQ triggers accepted by the irq chip
are settable in sysfs, so you can trust the value read from that file.

> Sadly the main creator of this ABI is David Brownell who is
> not able to respond nor maintain it from where he is now. But
> we need to think hard about what we shall do with this particular
> piece of legacy. Some of the stuff was added by Daniel
> Gl?ckner so requesting advice from him.

I'm only guilty of adding the IRQ sysfs interface.

> Daniel: are you interested in helping us fixing the GPIOlib
> sysfs ABI and kernel internals? I'm a bit afraid of it.

Actually I don't know what you want to change to fix the existing sysfs
ABI. Personally I'd like to see the following things changed:
 - /sys/gpio/.../direction does not correspond to hardware before first
   use of gpio_direction_* due to lack of gpio_get_direction.
 - Names given to gpios by the chip should just result in symlinks to
   the usual gpioX directories or (un)exporting of gpios should accept
   names.

Best regards,

  Daniel

[PATCH RFC 2/6 v3] gpio: Add sysfs support to block GPIO API

[Linus Walleij]

On Thu, Oct 18, 2012 at 6:38 AM, Daniel Gl?ckner <daniel-gl@gmx.net> wrote:
> On Mon, Oct 15, 2012 at 10:30:15PM +0200, Linus Walleij wrote:
>>
>> Another patch that is circulating concerns edge triggers and similar,
>> and it appear that some parts of the GPIO sysfs is for example
>> redefining and exporting IRQchip properties like trigger edge
>> in sysfs, while the settings of the irqchip actually used by the driver
>> is not reflected in the other direction. So you can *set* these things
>> by writing in the GPIO sysfs, but never trust what you *read* from
>> there. And you can set what edges an IRQ will trigger on a certain
>> GPIO, and the way to handle the IRQs from usespace is to poll
>> on a value. This is not really documented but well ...
>
> Part of this sounds like you are not familiar with the GPIOlib sysfs
> IRQ stuff.

Yeah you bet :-)

I'm just trying to maintain it, I think I need your help with this.

Do you think it'd be possible for you to augment the
Documentation/gpio.txt file with some userspace code examples?
I think this could be very useful.

> The trigger edge set in sysfs is only used when userspace
> polls the GPIO via sysfs. Drivers that want to register a gpio IRQ
> should IMHO always explicitly request the edge/level to trigger on and
> they should request the gpio beforehand. This prevents the gpio from
> being exported to userspace. Only IRQ triggers accepted by the irq chip
> are settable in sysfs, so you can trust the value read from that file.

OK.

>> Daniel: are you interested in helping us fixing the GPIOlib
>> sysfs ABI and kernel internals? I'm a bit afraid of it.
>
> Actually I don't know what you want to change to fix the existing sysfs
> ABI. Personally I'd like to see the following things changed:
>  - /sys/gpio/.../direction does not correspond to hardware before first
>    use of gpio_direction_* due to lack of gpio_get_direction.
>  - Names given to gpios by the chip should just result in symlinks to
>    the usual gpioX directories or (un)exporting of gpios should accept
>    names.

Please send a patch! I'll merge.

Actually I'm mostly referring to another floating patch, I will try to
dig it up and CC you.

Yours,
Linus Walleij

[PATCH RFC 3/6 v3] gpio: Add device tree support to block GPIO API

[Roland Stigge]

This patch adds device tree support to the block GPIO API.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 Documentation/devicetree/bindings/gpio/gpio-block.txt |   36 +++++++
 drivers/gpio/Makefile                                 |    1 
 drivers/gpio/gpioblock-of.c                           |   84 ++++++++++++++++++
 3 files changed, 121 insertions(+)

--- /dev/null
+++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio-block.txt
@@ -0,0 +1,36 @@
+Block GPIO definition
+=====================
+
+This binding specifies arbitrary blocks of gpios, combining gpios from one or
+more GPIO controllers together, to form a word for r/w access.
+
+Required property:
+- compatible: must be "linux,gpio-block"
+
+Required subnodes:
+- the name will be the registered name of the block
+- property "gpios" is a list of gpios for the respective block
+
+Example:
+
+        blockgpio {
+                compatible = "linux,gpio-block";
+
+                block0 {
+                        gpios = <&gpio 3 0 0>,
+                                <&gpio 3 1 0>;
+                };
+                block1 {
+                        gpios = <&gpio 4 1 0>,
+                                <&gpio 4 3 0>,
+                                <&gpio 4 2 0>,
+                                <&gpio 4 4 0>,
+                                <&gpio 4 5 0>,
+                                <&gpio 4 6 0>,
+                                <&gpio 4 7 0>,
+                                <&gpio 4 8 0>,
+                                <&gpio 4 9 0>,
+                                <&gpio 4 10 0>,
+                                <&gpio 4 19 0>;
+                };
+        };
--- linux-2.6.orig/drivers/gpio/Makefile
+++ linux-2.6/drivers/gpio/Makefile
@@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO)	+= -DDEBUG
 
 obj-$(CONFIG_GPIOLIB)		+= gpiolib.o devres.o
 obj-$(CONFIG_OF_GPIO)		+= gpiolib-of.o
+obj-$(CONFIG_OF_GPIO)		+= gpioblock-of.o
 
 # Device drivers. Generally keep list sorted alphabetically
 obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
--- /dev/null
+++ linux-2.6/drivers/gpio/gpioblock-of.c
@@ -0,0 +1,84 @@
+/*
+ * OF implementation for Block GPIO
+ *
+ * Copyright (C) 2012 Roland Stigge <stigge@antcom.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/err.h>
+
+static int __devinit gpioblock_of_probe(struct platform_device *pdev)
+{
+	struct device_node *block;
+	unsigned *gpios;
+	int ngpio;
+	int ret;
+	struct gpio_block *gb;
+
+	for_each_available_child_of_node(pdev->dev.of_node, block) {
+		int i;
+
+		ngpio = of_gpio_count(block);
+		gpios = kzalloc(ngpio * sizeof(*gpios), GFP_KERNEL);
+		if (!gpios)
+			return -ENOMEM;
+		for (i = 0; i < ngpio; i++) {
+			ret = of_get_gpio(block, i);
+			if (ret < 0)
+				return ret; /* expect -EPROBE_DEFER */
+			gpios[i] = ret;
+		}
+		gb = gpio_block_create(gpios, ngpio, block->name);
+		if (IS_ERR(gb)) {
+			dev_err(&pdev->dev,
+				"Error creating GPIO block from device tree\n");
+			return PTR_ERR(gb);
+		}
+		ret = gpio_block_register(gb);
+		if (ret < 0) {
+			gpio_block_free(gb);
+			return ret;
+		}
+		kfree(gpios);
+		dev_info(&pdev->dev, "Registered gpio block %s: %d gpios\n",
+			 block->name, ngpio);
+	}
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static struct of_device_id gpioblock_of_match[] __devinitdata = {
+	{ .compatible = "linux,gpio-block", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, gpioblock_of_match);
+#endif
+
+static struct platform_driver gpioblock_of_driver = {
+	.driver	= {
+		.name = "gpio-block",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(gpioblock_of_match),
+
+	},
+	.probe	= gpioblock_of_probe,
+};
+
+module_platform_driver(gpioblock_of_driver);
+
+MODULE_DESCRIPTION("GPIO Block driver");
+MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:gpioblock-of");

[PATCH RFC 4/6 v3] gpio-max730x: Add block GPIO API

[Roland Stigge]

This patch adds block GPIO API support to the MAX730x driver.

Due to hardware constraints in this chip, simultaneous access to GPIO lines can
only be done in groups of 8: GPIOs 0-7, 8-15, 16-23, 24-27. However, setting
and clearing will be done at once.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 drivers/gpio/gpio-max730x.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

--- linux-2.6.orig/drivers/gpio/gpio-max730x.c
+++ linux-2.6/drivers/gpio/gpio-max730x.c
@@ -146,6 +146,43 @@ static int max7301_get(struct gpio_chip
 	return level;
 }
 
+static unsigned max7301_get_block(struct gpio_chip *chip, unsigned mask)
+{
+	struct max7301 *ts = container_of(chip, struct max7301, chip);
+	int i, j;
+	unsigned result = 0;
+
+	for (i = 0; i < 4; i++) {
+		if ((mask >> (i * 8)) & 0xFF) { /* i/o only if necessary */
+			u8 in_level = ts->read(ts->dev, 0x44 + i * 8);
+			u8 in_mask = 0;
+			u8 out_level = (ts->out_level >> (i * 8 + 4)) & 0xFF;
+			u8 out_mask = 0;
+
+			for (j = 0; j < 8; j++) {
+				int offset = 4 + i * 8 + j;
+				int config = (ts->port_config[offset >> 2] >>
+					      ((offset & 3) << 1)) &
+					PIN_CONFIG_MASK;
+
+				switch (config) {
+				case PIN_CONFIG_OUT:
+					out_mask |= BIT(j);
+					break;
+				case PIN_CONFIG_IN_WO_PULLUP:
+				case PIN_CONFIG_IN_PULLUP:
+					in_mask |= BIT(j);
+				}
+			}
+
+			result |= ((unsigned)(in_level & in_mask) |
+				   (out_level & out_mask)) << (i * 8);
+		}
+	}
+
+	return result & mask;
+}
+
 static void max7301_set(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct max7301 *ts = container_of(chip, struct max7301, chip);
@@ -160,6 +197,27 @@ static void max7301_set(struct gpio_chip
 	mutex_unlock(&ts->lock);
 }
 
+static
+void max7301_set_block(struct gpio_chip *chip, unsigned mask, unsigned values)
+{
+	struct max7301 *ts = container_of(chip, struct max7301, chip);
+	unsigned changes;
+	int i;
+
+	mutex_lock(&ts->lock);
+
+	changes = (ts->out_level ^ (values << 4)) & (mask << 4);
+	ts->out_level ^= changes;
+
+	for (i = 0; i < 4; i++) {
+		if ((changes >> (i * 8 + 4)) & 0xFF) /* i/o only on change */
+			ts->write(ts->dev, 0x44 + i * 8,
+				  (ts->out_level >> (i * 8 + 4)) & 0xFF);
+	}
+
+	mutex_unlock(&ts->lock);
+}
+
 int __devinit __max730x_probe(struct max7301 *ts)
 {
 	struct device *dev = ts->dev;
@@ -183,8 +241,10 @@ int __devinit __max730x_probe(struct max
 
 	ts->chip.direction_input = max7301_direction_input;
 	ts->chip.get = max7301_get;
+	ts->chip.get_block = max7301_get_block;
 	ts->chip.direction_output = max7301_direction_output;
 	ts->chip.set = max7301_set;
+	ts->chip.set_block = max7301_set_block;
 
 	ts->chip.base = pdata->base;
 	ts->chip.ngpio = PIN_NUMBER;

[PATCH RFC 5/6 v3] gpio-lpc32xx: Add block GPIO API

[Roland Stigge]

This patch adds block GPIO API support to the LPC32xx driver.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 drivers/gpio/gpio-lpc32xx.c |   78 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

--- linux-2.6.orig/drivers/gpio/gpio-lpc32xx.c
+++ linux-2.6/drivers/gpio/gpio-lpc32xx.c
@@ -297,6 +297,21 @@ static int lpc32xx_gpio_get_value_p3(str
 	return __get_gpio_state_p3(group, pin);
 }
 
+static unsigned lpc32xx_gpio_get_block_p3(struct gpio_chip *chip, unsigned mask)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 bits = __raw_readl(group->gpio_grp->inp_state);
+
+	/* In P3_INP_STATE, the 6 GPIOs are scattered over the register,
+	 * rearranging to bits 0-5
+	 */
+	bits >>= 10;
+	bits &= 0x401F;
+	bits |= (bits & 0x4000) >> 9;
+
+	return bits & mask & 0x3F;
+}
+
 static int lpc32xx_gpi_get_value(struct gpio_chip *chip, unsigned pin)
 {
 	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
@@ -304,6 +319,14 @@ static int lpc32xx_gpi_get_value(struct
 	return __get_gpi_state_p3(group, pin);
 }
 
+static unsigned lpc32xx_gpi_get_block(struct gpio_chip *chip, unsigned mask)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 bits = __raw_readl(group->gpio_grp->inp_state);
+
+	return bits & mask;
+}
+
 static int lpc32xx_gpio_dir_output_p012(struct gpio_chip *chip, unsigned pin,
 	int value)
 {
@@ -351,6 +374,26 @@ static void lpc32xx_gpio_set_value_p3(st
 	__set_gpio_level_p3(group, pin, value);
 }
 
+static void lpc32xx_gpio_set_block_p3(struct gpio_chip *chip, unsigned mask,
+				      unsigned values)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 set_bits = values & mask;
+	u32 clr_bits = ~values & mask;
+
+	/* States of GPIO 0-5 start at bit 25 */
+	set_bits <<= 25;
+	clr_bits <<= 25;
+
+	/* Note: On LPC32xx, GPOs can only be set at once or cleared at once,
+	 *       but not set and cleared@once
+	 */
+	if (set_bits)
+		__raw_writel(set_bits, group->gpio_grp->outp_set);
+	if (clr_bits)
+		__raw_writel(clr_bits, group->gpio_grp->outp_clr);
+}
+
 static void lpc32xx_gpo_set_value(struct gpio_chip *chip, unsigned pin,
 	int value)
 {
@@ -366,6 +409,30 @@ static int lpc32xx_gpo_get_value(struct
 	return __get_gpo_state_p3(group, pin);
 }
 
+static void lpc32xx_gpo_set_block(struct gpio_chip *chip, unsigned mask,
+				  unsigned values)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 set_bits = values & mask;
+	u32 clr_bits = ~values & mask;
+
+	/* Note: On LPC32xx, GPOs can only be set at once or cleared at once,
+	 *       but not set and cleared at once
+	 */
+	if (set_bits)
+		__raw_writel(set_bits, group->gpio_grp->outp_set);
+	if (clr_bits)
+		__raw_writel(clr_bits, group->gpio_grp->outp_clr);
+}
+
+static unsigned lpc32xx_gpo_get_block(struct gpio_chip *chip, unsigned mask)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 bits = __raw_readl(group->gpio_grp->outp_state);
+
+	return bits & mask;
+}
+
 static int lpc32xx_gpio_request(struct gpio_chip *chip, unsigned pin)
 {
 	if (pin < chip->ngpio)
@@ -440,8 +507,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p0",
 			.direction_input	= lpc32xx_gpio_dir_input_p012,
 			.get			= lpc32xx_gpio_get_value_p012,
+			.get_block		= lpc32xx_gpi_get_block,
 			.direction_output	= lpc32xx_gpio_dir_output_p012,
 			.set			= lpc32xx_gpio_set_value_p012,
+			.set_block		= lpc32xx_gpo_set_block,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_p01,
 			.base			= LPC32XX_GPIO_P0_GRP,
@@ -456,8 +525,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p1",
 			.direction_input	= lpc32xx_gpio_dir_input_p012,
 			.get			= lpc32xx_gpio_get_value_p012,
+			.get_block		= lpc32xx_gpi_get_block,
 			.direction_output	= lpc32xx_gpio_dir_output_p012,
 			.set			= lpc32xx_gpio_set_value_p012,
+			.set_block		= lpc32xx_gpo_set_block,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_p01,
 			.base			= LPC32XX_GPIO_P1_GRP,
@@ -472,8 +543,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p2",
 			.direction_input	= lpc32xx_gpio_dir_input_p012,
 			.get			= lpc32xx_gpio_get_value_p012,
+			.get_block		= lpc32xx_gpi_get_block,
 			.direction_output	= lpc32xx_gpio_dir_output_p012,
 			.set			= lpc32xx_gpio_set_value_p012,
+			.set_block		= lpc32xx_gpo_set_block,
 			.request		= lpc32xx_gpio_request,
 			.base			= LPC32XX_GPIO_P2_GRP,
 			.ngpio			= LPC32XX_GPIO_P2_MAX,
@@ -487,8 +560,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p3",
 			.direction_input	= lpc32xx_gpio_dir_input_p3,
 			.get			= lpc32xx_gpio_get_value_p3,
+			.get_block		= lpc32xx_gpio_get_block_p3,
 			.direction_output	= lpc32xx_gpio_dir_output_p3,
 			.set			= lpc32xx_gpio_set_value_p3,
+			.set_block		= lpc32xx_gpio_set_block_p3,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_gpio_p3,
 			.base			= LPC32XX_GPIO_P3_GRP,
@@ -503,6 +578,7 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpi_p3",
 			.direction_input	= lpc32xx_gpio_dir_in_always,
 			.get			= lpc32xx_gpi_get_value,
+			.get_block		= lpc32xx_gpi_get_block,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_gpi_p3,
 			.base			= LPC32XX_GPI_P3_GRP,
@@ -517,7 +593,9 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpo_p3",
 			.direction_output	= lpc32xx_gpio_dir_out_always,
 			.set			= lpc32xx_gpo_set_value,
+			.set_block		= lpc32xx_gpo_set_block,
 			.get			= lpc32xx_gpo_get_value,
+			.get_block		= lpc32xx_gpo_get_block,
 			.request		= lpc32xx_gpio_request,
 			.base			= LPC32XX_GPO_P3_GRP,
 			.ngpio			= LPC32XX_GPO_P3_MAX,

[PATCH RFC 6/6 v3] gpio-generic: Add block GPIO API

[Roland Stigge]

This patch adds block GPIO API support to the gpio-generic driver.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 drivers/gpio/gpio-generic.c |   56 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

--- linux-2.6.orig/drivers/gpio/gpio-generic.c
+++ linux-2.6/drivers/gpio/gpio-generic.c
@@ -122,6 +122,13 @@ static int bgpio_get(struct gpio_chip *g
 	return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
 }
 
+static unsigned bgpio_get_block(struct gpio_chip *gc, unsigned mask)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+	return bgc->read_reg(bgc->reg_dat) & mask;
+}
+
 static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
@@ -170,6 +177,51 @@ static void bgpio_set_set(struct gpio_ch
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
+static void bgpio_set_block(struct gpio_chip *gc, unsigned mask,
+			    unsigned values)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->data &= ~mask;
+	bgc->data |= values & mask;
+
+	bgc->write_reg(bgc->reg_dat, bgc->data);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static void bgpio_set_with_clear_block(struct gpio_chip *gc, unsigned mask,
+				       unsigned values)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned set_bits = values & mask;
+	unsigned clr_bits = ~values & mask;
+
+	if (set_bits)
+		bgc->write_reg(bgc->reg_set, set_bits);
+	if (clr_bits)
+		bgc->write_reg(bgc->reg_set, clr_bits);
+}
+
+static void bgpio_set_set_block(struct gpio_chip *gc, unsigned mask,
+				unsigned values)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->data &= ~mask;
+	bgc->data |= values & mask;
+
+	bgc->write_reg(bgc->reg_set, bgc->data);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
 static int bgpio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
 	return 0;
@@ -317,14 +369,18 @@ static int bgpio_setup_io(struct bgpio_c
 		bgc->reg_set = set;
 		bgc->reg_clr = clr;
 		bgc->gc.set = bgpio_set_with_clear;
+		bgc->gc.set_block = bgpio_set_with_clear_block;
 	} else if (set && !clr) {
 		bgc->reg_set = set;
 		bgc->gc.set = bgpio_set_set;
+		bgc->gc.set_block = bgpio_set_set_block;
 	} else {
 		bgc->gc.set = bgpio_set;
+		bgc->gc.set_block = bgpio_set_block;
 	}
 
 	bgc->gc.get = bgpio_get;
+	bgc->gc.get_block = bgpio_get_block;
 
 	return 0;
 }