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

On Sun, Dec 28, 2008 at 10:38 PM, Eric Miao [off-list ref] wrote:

On Sun, Dec 28, 2008 at 12:24 AM, Jaya Kumar [off-list ref] wrote:

quoted

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

quoted

+       /* 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.

quoted

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

ditto

Will do.

quoted

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

quoted

+}
+#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.

quoted

+               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