Messages in this thread | | | Date | Wed, 31 Oct 2012 15:06:50 +0100 | Subject | Re: [PATCH RESEND 1/5 v6] gpio: Add a block GPIO API to gpiolib | From | Linus Walleij <> |
| |
On Sun, Oct 28, 2012 at 9:46 PM, Roland Stigge <stigge@antcom.de> 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
| |