| Message ID | 1351457210-25222-2-git-send-email-stigge@antcom.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On Sun, Oct 28, 2012 at 9:46 PM, Roland Stigge <stigge@antcom.de> wrote: > The recurring task of providing simultaneous access to GPIO lines (especially > for bit banging protocols) needs an appropriate API. > > This patch adds a kernel internal "Block GPIO" API that enables simultaneous > access to several GPIOs. This is done by abstracting GPIOs to an n-bit word: > Once requested, it provides access to a group of GPIOs which can range over > multiple GPIO chips. > > Signed-off-by: Roland Stigge <stigge@antcom.de> Hey Roland, Linus and I just sat down and talked about your changes. I think I understand what you need to do, but I've got concerns about the approach. I'm already not a big fan of the sysfs gpio interface design*, so you can understand that I'm not keen to extend the interface further. At the very least, I want to be really careful about the form that the extension takes. First off, thank you for writing good documentation. That makes it a lot easier to understand how the series is intended to be used, and I really appreciate it. For the API, I don't think it is a good idea at all to try and abstract away gpios on multiple controllers. I understand that it makes life a lot easier for userspace to abstract those details away, but the problem is that it hides very important information about how the system is actually constructed that is important to actually get things to work. For example, say you have a gpio-connected device with the constraint that GPIOA must change either before or at the same time as GPIOB, but never after. If those GPIOs are on separate controllers, then the order is completely undefined, and the user has no way to control that other than to fall back to manipulating GPIOs one at a time again (and losing all the performance benefits). Either controller affinity needs to be explicit in the API, or the API needs to be constraint oriented (ie. a stream of commands and individual commands can be coalesced if they meet the constraints**). Also, the API requires remapping the GPIO numbers which forces the code to be a lot more complex than it needs to be. I would rather see new attribute(s) added to the gpiochip's directory to allow modifying all the pins on a given controller. It's considerably less complex, and I'm a lot happier about extending the sysfs ABI in that way than committing to the remapping block approach. Second, the API appears a little naive in the way it approaches changing values. It makes the assumption that every gpio in the block will be written at the same time, which doesn't take into account that even within a block it is highly likely that only a subset of the gpios need to be manipulated. A lot of GPIO controllers implement separate 'set' and 'clear' registers for exactly this reason. The API needs to allow users to choose a subset for manipulation. The ABI needs to either have separate 'set' and 'clear' operations, or operations need to have both mask and value arguments. Similarly, how do users manipulate pin direction with this ABI? *The big problem with the sysfs interface is that each operation is performed on a different file descriptor. While it is convenient for manipulating gpios from shell scripts, it doesn't provide any good mechanism to restrict gpios to a specific process or keep track of which gpios are "opened' by userspace. Ultimately what I think what is really needed is a proper character device interface that can keep track of multiple users and who can flip which bits, but that is slightly out of scope for this discussion. ** Actually, the command stream model is a very interesting idea. It is worth exploring. It would be a more natural ABI than the multiple files-and-directories model currently used. It would also eliminate the problems I have with the sysfs abi while still being usable from shell script. :-) g.
Hi Grant, thank you for your feedback! Notes below. On 10/31/2012 04:00 PM, Grant Likely wrote: > Linus and I just sat down and talked about your changes. I think I > understand what you need to do, but I've got concerns about the > approach. I'm already not a big fan of the sysfs gpio interface > design*, so you can understand that I'm not keen to extend the > interface further. At the very least, I want to be really careful > about the form that the extension takes. > > First off, thank you for writing good documentation. That makes it a > lot easier to understand how the series is intended to be used, and I > really appreciate it. > > For the API, I don't think it is a good idea at all to try and > abstract away gpios on multiple controllers. I understand that it > makes life a lot easier for userspace to abstract those details away, > but the problem is that it hides very important information about how > the system is actually constructed that is important to actually get > things to work. For example, say you have a gpio-connected device with > the constraint that GPIOA must change either before or at the same > time as GPIOB, but never after. If those GPIOs are on separate > controllers, then the order is completely undefined It is correct that it's not (yet) well documented and the API is also not very explicit about it, but the actual approach of the manipulation order is to let drivers handle gpios "as simultaneous as possible" and when not possible, do it in the _order of bits specified_ (either defined at the device tree level, or when created via block_gpio_create() directly). I consider it a good thing to abstract things away if possible here, if it is well documented what actually happens, which info should be available from the definition and the possibilities of the drivers and hardware actually used (the optional block gpio interface must be well implemented in the respective driver, and when combining multiple gpio controller chips, it should be clear that certain realtime timings on a resulting virtual n-bit bus are not possible.) > Second, the API appears a little naive in the way it approaches > changing values. It makes the assumption that every gpio in the block > will be written at the same time, which doesn't take into account that > even within a block it is highly likely that only a subset of the > gpios need to be manipulated. A lot of GPIO controllers implement > separate 'set' and 'clear' registers for exactly this reason. The API > needs to allow users to choose a subset for manipulation. The ABI > needs to either have separate 'set' and 'clear' operations, or > operations need to have both mask and value arguments. Similarly, how > do users manipulate pin direction with this ABI? I'm not sure how far you tested the API in depth: You can already define a block that maps onto a subset of gpios on a controller and internally of course maps onto those set and clear operations. Whenever you need to manipulate a different subset (whether disjoint or overlapping), you can easily define _additional_ blocks. From my experience, this solves most of the real world problems when n-bit busses are bit banged over GPIOs. Doesn't this already solve this (in a different way, though)? Pin direction currently needs to be set up separately, analogous to requesting gpios. Need to document this better, right. The assumption is that I/O needs to be efficient primarily, before bloating the API with direction functions. Or should I add functions for this? As long as there is no consensus about mainlining this API, I will maintain it further at git://git.antcom.de/linux-2.6 blockgpio because I need it in projects anyway. Will post updates that go in the direction that you proposed. Thanks! Roland
Hi Linus, thanks for your notes, comments below: On 10/31/2012 03:06 PM, Linus Walleij wrote: >> +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! :-) >> + 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.) > >> + 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. :-) >> + 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
Hi Roland On Wed, Oct 31, 2012 at 6:19 PM, Roland Stigge <stigge@antcom.de> wrote: > On 10/31/2012 04:00 PM, Grant Likely wrote: >> For the API, I don't think it is a good idea at all to try and >> abstract away gpios on multiple controllers. I understand that it >> makes life a lot easier for userspace to abstract those details away, >> but the problem is that it hides very important information about how >> the system is actually constructed that is important to actually get >> things to work. For example, say you have a gpio-connected device with >> the constraint that GPIOA must change either before or at the same >> time as GPIOB, but never after. If those GPIOs are on separate >> controllers, then the order is completely undefined > > It is correct that it's not (yet) well documented and the API is also > not very explicit about it, but the actual approach of the manipulation > order is to let drivers handle gpios "as simultaneous as possible" and > when not possible, do it in the _order of bits specified_ (either > defined at the device tree level, or when created via > block_gpio_create() directly). The documentation is actually fine. I do understand that the intent is "as simultaneous as possible", but I accept the point that the order of specification affects the behaviour*. However, it still remains that the method used by the ABI abstracts at the wrong level and that blocking arbitrary GPIO pins into a single virtual GPIO register is a bad idea. *note that the current code doesn't implement that intended behaviour either since the gpios are processed in the order of the controllers, not the order of the bits. > I'm not sure how far you tested the API in depth: You can already define > a block that maps onto a subset of gpios on a controller and internally > of course maps onto those set and clear operations. Whenever you need to > manipulate a different subset (whether disjoint or overlapping), you can > easily define _additional_ blocks. From my experience, this solves most > of the real world problems when n-bit busses are bit banged over GPIOs. > Doesn't this already solve this (in a different way, though)? Blech! Requiring a new block for each possible combination of write-at-once bits is a horrible ABI. That just strengthens my opinion that the abstraction isn't right yet. > Pin direction currently needs to be set up separately, analogous to > requesting gpios. Need to document this better, right. The assumption is > that I/O needs to be efficient primarily, before bloating the API with > direction functions. Or should I add functions for this? Since this is a userspace facing ABI, once it is merged it cannot be changed in an incompatible way. I cannot merge it until there is at least a plan for how to handle all of the reasonable use cases. That means it must support set/clear or mask operations. Also, if it sticks with the design of grouping pins from multiple controllers, then it needs to handle explicitly constraining what order operations are performed in at the time of the operation. At the time of setup doesn't work since constraints between pins may not always be in the same order. I really think you should consider implementing a command stream type of interface. g.
On Wed, Oct 31, 2012 at 04:00:17PM +0100, Grant Likely wrote: > For the API, I don't think it is a good idea at all to try and > abstract away gpios on multiple controllers. I understand that it > makes life a lot easier for userspace to abstract those details away, > but the problem is that it hides very important information about how > the system is actually constructed that is important to actually get > things to work. For example, say you have a gpio-connected device with > the constraint that GPIOA must change either before or at the same > time as GPIOB, but never after. If those GPIOs are on separate > controllers, then the order is completely undefined, and the user has > no way to control that other than to fall back to manipulating GPIOs > one at a time again (and losing all the performance benefits). Either > controller affinity needs to be explicit in the API, or the API needs > to be constraint oriented (ie. a stream of commands and individual > commands can be coalesced if they meet the constraints**). Also, the > API requires remapping the GPIO numbers which forces the code to be a > lot more complex than it needs to be. It feels like I'm missing something here but can we not simply say that if the user cares about the ordering of the signal changes within an update then they should be doing two separate updates? Most of the cases I'm aware of do things as an update with a strobe or clock that latches the values. The big advantage of grouping things together is that it means that we centralise the fallback code. > I would rather see new attribute(s) added to the gpiochip's directory > to allow modifying all the pins on a given controller. It's > considerably less complex, and I'm a lot happier about extending the > sysfs ABI in that way than committing to the remapping block approach. When I've looked at this stuff I've only looked at and thought about in kernel users. The gpiolib sysfs ABI is already fun enough :)
On 19:59 Wed 31 Oct , Grant Likely wrote: > Hi Roland > > On Wed, Oct 31, 2012 at 6:19 PM, Roland Stigge <stigge@antcom.de> wrote: > > On 10/31/2012 04:00 PM, Grant Likely wrote: > >> For the API, I don't think it is a good idea at all to try and > >> abstract away gpios on multiple controllers. I understand that it > >> makes life a lot easier for userspace to abstract those details away, > >> but the problem is that it hides very important information about how > >> the system is actually constructed that is important to actually get > >> things to work. For example, say you have a gpio-connected device with > >> the constraint that GPIOA must change either before or at the same > >> time as GPIOB, but never after. If those GPIOs are on separate > >> controllers, then the order is completely undefined > > > > It is correct that it's not (yet) well documented and the API is also > > not very explicit about it, but the actual approach of the manipulation > > order is to let drivers handle gpios "as simultaneous as possible" and > > when not possible, do it in the _order of bits specified_ (either > > defined at the device tree level, or when created via > > block_gpio_create() directly). > > The documentation is actually fine. I do understand that the intent is > "as simultaneous as possible", but I accept the point that the order > of specification affects the behaviour*. However, it still remains > that the method used by the ABI abstracts at the wrong level and that > blocking arbitrary GPIO pins into a single virtual GPIO register is a > bad idea. > > *note that the current code doesn't implement that intended behaviour > either since the gpios are processed in the order of the controllers, > not the order of the bits. > > > I'm not sure how far you tested the API in depth: You can already define > > a block that maps onto a subset of gpios on a controller and internally > > of course maps onto those set and clear operations. Whenever you need to > > manipulate a different subset (whether disjoint or overlapping), you can > > easily define _additional_ blocks. From my experience, this solves most > > of the real world problems when n-bit busses are bit banged over GPIOs. > > Doesn't this already solve this (in a different way, though)? > > Blech! Requiring a new block for each possible combination of > write-at-once bits is a horrible ABI. That just strengthens my opinion > that the abstraction isn't right yet. > > > Pin direction currently needs to be set up separately, analogous to > > requesting gpios. Need to document this better, right. The assumption is > > that I/O needs to be efficient primarily, before bloating the API with > > direction functions. Or should I add functions for this? > > Since this is a userspace facing ABI, once it is merged it cannot be > changed in an incompatible way. I cannot merge it until there is at > least a plan for how to handle all of the reasonable use cases. That > means it must support set/clear or mask operations. Also, if it sticks > with the design of grouping pins from multiple controllers, then it > needs to handle explicitly constraining what order operations are > performed in at the time of the operation. At the time of setup > doesn't work since constraints between pins may not always be in the > same order. > > I really think you should consider implementing a command stream type > of interface. I agreed with Grant and I'm not also a fan of the sysfs ABI as I already point out in the previous version and Linus too Best Regards, J. > > g.
On Wed, Oct 31, 2012 at 8:30 PM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Oct 31, 2012 at 04:00:17PM +0100, Grant Likely wrote: > >> For the API, I don't think it is a good idea at all to try and >> abstract away gpios on multiple controllers. I understand that it >> makes life a lot easier for userspace to abstract those details away, >> but the problem is that it hides very important information about how >> the system is actually constructed that is important to actually get >> things to work. For example, say you have a gpio-connected device with >> the constraint that GPIOA must change either before or at the same >> time as GPIOB, but never after. If those GPIOs are on separate >> controllers, then the order is completely undefined, and the user has >> no way to control that other than to fall back to manipulating GPIOs >> one at a time again (and losing all the performance benefits). Either >> controller affinity needs to be explicit in the API, or the API needs >> to be constraint oriented (ie. a stream of commands and individual >> commands can be coalesced if they meet the constraints**). Also, the >> API requires remapping the GPIO numbers which forces the code to be a >> lot more complex than it needs to be. > > It feels like I'm missing something here but can we not simply say that > if the user cares about the ordering of the signal changes within an > update then they should be doing two separate updates? Most of the > cases I'm aware of do things as an update with a strobe or clock that > latches the values. > > The big advantage of grouping things together is that it means that we > centralise the fallback code. The internal ABI is less of an issue because it is a whole lot easier to change compared to a userspace ABI (though I think we can do a lot better before deciding to merge it). Userspace also appears to be the intended usage, so I've focused my review on that use case. g.
On 10/31/2012 07:59 PM, Grant Likely wrote: >> Pin direction currently needs to be set up separately, analogous to >> requesting gpios. Need to document this better, right. The assumption is >> that I/O needs to be efficient primarily, before bloating the API with >> direction functions. Or should I add functions for this? > > Since this is a userspace facing ABI, once it is merged it cannot be > changed in an incompatible way. I cannot merge it until there is at > least a plan for how to handle all of the reasonable use cases. That > means it must support set/clear or mask operations. Also, if it sticks > with the design of grouping pins from multiple controllers, then it > needs to handle explicitly constraining what order operations are > performed in at the time of the operation. At the time of setup > doesn't work since constraints between pins may not always be in the > same order. > > I really think you should consider implementing a command stream type > of interface. Yes, understand. What do you consider a good example of a command stream type interface? Link or hint appreciated! There was already mentioned the idea of a device node which can be interfaced to via ioctl() for the various operations. Can it be a "struct miscdevice" or do you require sth. more sophisticated? Thanks in advance, Roland
--- linux-2.6.orig/Documentation/gpio.txt +++ linux-2.6/Documentation/gpio.txt @@ -439,6 +439,53 @@ slower clock delays the rising edge of S signaling rate accordingly. +Block GPIO +---------- + +The above described interface concentrates on handling single GPIOs. However, +in applications where it is critical to set several GPIOs at once, this +interface doesn't work well, e.g. bit-banging protocols via grouped GPIO lines. +Consider a GPIO controller that is connected via a slow I2C line. When +switching two or more GPIOs one after another, there can be considerable time +between those events. This is solved by an interface called Block GPIO: + +struct gpio_block *gpio_block_create(unsigned int *gpios, size_t size); + +This creates a new block of GPIOs as a list of GPIO numbers with the specified +size which are accessible via the returned struct gpio_block and the accessor +functions described below. Please note that you need to request the GPIOs +separately via gpio_request(). An arbitrary list of globally valid GPIOs can be +specified, even ranging over several gpio_chips. Actual handling of I/O +operations will be done on a best effort base, i.e. simultaneous I/O only where +possible by hardware and implemented in the respective GPIO driver. The number +of GPIOs in one block is limited to the number of bits in an unsigned long, or +BITS_PER_LONG, of the respective platform, i.e. typically at least 32 on a 32 +bit system, and at least 64 on a 64 bit system. However, several blocks can be +defined at once. + +unsigned gpio_block_get(struct gpio_block *block); +void gpio_block_set(struct gpio_block *block, unsigned value); + +With those accessor functions, setting and getting the GPIO values is possible, +analogous to gpio_get_value() and gpio_set_value(). Each bit in the return +value of gpio_block_get() and in the value argument of gpio_block_set() +corresponds to a bit specified on gpio_block_create(). Block operations in +hardware are only possible where the respective GPIO driver implements it, +falling back to using single GPIO operations where the driver only implements +single GPIO access. + +void gpio_block_free(struct gpio_block *block); + +After the GPIO block isn't used anymore, it should be free'd via +gpio_block_free(). + +int gpio_block_register(struct gpio_block *block); +void gpio_block_unregister(struct gpio_block *block); + +These functions can be used to register a GPIO block. Blocks registered this +way will be available via sysfs. + + What do these conventions omit? =============================== One of the biggest things these conventions omit is pin multiplexing, since --- linux-2.6.orig/drivers/gpio/gpiolib.c +++ linux-2.6/drivers/gpio/gpiolib.c @@ -83,6 +83,8 @@ static inline void desc_set_label(struct #endif } +static LIST_HEAD(gpio_block_list); + /* Warn when drivers omit gpio_request() calls -- legal but ill-advised * when setting direction, and otherwise illegal. Until board setup code * and drivers use explicit requests everywhere (which won't happen when @@ -1676,6 +1678,221 @@ void __gpio_set_value(unsigned gpio, int } EXPORT_SYMBOL_GPL(__gpio_set_value); +static int gpio_block_chip_index(struct gpio_block *block, struct gpio_chip *gc) +{ + int i; + + for (i = 0; i < block->nchip; i++) { + if (block->gbc[i].gc == gc) + return i; + } + return -1; +} + +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); + + 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); + + 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); + if (!tmp) { + kfree(block->gbc); + goto err2; + } + block->gbc = tmp; + gbc = &block->gbc[block->nchip - 1]; + gbc->gc = gc; + gbc->remap = NULL; + gbc->nremap = 0; + gbc->mask = 0; + } else { + gbc = &block->gbc[index]; + } + /* 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. + */ + 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); + 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); + +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; + return NULL; +} +EXPORT_SYMBOL_GPL(gpio_block_find_by_name); + +int gpio_block_register(struct gpio_block *block) +{ + if (gpio_block_find_by_name(block->name)) + return -EBUSY; + + list_add(&block->list, &gpio_block_list); + + return 0; +} +EXPORT_SYMBOL_GPL(gpio_block_register); + +void gpio_block_unregister(struct gpio_block *block) +{ + struct gpio_block *i; + + list_for_each_entry(i, &gpio_block_list, list) + if (i == block) { + list_del(&i->list); + break; + } +} +EXPORT_SYMBOL_GPL(gpio_block_unregister); + /** * __gpio_cansleep() - report whether gpio value access will sleep * @gpio: gpio in question --- linux-2.6.orig/include/asm-generic/gpio.h +++ linux-2.6/include/asm-generic/gpio.h @@ -43,6 +43,7 @@ static inline bool gpio_is_valid(int num struct device; struct gpio; +struct gpio_block; struct seq_file; struct module; struct device_node; @@ -105,6 +106,8 @@ struct gpio_chip { unsigned offset); int (*get)(struct gpio_chip *chip, unsigned offset); + unsigned long (*get_block)(struct gpio_chip *chip, + unsigned long mask); int (*direction_output)(struct gpio_chip *chip, unsigned offset, int value); int (*set_debounce)(struct gpio_chip *chip, @@ -112,6 +115,9 @@ struct gpio_chip { void (*set)(struct gpio_chip *chip, unsigned offset, int value); + void (*set_block)(struct gpio_chip *chip, + unsigned long mask, + unsigned long values); int (*to_irq)(struct gpio_chip *chip, unsigned offset); @@ -171,6 +177,15 @@ extern void gpio_set_value_cansleep(unsi extern int __gpio_get_value(unsigned gpio); extern void __gpio_set_value(unsigned gpio, int value); +extern struct gpio_block *gpio_block_create(unsigned *gpio, size_t size, + const char *name); +extern void gpio_block_free(struct gpio_block *block); +extern unsigned long gpio_block_get(const struct gpio_block *block); +extern void gpio_block_set(struct gpio_block *block, unsigned long values); +extern struct gpio_block *gpio_block_find_by_name(const char *name); +extern int gpio_block_register(struct gpio_block *block); +extern void gpio_block_unregister(struct gpio_block *block); + extern int __gpio_cansleep(unsigned gpio); extern int __gpio_to_irq(unsigned gpio); --- linux-2.6.orig/include/linux/gpio.h +++ linux-2.6/include/linux/gpio.h @@ -2,6 +2,8 @@ #define __LINUX_GPIO_H #include <linux/errno.h> +#include <linux/types.h> +#include <linux/list.h> /* see Documentation/gpio.txt */ @@ -39,6 +41,43 @@ struct gpio { const char *label; }; +/* + * struct gpio_remap - a structure for describing a bit mapping + * @mask: a bit mask + * @offset: how many bits to shift to the left (negative: to the right) + * + * When we are mapping bit values from one word to another (here: from GPIO + * block domain to GPIO driver domain) we first mask them out with mask and + * shift them as specified with offset. More complicated mappings are done by + * grouping several of those structs and adding the results together. + */ +struct gpio_remap { + unsigned long mask; + int offset; +}; + +struct gpio_block_chip { + struct gpio_chip *gc; + struct gpio_remap *remap; + int nremap; + unsigned long mask; +}; + +/** + * struct gpio_block - a structure describing a list of GPIOs for simultaneous + * operations + */ +struct gpio_block { + struct gpio_block_chip *gbc; + size_t nchip; + const char *name; + + int ngpio; + unsigned *gpio; + + struct list_head list; +}; + #ifdef CONFIG_GENERIC_GPIO #ifdef CONFIG_ARCH_HAVE_CUSTOM_GPIO_H @@ -169,6 +208,41 @@ static inline void gpio_set_value(unsign WARN_ON(1); } +static inline +struct gpio_block *gpio_block_create(unsigned *gpios, size_t size, + const char *name) +{ + WARN_ON(1); + return NULL; +} + +static inline void gpio_block_free(struct gpio_block *block) +{ + WARN_ON(1); +} + +static inline unsigned long gpio_block_get(const struct gpio_block *block) +{ + WARN_ON(1); + return 0; +} + +static inline void gpio_block_set(struct gpio_block *block, unsigned long value) +{ + WARN_ON(1); +} + +static inline int gpio_block_register(struct gpio_block *block) +{ + WARN_ON(1); + return 0; +} + +static inline void gpio_block_unregister(struct gpio_block *block) +{ + WARN_ON(1); +} + static inline int gpio_cansleep(unsigned gpio) { /* GPIO can never have been requested or set as {in,out}put */
The recurring task of providing simultaneous access to GPIO lines (especially for bit banging protocols) needs an appropriate API. This patch adds a kernel internal "Block GPIO" API that enables simultaneous access to several GPIOs. This is done by abstracting GPIOs to an n-bit word: Once requested, it provides access to a group of GPIOs which can range over multiple GPIO chips. Signed-off-by: Roland Stigge <stigge@antcom.de> --- Documentation/gpio.txt | 47 +++++++++ drivers/gpio/gpiolib.c | 217 +++++++++++++++++++++++++++++++++++++++++++++ include/asm-generic/gpio.h | 15 +++ include/linux/gpio.h | 74 +++++++++++++++ 4 files changed, 353 insertions(+)