On Sun, Oct 28, 2012 at 9:46 PM, Roland Stigge [off-list ref] wrote:

Just a quick few things I spotted:

+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?

Maybe there was some discussion I missed, anyway it deserves
a comment because this looks completely arbitrary.

+       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.

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

+       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.

+                       if (!tmp) {
+                               kfree(block->gbc);
+                               goto err2;
+                       }
+                       block->gbc = tmp;
+                       gbc = &block->gbc[block->nchip - 1];

So this becomes a list traversal and lookup instead.

+                       gbc->gc = gc;
+                       gbc->remap = NULL;
+                       gbc->nremap = 0;
+                       gbc->mask = 0;
+               } else {
+                       gbc = &block->gbc[index];

So find it in the list instead.

+               }
+               /* 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
+                *
+                * Note that even though in setting remap is given a negative
+                * index, the next lines guard that the potential out-of-bounds
+                * pointer is not dereferenced when out of bounds.
+                */

/*
 * 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.
 */

+               remap = &gbc->remap[gbc->nremap - 1];
+               if (!gbc->nremap || (bit - i != remap->offset)) {
+                       gbc->nremap++;
+                       tmp = krealloc(gbc->remap,
+                                             sizeof(struct gpio_remap) *
+                                             gbc->nremap, GFP_KERNEL);

I don't like this dynamic array either.
Basic pattern: wherever there is a krealloc, use a list.

If lists aren't efficient enough, use some other datastructure, rbtree or
whatever.

+                       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.

+unsigned long gpio_block_get(const struct gpio_block *block)
+{
+       struct gpio_block_chip *gbc;
+       int i, j;
+       unsigned long values = 0;
+
+       for (i = 0; i < block->nchip; i++) {
+               unsigned long remapped = 0;
+
+               gbc = &block->gbc[i];
+
+               if (gbc->gc->get_block) {
+                       remapped = gbc->gc->get_block(gbc->gc, gbc->mask);
+               } else {
+                       /* emulate */
+                       for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
+                               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 long values)
+{
+       struct gpio_block_chip *gbc;
+       int i, j;
+
+       for (i = 0; i < block->nchip; i++) {
+               unsigned long 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 */
+                       for_each_set_bit(j, &gbc->mask, BITS_PER_LONG)
+                               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)
+{
+       struct gpio_block *i;
+
+       list_for_each_entry(i, &gpio_block_list, list)
+               if (!strcmp(i->name, name))
+                       return i;

And here is a list anyway, I'm getting confused of this partitioning between
using dynamic arrays and lists. Just use a list please. This part looks
good.

Yours,
Linus Walleij