[PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[H Hartley Sweeten]

gpiolib: introduce for_each_gpio_in_chip macro

There are a number of places in gpiolib where all the gpio's handled by a
chip are walked thru using a for() loop.  This introduces a for_each_*
macro to clarify the code.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>

---

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 51a8d41..b060f73 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -56,6 +56,11 @@ struct gpio_desc {
 };
 static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
 
+#define for_each_gpio_in_chip(__gpio, __chip)			\
+	for ((__gpio) = (__chip)->base;				\
+		(__gpio) < (__chip)->base + (__chip)->ngpio;	\
+		(__gpio)++)
+
 static inline void desc_set_label(struct gpio_desc *d, const char *label)
 {
 #ifdef CONFIG_DEBUG_FS
@@ -694,14 +699,14 @@ int gpiochip_add(struct gpio_chip *chip)
 	}
 
 	/* these GPIO numbers must not be managed by another gpio_chip */
-	for (id = base; id < base + chip->ngpio; id++) {
+	for_each_gpio_in_chip(id, chip) {
 		if (gpio_desc[id].chip != NULL) {
 			status = -EBUSY;
 			break;
 		}
 	}
 	if (status == 0) {
-		for (id = base; id < base + chip->ngpio; id++) {
+		for_each_gpio_in_chip(id, chip) {
 			gpio_desc[id].chip = chip;
 
 			/* REVISIT:  most hardware initializes GPIOs as
@@ -744,14 +749,14 @@ int gpiochip_remove(struct gpio_chip *chip)
 
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	for (id = chip->base; id < chip->base + chip->ngpio; id++) {
+	for_each_gpio_in_chip(id, chip) {
 		if (test_bit(FLAG_REQUESTED, &gpio_desc[id].flags)) {
 			status = -EBUSY;
 			break;
 		}
 	}
 	if (status == 0) {
-		for (id = chip->base; id < chip->base + chip->ngpio; id++)
+		for_each_gpio_in_chip(id, chip)
 			gpio_desc[id].chip = NULL;
 	}
 

Re: [PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[David Brownell]

On Friday 31 July 2009, H Hartley Sweeten wrote:
> gpiolib: introduce for_each_gpio_in_chip macro
> 
> There are a number of places in gpiolib where all the gpio's handled by a
> chip are walked thru using a for() loop.  This introduces a for_each_*
> macro to clarify the code.

I'd rather not.  There are four such loops, and these are
really simple iterators.  Such a macro IMO just obfuscates,
when it's just hiding such trivial index ops.

NAK.

> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> 
> ---
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 51a8d41..b060f73 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -56,6 +56,11 @@ struct gpio_desc {
>  };
>  static struct gpio_desc gpio_desc[ARCH_NR_GPIOS];
>  
> +#define for_each_gpio_in_chip(__gpio, __chip)			\
> +	for ((__gpio) = (__chip)->base;				\
> +		(__gpio) < (__chip)->base + (__chip)->ngpio;	\
> +		(__gpio)++)
> +
>  static inline void desc_set_label(struct gpio_desc *d, const char *label)
>  {
>  #ifdef CONFIG_DEBUG_FS
> @@ -694,14 +699,14 @@ int gpiochip_add(struct gpio_chip *chip)
>  	}
>  
>  	/* these GPIO numbers must not be managed by another gpio_chip */
> -	for (id = base; id < base + chip->ngpio; id++) {
> +	for_each_gpio_in_chip(id, chip) {
>  		if (gpio_desc[id].chip != NULL) {
>  			status = -EBUSY;
>  			break;
>  		}
>  	}
>  	if (status == 0) {
> -		for (id = base; id < base + chip->ngpio; id++) {
> +		for_each_gpio_in_chip(id, chip) {
>  			gpio_desc[id].chip = chip;
>  
>  			/* REVISIT:  most hardware initializes GPIOs as
> @@ -744,14 +749,14 @@ int gpiochip_remove(struct gpio_chip *chip)
>  
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
> -	for (id = chip->base; id < chip->base + chip->ngpio; id++) {
> +	for_each_gpio_in_chip(id, chip) {
>  		if (test_bit(FLAG_REQUESTED, &gpio_desc[id].flags)) {
>  			status = -EBUSY;
>  			break;
>  		}
>  	}
>  	if (status == 0) {
> -		for (id = chip->base; id < chip->base + chip->ngpio; id++)
> +		for_each_gpio_in_chip(id, chip)
>  			gpio_desc[id].chip = NULL;
>  	}
>  
> 
> 

RE: [PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[H Hartley Sweeten]

On Tuesday, August 04, 2009 2:14 PM, David Brownell wrote:
> On Friday 31 July 2009, H Hartley Sweeten wrote:
>> gpiolib: introduce for_each_gpio_in_chip macro
>> 
>> There are a number of places in gpiolib where all the gpio's handled by a
>> chip are walked thru using a for() loop.  This introduces a for_each_*
>> macro to clarify the code.
>
> I'd rather not.  There are four such loops, and these are
> really simple iterators.  Such a macro IMO just obfuscates,
> when it's just hiding such trivial index ops.
>
> NAK.

Ok.

For the record. The reason I sent this is I'm trying to work out an
extension to gpiolib that adds gpio_port_* access to the API.  Most
of the gpiolib drivers already the necessary logic since the raw I/O
is performed on the entire 'chip'.  The API just needs the extensions
added to request/free the port, set the direction and get/set the value.

Is this a worthwhile addition?

Regards,
Hartley

RE: [PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[Ben Nizette]

On Tue, 2009-08-04 at 20:48 -0400, H Hartley Sweeten wrote:

> For the record. The reason I sent this is I'm trying to work out an
> extension to gpiolib that adds gpio_port_* access to the API.  Most
> of the gpiolib drivers already the necessary logic since the raw I/O
> is performed on the entire 'chip'.  The API just needs the extensions
> added to request/free the port, set the direction and get/set the value.
> 
> Is this a worthwhile addition?

Plenty of people seem to think so.  Personally I haven't seen a great
use case except "'coz I can", but if you've got one I'd love to hear.

Have you seen http://lkml.org/lkml/2009/1/25/10 ?  Donno what ended up
happening to that patchset..

	--Ben.

Re: [PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[Jaya Kumar]

On Thu, Aug 6, 2009 at 11:31 AM, Ben Nizette <bn@niasdigital.com> wrote:
> On Tue, 2009-08-04 at 20:48 -0400, H Hartley Sweeten wrote:
>
>> For the record. The reason I sent this is I'm trying to work out an
>> extension to gpiolib that adds gpio_port_* access to the API.  Most
>> of the gpiolib drivers already the necessary logic since the raw I/O
>> is performed on the entire 'chip'.  The API just needs the extensions
>> added to request/free the port, set the direction and get/set the value.
>>
>> Is this a worthwhile addition?
>
> Plenty of people seem to think so.  Personally I haven't seen a great
> use case except "'coz I can", but if you've got one I'd love to hear.

Yes, you're right that there has been no major demand for it. There
are (luckily?) only a moderate number of devices that are using gpio
as their parallel bus interface. I've been supporting the batch-gpio
patchset below out-of-the-tree because it has come in handy with a few
e-paper display controllers and LCD 8080-IO that I've been developing
with.

>
> Have you seen http://lkml.org/lkml/2009/1/25/10 ?  Donno what ended up
> happening to that patchset..
>

I didn't pursue it further and have maintained it out-of-tree. I felt
that David had concerns about the API I implemented so it was unlikely
to get merged and I didn't have the motivation to implement another.
:-)

Thanks,
jaya

ps: I'm in Portland for the festival of linux conferences this week
and would be happy to work on this/discuss alternate APIs if it is of
interest.

RE: [PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[H Hartley Sweeten]

On Friday, September 18, 2009 5:03 PM, Jaya Kumar wrote:
> On Thu, Aug 6, 2009 at 11:31 AM, Ben Nizette <bn@niasdigital.com> wrote:
>> On Tue, 2009-08-04 at 20:48 -0400, H Hartley Sweeten wrote:
>>
>>> For the record. The reason I sent this is I'm trying to work out an
>>> extension to gpiolib that adds gpio_port_* access to the API.  Most
>>> of the gpiolib drivers already the necessary logic since the raw I/O
>>> is performed on the entire 'chip'.  The API just needs the extensions
>>> added to request/free the port, set the direction and get/set the value.
>>>
>>> Is this a worthwhile addition?
>>
>> Plenty of people seem to think so.  Personally I haven't seen a great
>> use case except "'coz I can", but if you've got one I'd love to hear.
>
> Yes, you're right that there has been no major demand for it. There
> are (luckily?) only a moderate number of devices that are using gpio
> as their parallel bus interface. I've been supporting the batch-gpio
> patchset below out-of-the-tree because it has come in handy with a few
> e-paper display controllers and LCD 8080-IO that I've been developing
> with.

With the abundant number of GPIO's available on many of the ARM chips it
seemed to me a "port" extension would be worthwhile.  It would allow a
side-band bus from the chip to access low-speed devices like a character
LCD as an example.

>>
>> Have you seen http://lkml.org/lkml/2009/1/25/10 ?  Donno what ended up
>> happening to that patchset..
>>
>
> I didn't pursue it further and have maintained it out-of-tree. I felt
> that David had concerns about the API I implemented so it was unlikely
> to get merged and I didn't have the motivation to implement another.
> :-)

Hmm.. That patchset is a lot different than what I was thinking of.  Your 
patch allows a variable width to the number of gpio's in the "port".  But
it also still gets/sets the "port" by individual bit accesses to the
gpio_chip.  By doing this I don't see how you could get a performance
increase.

The extension I was working on just allowed accessing the native width
of the gpio chip.  Most of the gpiolib drivers read/write the bits in a
native width, the individual gpio pin is masked in/out.  My patch just
allows access to the raw data without masking anything.

Take the .get method in pca953x.c driver as an example.

static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
{
	struct pca953x_chip *chip;
	uint16_t reg_val;
	int ret;

	chip = container_of(gc, struct pca953x_chip, gpio_chip);

	ret = pca953x_read_reg(chip, PCA953X_INPUT, &reg_val);
	if (ret < 0) {
		/* NOTE:  diagnostic already emitted; that's all we should
		 * do unless gpio_*_value_cansleep() calls become different
		 * from their nonsleeping siblings (and report faults).
		 */
		return 0;
	}

	return (reg_val & (1u << off)) ? 1 : 0;
}

The native width of the device is either 8 or 16 bits.  To get a gpio value
all of the bits are read then the desired gpio is masked out.

My thought was to just add the following methods to struct gpio_chip:

int			(*port_direction_input)(struct gpio_chip *chip);
unsigned int	(*port_get)(struct gpio_chip *chip);
int			(*port_direction_output)(struct gpio_chip *chip, unsigned int value);
void			(*port_set)(struct gpio_chip *chip,	unsigned int value);

I basically stopped working on this after Ben's comment and getting
no other feedback.  If this appears useful I can look at it again.

> Thanks,
> jaya
> 
> ps: I'm in Portland for the festival of linux conferences this week
> and would be happy to work on this/discuss alternate APIs if it is of
> interest.

Regards,
Hartley

Re: [PATCH] gpiolib: introduce for_each_gpio_in_chip macro

[Jaya Kumar]

On Sat, Sep 19, 2009 at 8:42 AM, H Hartley Sweeten
<hartleys@visionengravers.com> wrote:
> On Friday, September 18, 2009 5:03 PM, Jaya Kumar wrote:
>
> Hmm.. That patchset is a lot different than what I was thinking of.  Your
> patch allows a variable width to the number of gpio's in the "port".  But
> it also still gets/sets the "port" by individual bit accesses to the
> gpio_chip.  By doing this I don't see how you could get a performance
> increase.

It only does individual bit set/get if the specific arch does not
offer a multi bit set/get. See __gpio_set_batch_generic which calls
chip->set versus __gpio_set_batch which calls chip->set_batch so that
if the underlying architecture supports it then we do multi-bit
access. I provided one for pxa here:
http://marc.info/?l=linux-kernel&m=123287743104542&w=2 . So, the
following scenarios should all be optimized for:

a) if the user calls gpio_s/get_batch with a width that fits the
native register width and all those bits fit into the said register,
then it results in a call directly to the multibit register set/get
function/inline. For example, on pxa with above implementation,
gpio_set_batch(1, mask=0xFFFFFFF, value=0xc001f00d), at compile time,
that will result in a single GPSR/GPCR.
b) if the user calls gpio_s/get_batch with a width that more than one
register, the implementation walks the bits and splits it across the
registers as needed. This is done in __gpio_s/get_batch
c) if the user calls the gpio_s/get_batch with non-consecutive bits
using the 32-bit mask, then the implementation walks the bits and
splits it across the registers as needed.

So, it is fairly optimized. If I understood David's suggested design
correctly, it is one where the user does not need to worry about masks
or widths at all. Instead, you register with the library, a list of
bits you want to make a bus out of, and then the library is
responsible for taking that and optimizing that further as much as
possible. I agree that that would be a more elegant API. What I was
hoping for is that my implementation above could be a starting point
for that by first providing optimized multi-bit access that could be
used by the more elegant API.

Thanks,
jaya