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

From: Jaya Kumar
Date: Sun Dec 28 2008 - 23:12:20 EST


On Sun, Dec 28, 2008 at 10:38 PM, Eric Miao <eric.y.miao@xxxxxxxxx> wrote:
> On Sun, Dec 28, 2008 at 12:24 AM, Jaya Kumar <jayakumar.lkml@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/