Re: [RFC 2.6.27 1/1] gpiolib: add support for batch set of pins

["Jaya Kumar" <jayakumar.lkml@gmail.com>]

On Mon, Dec 1, 2008 at 9:10 AM, Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
> On Mon, Dec 1, 2008 at 1:55 AM, David Brownell <david-b@pacbell.net> wrote:
>>
>> They wouldn't want names so easily confused with the current set
>> of GPIO calls, no.
>>
>
> Okay, how does gpio_set/get_values_batch() sound?
>
>>
>>> >
>>> > Then separately two more things:
>>> >
>>> >  - A gpio_* interface proposal for higher-level bitmask calls.
>>> >   This is worth some discussion; the calls should clearly
>>> >   be optional (not everything will implement them), and I
>>> >   can't help but suspect <linux/bitmap.h> interfaces should
>>> >   interoperate smoothly.
>>
>> ... including probably ganged request/free, and direction updates.
>> When bitbanging a multiplexed parallel databus, you'll often need
>> to change directions.
>
> Okay, so I'll also add gpio_direction_output/input_batch and request/free_batch.
>
>>
>>
>>> >
>>> >  - A gpiolib based implementation, using those new gpio_chip
>>> >   methods as accelerators if they're present.  This should
>>> >   probably also be optional, even at the Kconfig level; many
>>> >   systems won't need to spend memory on these calls.
>>>
>>> Understood. I will make it optional. Does
>>> CONFIG_GPIOLIB_MULTIBIT_ACCESS sound okay?
>>
>> Kind of verbose for my taste, and it's already "multibit" (one
>> at a time, but still multiple) ... let's see some more mergeable
>> proposals and code first.  Different C file too, I suspect.
>
> Ok, CONFIG_GPIOLIB_BATCH and I'll add this code in
> drivers/gpio/gpiolib_batch.c. I hope I have understood that suggestion
> correctly.
>
>>
>>
>>> > Don't assume gpiolib when defining the interface.
>>>
>>> Ok, I didn't understand this part. I think you mentioned a higher
>>> level interface before but I didn't fully understand, if not gpiolib,
>>> then at what level do you recommend?
>>
>> The gpio_*() interfaces are how system glue code and drivers
>> access GPIOs, unless they roll their own chip-specific calls.
>>
>> Whereas gpiolib (and gpio_chip) are an *optional* framework
>> for implementing those calls.  Platforms can (and do!) use
>> other frameworks ... maybe they don't want to pay its costs,
>> and don't need the various GPIO expander drivers.
>>
>> So to repeat:  don't assume the interface is implemented in
>> one particular way (using gpiolib).  It has to make sense
>> with other implementation strategies too.  Standard split
>> between interface and implementation, that's all.  (Some folk
>> have been heard to talk about "gpiolib" as the interface to
>> drivers ... it's not, it's a provider-only interface library.)
>>
>
> Okay, I read above several times and I read Documentation/gpio.txt.
> But... I'm not confident I've understood your meanings above in terms
> of how it should change the code. What I know so far is:
>
> a)
> arch/arm/mach-pxa/include/mach/gpio.h is where the pxa GPIO api
> interface is defined. So that's where I will put the
> gpio_get/set_values_batch functions. This will match the existing
> gpio_set/get_value code there.
>
> b)
> arch/arm/mach-pxa/gpio.c is where the implementation is.
> So I'll put the pxa_gpio_direction_input/output_batch and
> pxa_gpio_set/get_batch there.
>
> c)
> drivers/gpio/gpiolib_batch.c
>
> This is where I'd put the generic platform independent implementations
> of __gpio_set/get_values_batch
>
> Does this sound consistent with your recommendation? If not, I need
> more help to understand what changes you are recommending.
>
>
>> Doesn't necessarily need to be *you* doing that, but if it only
>> works on older gumstix boards it'd not be general enough.  Which
>> is part of why I say to get the lowest level proposal out there
>> first.  If done right, it'll be easy to support on other chips.
>
> Yes, I completely agree that it must work on a wide range of
> architectures. But I don't understand what you mean when you say get
> the lowest level proposal out there first. I see the RFC code that I
> posted as being the lowest level proposal (albeit needing better name
> and bitmask support) and one that is sufficiently general that it can
> be implemented on other architectures. If it can't, can you help me
> understand by pointing to which portion would break on other archs?
>

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

While we are here, I was thinking about it, and its better if I give
gpio_request/free/direction_batch a miss for now. 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.

Thanks,
jaya