On Saturday 27 December 2008, Jaya Kumar wrote:

Oh, gosh darn it, how time has flown. My email above was to make sure
I have understood the feedback. I assume I should just get started on
implementing. Just to double check, the plan is:
- add bitmask support.
- add get_batch support
- improve naming. I think gpio_get_batch/gpio_set_batch sounds good.

I was expecting to get some agreement on interfaces first.

I plan to have something like:

void gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth);
u32 gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth);

I still don't like the word "batch" here.  Did you look
at the <linux/bitmask.h> interfaces as I suggested?  They
would suggest something rather different:

 - passing bitmasks as {unsigned long *bits, int count} tuples
 - separate calls to:
     * zero a set of bits (loop: gpio_set_value to 0)
     * fill a set of bits (loop: gpio_set_value to 1)
     * copy a set of bits (loop: gpio_set_value to src[i])
     * read a set of bits (loop: dst[i] = gpio_get_value)
     * ... maybe more

Any restriction to 32 bit value seems shortsighted.  It would
make sense to wrap the GPIO bitmask descriptions in a struct,
letting drivers work with smaller sets -- probably most would
fit into a single "unsigned long".

I think I should explain the bitmask and bitwidth choice. The intended
use case is:
for (i=0; i < 800*600; i++) {
 gpio_set_batch(...)
}

bitwidth (needed to iterate and map to chip ngpios) could be
calculated from bitmask, but that involves iteratively counting bits
from the mask, so we would have to do 800*600 bit counts. Unless, we
do ugly things like cache the previous bitwidth/mask and compare
against the current caller arguments. Given that the bitwidth would
typically be a fixed value, I believe it is more intuitive for the
caller to provide it, eg:

gpio_set_batch(DB0, value, 0xFFFF, 16)

which has the nice performance benefit of skipping all the bit
counting in the most common use case scenario.

That doesn't explain the bit mask and bitwidth parameters at all.

While we are here, I was thinking about it, and its better if I give
gpio_request/free/direction_batch a miss for now.

Let's not and say we didn't.  Providing incomplete interfaces
isn't really much help.

Nothing prevents 
those features being added at a later point. That way I can focus on
getting gpio_set/get_batch implemented correctly and merged as the
first step.

First step needs to be defining the interface extensions needed.

- Dave