Re: [RFC 2.6.27 1/1] gpiolib: add batch set/get

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

On Sun, Dec 28, 2008 at 10:38 PM, Eric Miao <eric.y.miao@gmail.com> wrote:
> On Sun, Dec 28, 2008 at 12:24 AM, Jaya Kumar <jayakumar.lkml@gmail.com> wrote:
>> +#ifdef CONFIG_GPIOLIB_BATCH
>> +       gpio_set_batch(DB0_GPIO_PIN, data, 0xFFFF, 16);
>> +#else
>>        for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++)
>>                gpio_set_value(DB0_GPIO_PIN + i, (data >> i) & 0x01);
>> +#endif
>
> Well, if AM300 selects GPIOLIB_BATCH, I don't think we need the
> gpio_set_value() stuffs, and get rid of this #ifdef completely.

Good point. Will do.

>> +       /* shift the bits into our register specific position */
>> +       values <<= offset;
>> +       bitmask <<= offset;
>> +
>> +       values &= bitmask;
>
> or a single 'values = (values & bitmask) << offset' ?

Yup, good point. Will do.

>
>> +       if (values)
>> +               __raw_writel(values, pxa->regbase + GPSR_OFFSET);
>> +
>> +       values = ~values;
>> +       values &= bitmask;
>
> ditto

Will do.

>
>> +       if (values)
>> +               __raw_writel(values, pxa->regbase + GPCR_OFFSET);
>> +}
>> +
>> +/*
>> + * Get output GPIO level in batches
>> + */
>> +static u32 pxa_gpio_get_batch(struct gpio_chip *chip, unsigned offset,
>> +                               u32 bitmask)
>> +{
>> +       u32 values;
>> +       struct pxa_gpio_chip *pxa;
>> +
>> +       /* we're guaranteed by the caller that offset + bitmask remains
>> +        * in this chip.
>> +        */
>> +       pxa = container_of(chip, struct pxa_gpio_chip, chip);
>> +
>> +       values = __raw_readl(pxa->regbase + GPLR_OFFSET);
>> +
>> +       /* shift the result back into original position */
>> +       values >>= offset;
>> +       /* no need to shift bitmask since we've already shifted values */
>> +       values &= bitmask;
>> +
>> +       return values;
>
> or a single 'return (values >> offset) & bitmask;' should be enough.

Agreed.

>
>> +}
>> +#endif
>> +
>> +#ifdef CONFIG_GPIOLIB_BATCH
>> +#define GPIO_CHIP(_n)                                                  \
>> +       [_n] = {                                                        \
>> +               .regbase = GPIO##_n##_BASE,                             \
>> +               .chip = {                                               \
>> +                       .label            = "gpio-" #_n,                \
>> +                       .direction_input  = pxa_gpio_direction_input,   \
>> +                       .direction_output = pxa_gpio_direction_output,  \
>> +                       .get              = pxa_gpio_get,               \
>> +                       .set              = pxa_gpio_set,               \
>> +                       .base             = (_n) * 32,                  \
>> +                       .ngpio            = 32,                         \
>> +                       .set_batch        = pxa_gpio_set_batch,         \
>> +                       .get_batch        = pxa_gpio_get_batch,         \
>
> This is a bit ugly, define pxa_gpio_set_batch to NULL #ifndef GPIOLIB_BATCH
> in the above code, and force .{set,get}_batch assignment anyway, this will
> look a bit better, the same way as PM. However, this requires a modification
> to gpio_chip to always allow these two pointers, which might be a concern.

I think I tried that but then encountered the problem that I can't put
ifdefs within the define GPIO_CHIP macro. Will try to find a different
way. How about if I do:
#ifdef GPIOLIB_BATCH
#define SET_BATCH_MACRO .set_batch = pxa_gpio_set_batch \
#else
#define SET_BATCH_MACRO
#endif

then leave SET_BATCH_MACRO in the GPIO_CHIP macro. I think that would work.

>> +               if (!chip->set_batch) {
>> +                       while (((gpio + i) < (chip->base + chip->ngpio))
>> +                               && bitwidth) {
>> +                               mask = 1 << i;
>> +                               value = values & mask;
>> +                               if (bitmask & mask)
>> +                                       chip->set(chip, gpio + i - chip->base,
>> +                                                       value);
>> +                               i++;
>> +                               bitwidth--;
>
> I recommend this being put into something like 'default_gpio_set_batch', and
> assign this to 'chip->set_batch' when the gpio chip is being registered and
> found 'chip->set_batch == NULL', so to keep this block consistent.
>
> Same comment to the 'get_batch' implementation below.

Ok, that should also make the code nicer, will do.

Thanks,
jaya