Hi Linus,

thanks for your notes, comments below:

On 10/31/2012 03:06 PM, Linus Walleij wrote:

quoted

+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;
+       void *tmp;
+       int i;
+
+       if (size < 1 || size > sizeof(unsigned long) * 8)
+               return ERR_PTR(-EINVAL);

If the most GPIOs in a block is BITS_PER_LONG, why is the
latter clause not size > BITS_PER_LONG?

Good catch, thanks! :-)

quoted

+       for (i = 0; i < size; i++)
+               if (!gpio_is_valid(gpios[i]))
+                       return ERR_PTR(-EINVAL);
+
+       block = kzalloc(sizeof(struct gpio_block), GFP_KERNEL);
+       if (!block)
+               return ERR_PTR(-ENOMEM);

If these were instead glued to a struct device you could do
devm_kzalloc() here and have it garbage collected.

Good, will do.

This is why
https://blueprints.launchpad.net/linux-linaro/+spec/gpiochip-to-dev
needs to happen. (Sorry for hyperlinking.)

quoted

+       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++) {
+               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++;
+                       tmp = krealloc(block->gbc,
+                                      sizeof(struct gpio_block_chip) *
+                                      block->nchip, GFP_KERNEL);

Don't do this dynamic array business. Use a linked list instead.

OK, I checked the important spots where access to the two lists (gbc and
remap) happen (set and get functions), and the access is always
sequential, monotonic. So will use lists. Thanks.

[...]
/*
 * Maybe I'm a bit picky on comment style but I prefer
 * that the first line of a multi-line comment is blank.
 * Applies everywhere.
 */

As noted earlier in the discussion, current gpiolib.c's convention is
done first-line-not-blank. So I sticked to this (un)convention. But can
change this - you are not the first one pointing this out. :-)

quoted

+                       if (!tmp) {
+                               kfree(gbc->remap);
+                               goto err3;
+                       }
+                       gbc->remap = tmp;
+                       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);

So if we only had a real struct device  inside the gpiochip all
this boilerplate would go away, as devm_* take care of releasing
the resources.

Right. I guess you mean struct gpio_block here which should have a dev?
(Having gpiochip as a dev also would be best, though.) :-)

We need a separate dev for struct gpio_block, since those can be defined
dynamically (i.e. often) during the lifespan of gpio and gpiochip, so
garbage collection would be deferred for too long.

Thanks,

Roland