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 <[email protected]>
---
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;
}
On Thu, Aug 6, 2009 at 11:31 AM, Ben Nizette <[email protected]> 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.
On Friday, September 18, 2009 5:03 PM, Jaya Kumar wrote:
> On Thu, Aug 6, 2009 at 11:31 AM, Ben Nizette <[email protected]> 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, ®_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
On Sat, Sep 19, 2009 at 8:42 AM, H Hartley Sweeten
<[email protected]> 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