* [RFC 2.6.27 1/1] gpiolib: add batch set/get @ 2008-12-27 16:24 Jaya Kumar 2008-12-29 3:38 ` Eric Miao 0 siblings, 1 reply; 4+ messages in thread From: Jaya Kumar @ 2008-12-27 16:24 UTC (permalink / raw) Cc: Jaya Kumar, David Brownell, Eric Miao, Paulius Zaleckas, Geert Uytterhoeven, David Brownell, Sam Ravnborg, Haavard Skinnemoen, Philipp Zabel, Russell King, Ben Gardner, Greg KH, linux-arm-kernel, linux-fbdev-devel, linux-kernel Hi friends, This is v2 of my attempt to add batch support to gpiolib. Thanks to David Brownell, Eric Miao, Paulius Zaleckas, Geert, and Sam for prior feedback. I have tested set_batch using the am300 driver. get_batch is not tested. Please let me know your feedback. Thanks, jaya am300epd was doing 800*600*16*gpio_set_value for each framebuffer transfer (it uses 16-pins of gpio as its data bus). I found this caused a wee performance limitation. This patch adds an API for gpio_s/get_batch which allows drivers to s/get batches of gpio together in a single call. I have done a test implementation on gumstix (pxa255) with am300epd and it provides a nice improvement in performance. Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com> Cc: David Brownell <dbrownell@users.sourceforge.net> Cc: Eric Miao <eric.miao@marvell.com> Cc: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> Cc: Geert Uytterhoeven <geert@linux-m68k.org> Cc: David Brownell <david-b@pacbell.net> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Haavard Skinnemoen <hskinnemoen@atmel.com> Cc: Philipp Zabel <philipp.zabel@gmail.com> Cc: Russell King <rmk@arm.linux.org.uk> Cc: Ben Gardner <bgardner@wabtec.com> CC: Greg KH <greg@kroah.com> Cc: linux-arm-kernel@lists.arm.linux.org.uk Cc: linux-fbdev-devel@lists.sourceforge.net Cc: linux-kernel@vger.kernel.org --- arch/arm/mach-pxa/Kconfig | 1 + arch/arm/mach-pxa/am300epd.c | 9 ++- arch/arm/mach-pxa/gpio.c | 72 +++++++++++++++++++ arch/arm/mach-pxa/include/mach/gpio.h | 47 +++++++++++++ drivers/gpio/Kconfig | 5 ++ drivers/gpio/gpiolib.c | 122 +++++++++++++++++++++++++++++++++ include/asm-generic/gpio.h | 15 ++++- 7 files changed, 269 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig index 768618b..a5a306e 100644 --- a/arch/arm/mach-pxa/Kconfig +++ b/arch/arm/mach-pxa/Kconfig @@ -41,6 +41,7 @@ config GUMSTIX_AM200EPD bool "Enable AM200EPD board support" config GUMSTIX_AM300EPD + select GPIOLIB_BATCH bool "Enable AM300EPD board support" endchoice diff --git a/arch/arm/mach-pxa/am300epd.c b/arch/arm/mach-pxa/am300epd.c index 4bd10a1..9594cf0 100644 --- a/arch/arm/mach-pxa/am300epd.c +++ b/arch/arm/mach-pxa/am300epd.c @@ -190,9 +190,12 @@ static u16 am300_get_hdb(struct broadsheetfb_par *par) u16 res = 0; int i; +#ifdef CONFIG_GPIOLIB_BATCH + res = gpio_get_batch(DB0_GPIO_PIN, 0xFFFF, 16); +#else for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++) res |= (gpio_get_value(DB0_GPIO_PIN + i)) ? (1 << i) : 0; - +#endif return res; } @@ -200,8 +203,12 @@ static void am300_set_hdb(struct broadsheetfb_par *par, u16 data) { int i; +#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 } diff --git a/arch/arm/mach-pxa/gpio.c b/arch/arm/mach-pxa/gpio.c index 5fec1e4..d85cf2e 100644 --- a/arch/arm/mach-pxa/gpio.c +++ b/arch/arm/mach-pxa/gpio.c @@ -162,6 +162,76 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value) __raw_writel(mask, pxa->regbase + GPCR_OFFSET); } +#ifdef CONFIG_GPIOLIB_BATCH +/* + * Set output GPIO level in batches + */ +static void pxa_gpio_set_batch(struct gpio_chip *chip, unsigned offset, + u32 values, u32 bitmask) +{ + 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); + + /* shift the bits into our register specific position */ + values <<= offset; + bitmask <<= offset; + + values &= bitmask; + if (values) + __raw_writel(values, pxa->regbase + GPSR_OFFSET); + + values = ~values; + values &= bitmask; + 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; +} +#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, \ + }, \ + } +#else #define GPIO_CHIP(_n) \ [_n] = { \ .regbase = GPIO##_n##_BASE, \ @@ -175,6 +245,8 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value) .ngpio = 32, \ }, \ } +#endif + static struct pxa_gpio_chip pxa_gpio_chip[] = { GPIO_CHIP(0), diff --git a/arch/arm/mach-pxa/include/mach/gpio.h b/arch/arm/mach-pxa/include/mach/gpio.h index 2c538d8..e66e752 100644 --- a/arch/arm/mach-pxa/include/mach/gpio.h +++ b/arch/arm/mach-pxa/include/mach/gpio.h @@ -56,6 +56,53 @@ static inline void gpio_set_value(unsigned gpio, int value) } } +#ifdef CONFIG_GPIOLIB_BATCH +static inline void gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, + int bitwidth) +{ + if (__builtin_constant_p(gpio) && + (gpio + bitwidth < NR_BUILTIN_GPIO) && + ((gpio + bitwidth) <= roundup(gpio+1, 32))) { + int shift; + + shift = gpio % 32; + values <<= shift; + bitmask <<= shift; + + values &= bitmask; + if (values) + GPSR(gpio) = values; + + values = ~values; + values &= bitmask; + if (values) + GPCR(gpio) = values; + } else { + __gpio_set_batch(gpio, values, bitmask, bitwidth); + } +} + +static inline u32 gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth) +{ + if (__builtin_constant_p(gpio) && + (gpio + bitwidth < NR_BUILTIN_GPIO) && + ((gpio + bitwidth) <= roundup(gpio+1, 32))) { + int shift; + u32 values; + + shift = gpio % 32; + + values = GPLR(gpio); + values >>= shift; + values &= bitmask; + + return values; + } else { + return __gpio_get_batch(gpio, bitmask, bitwidth); + } +} +#endif + #define gpio_cansleep __gpio_cansleep #define gpio_to_irq(gpio) IRQ_GPIO(gpio) diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 48f49d9..aeeb83c 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -37,6 +37,11 @@ menuconfig GPIOLIB if GPIOLIB +config GPIOLIB_BATCH + bool "Batch GPIO support" + help + Say Y here to add the capability to batch set/get GPIOs. + config DEBUG_GPIO bool "Debug GPIO calls" depends on DEBUG_KERNEL diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 82020ab..7b36573 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1056,6 +1056,128 @@ void __gpio_set_value(unsigned gpio, int value) } EXPORT_SYMBOL_GPL(__gpio_set_value); +#ifdef CONFIG_GPIOLIB_BATCH +/** + * __gpio_set_batch() - assign a batch of gpio pins together + * @gpio: starting gpio pin + * @values: values to assign, sequential and including masked bits + * @bitmask: bitmask to be applied to values + * @bitwidth: width inclusive of masked-off bits + * Context: any + * + * This is used directly or indirectly to implement gpio_set_value(). + * It invokes the associated gpio_chip.set_batch() method. If that + * method does not exist for any segment that is involved, then it drops + * back down to standard gpio_chip.set() + */ +void __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth) +{ + struct gpio_chip *chip; + int i = 0; + int value, width, remwidth; + u32 mask; + + do { + chip = gpio_to_chip(gpio + i); + WARN_ON(extra_checks && chip->can_sleep); + + 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--; + } + } else { + value = values >> i; /* shift off the used data bits */ + remwidth = ((chip->base + (int) chip->ngpio) - + ((int) gpio + i)); + width = min(bitwidth, remwidth); + + /* shift off the used mask bits */ + mask = bitmask >> i; + /* now adjust mask by width of set */ + mask &= ((1 << width) - 1); + + chip->set_batch(chip, gpio + i - chip->base, value, + mask); + i += width; + bitwidth -= width; + } + } while (bitwidth); +} +EXPORT_SYMBOL_GPL(__gpio_set_batch); + +/** + * __gpio_get_batch() - get a batch of gpio pins together + * @gpio: starting gpio pin + * @bitmask: bitmask to be applied to values + * @bitwidth: width inclusive of masked-off bits + * Context: any + * + * This is used directly or indirectly to implement gpio_get_value(). + * It invokes the associated gpio_chip.get_batch() method and returns + * zero if no such method is provided. + */ +u32 __gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth) +{ + struct gpio_chip *chip; + int i = 0; + int width, remwidth; + u32 mask; + u32 values = 0; + u32 value; + + do { + chip = gpio_to_chip(gpio + i); + WARN_ON(extra_checks && chip->can_sleep); + + /* + * check if we have batch capability on this chip. + * if not, then we use the single bit gets + * any masked or inaccessible bits are already 0 + */ + if (!chip->get_batch) { + while (((gpio + i) < (chip->base + chip->ngpio)) + && bitwidth) { + mask = 1 << i; + /* skip if no get or masked out */ + if (chip->get && (bitmask & mask)) { + if (chip->get(chip, + gpio + i - chip->base)) + values |= mask; + } + i++; + bitwidth--; + } + } else { + remwidth = ((chip->base + (int) chip->ngpio) - + ((int) gpio + i)); + width = min(bitwidth, remwidth); + + /* shift off the used mask bits */ + mask = bitmask >> i; + /* now adjust mask by width of get */ + mask &= ((1 << width) - 1); + + value = chip->get_batch(chip, gpio + i - chip->base, + mask); + /* shift result back into caller position */ + values |= value << i; + i += width; + bitwidth -= width; + } + } while (bitwidth); + + return values; +} +EXPORT_SYMBOL_GPL(__gpio_get_batch); +#endif + /** * __gpio_cansleep() - report whether gpio value access will sleep * @gpio: gpio in question diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h index 81797ec..fa716a7 100644 --- a/include/asm-generic/gpio.h +++ b/include/asm-generic/gpio.h @@ -44,6 +44,8 @@ struct module; * returns either the value actually sensed, or zero * @direction_output: configures signal "offset" as output, or returns error * @set: assigns output value for signal "offset" + * @set_batch: batch assigns output values for consecutive signals starting at + * "offset" with mask in "bitmask" all within this gpio_chip * @to_irq: optional hook supporting non-static gpio_to_irq() mappings; * implementation may not sleep * @dbg_show: optional routine to show contents in debugfs; default code @@ -84,7 +86,13 @@ struct gpio_chip { unsigned offset, int value); void (*set)(struct gpio_chip *chip, unsigned offset, int value); - +#ifdef CONFIG_GPIOLIB_BATCH + void (*set_batch)(struct gpio_chip *chip, + unsigned offset, u32 values, + u32 bitmask); + u32 (*get_batch)(struct gpio_chip *chip, + unsigned offset, u32 bitmask); +#endif int (*to_irq)(struct gpio_chip *chip, unsigned offset); @@ -124,6 +132,11 @@ extern void gpio_set_value_cansleep(unsigned gpio, int value); */ extern int __gpio_get_value(unsigned gpio); extern void __gpio_set_value(unsigned gpio, int value); +#ifdef CONFIG_GPIOLIB_BATCH +extern void __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, + int bitwidth); +extern u32 __gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth); +#endif extern int __gpio_cansleep(unsigned gpio); -- 1.5.2.3 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC 2.6.27 1/1] gpiolib: add batch set/get 2008-12-27 16:24 [RFC 2.6.27 1/1] gpiolib: add batch set/get Jaya Kumar @ 2008-12-29 3:38 ` Eric Miao 2008-12-29 4:12 ` Jaya Kumar 2008-12-29 20:33 ` David Brownell 0 siblings, 2 replies; 4+ messages in thread From: Eric Miao @ 2008-12-29 3:38 UTC (permalink / raw) To: Jaya Kumar Cc: David Brownell, Eric Miao, Paulius Zaleckas, Geert Uytterhoeven, David Brownell, Sam Ravnborg, Haavard Skinnemoen, Philipp Zabel, Russell King, Ben Gardner, Greg KH, linux-arm-kernel, linux-fbdev-devel, linux-kernel On Sun, Dec 28, 2008 at 12:24 AM, Jaya Kumar <jayakumar.lkml@gmail.com> wrote: > Hi friends, > > This is v2 of my attempt to add batch support to gpiolib. Thanks to > David Brownell, Eric Miao, Paulius Zaleckas, Geert, and Sam for prior > feedback. I have tested set_batch using the am300 driver. get_batch > is not tested. > > Please let me know your feedback. > > Thanks, > jaya > > am300epd was doing 800*600*16*gpio_set_value for each framebuffer transfer > (it uses 16-pins of gpio as its data bus). I found this caused a wee > performance limitation. This patch adds an API for gpio_s/get_batch > which allows drivers to s/get batches of gpio together in a single call. > I have done a test implementation on gumstix (pxa255) with am300epd > and it provides a nice improvement in performance. > > Signed-off-by: Jaya Kumar <jayakumar.lkml@gmail.com> > Cc: David Brownell <dbrownell@users.sourceforge.net> > Cc: Eric Miao <eric.miao@marvell.com> > Cc: Paulius Zaleckas <paulius.zaleckas@teltonika.lt> > Cc: Geert Uytterhoeven <geert@linux-m68k.org> > Cc: David Brownell <david-b@pacbell.net> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Haavard Skinnemoen <hskinnemoen@atmel.com> > Cc: Philipp Zabel <philipp.zabel@gmail.com> > Cc: Russell King <rmk@arm.linux.org.uk> > Cc: Ben Gardner <bgardner@wabtec.com> > CC: Greg KH <greg@kroah.com> > Cc: linux-arm-kernel@lists.arm.linux.org.uk > Cc: linux-fbdev-devel@lists.sourceforge.net > Cc: linux-kernel@vger.kernel.org > --- > arch/arm/mach-pxa/Kconfig | 1 + > arch/arm/mach-pxa/am300epd.c | 9 ++- > arch/arm/mach-pxa/gpio.c | 72 +++++++++++++++++++ > arch/arm/mach-pxa/include/mach/gpio.h | 47 +++++++++++++ > drivers/gpio/Kconfig | 5 ++ > drivers/gpio/gpiolib.c | 122 +++++++++++++++++++++++++++++++++ > include/asm-generic/gpio.h | 15 ++++- > 7 files changed, 269 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-pxa/Kconfig b/arch/arm/mach-pxa/Kconfig > index 768618b..a5a306e 100644 > --- a/arch/arm/mach-pxa/Kconfig > +++ b/arch/arm/mach-pxa/Kconfig > @@ -41,6 +41,7 @@ config GUMSTIX_AM200EPD > bool "Enable AM200EPD board support" > > config GUMSTIX_AM300EPD > + select GPIOLIB_BATCH > bool "Enable AM300EPD board support" > > endchoice > diff --git a/arch/arm/mach-pxa/am300epd.c b/arch/arm/mach-pxa/am300epd.c > index 4bd10a1..9594cf0 100644 > --- a/arch/arm/mach-pxa/am300epd.c > +++ b/arch/arm/mach-pxa/am300epd.c > @@ -190,9 +190,12 @@ static u16 am300_get_hdb(struct broadsheetfb_par *par) > u16 res = 0; > int i; > > +#ifdef CONFIG_GPIOLIB_BATCH > + res = gpio_get_batch(DB0_GPIO_PIN, 0xFFFF, 16); > +#else > for (i = 0; i <= (DB15_GPIO_PIN - DB0_GPIO_PIN) ; i++) > res |= (gpio_get_value(DB0_GPIO_PIN + i)) ? (1 << i) : 0; > - > +#endif > return res; > } > > @@ -200,8 +203,12 @@ static void am300_set_hdb(struct broadsheetfb_par *par, u16 data) > { > int i; > > +#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. > } > > > diff --git a/arch/arm/mach-pxa/gpio.c b/arch/arm/mach-pxa/gpio.c > index 5fec1e4..d85cf2e 100644 > --- a/arch/arm/mach-pxa/gpio.c > +++ b/arch/arm/mach-pxa/gpio.c > @@ -162,6 +162,76 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > __raw_writel(mask, pxa->regbase + GPCR_OFFSET); > } > > +#ifdef CONFIG_GPIOLIB_BATCH > +/* > + * Set output GPIO level in batches > + */ > +static void pxa_gpio_set_batch(struct gpio_chip *chip, unsigned offset, > + u32 values, u32 bitmask) > +{ > + 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); > + > + /* shift the bits into our register specific position */ > + values <<= offset; > + bitmask <<= offset; > + > + values &= bitmask; or a single 'values = (values & bitmask) << offset' ? > + if (values) > + __raw_writel(values, pxa->regbase + GPSR_OFFSET); > + > + values = ~values; > + values &= bitmask; ditto > + 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. > +} > +#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. > + }, \ > + } > +#else > #define GPIO_CHIP(_n) \ > [_n] = { \ > .regbase = GPIO##_n##_BASE, \ > @@ -175,6 +245,8 @@ static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > .ngpio = 32, \ > }, \ > } > +#endif > + > > static struct pxa_gpio_chip pxa_gpio_chip[] = { > GPIO_CHIP(0), > diff --git a/arch/arm/mach-pxa/include/mach/gpio.h b/arch/arm/mach-pxa/include/mach/gpio.h > index 2c538d8..e66e752 100644 > --- a/arch/arm/mach-pxa/include/mach/gpio.h > +++ b/arch/arm/mach-pxa/include/mach/gpio.h > @@ -56,6 +56,53 @@ static inline void gpio_set_value(unsigned gpio, int value) > } > } > > +#ifdef CONFIG_GPIOLIB_BATCH > +static inline void gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, > + int bitwidth) > +{ > + if (__builtin_constant_p(gpio) && > + (gpio + bitwidth < NR_BUILTIN_GPIO) && > + ((gpio + bitwidth) <= roundup(gpio+1, 32))) { > + int shift; > + > + shift = gpio % 32; > + values <<= shift; > + bitmask <<= shift; > + > + values &= bitmask; > + if (values) > + GPSR(gpio) = values; > + > + values = ~values; > + values &= bitmask; > + if (values) > + GPCR(gpio) = values; I doubt the optimization of GPIO being a constant is essential for batch operation ... anyway, this is plus. > + } else { > + __gpio_set_batch(gpio, values, bitmask, bitwidth); > + } > +} > + > +static inline u32 gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth) > +{ > + if (__builtin_constant_p(gpio) && > + (gpio + bitwidth < NR_BUILTIN_GPIO) && > + ((gpio + bitwidth) <= roundup(gpio+1, 32))) { > + int shift; > + u32 values; > + > + shift = gpio % 32; > + > + values = GPLR(gpio); > + values >>= shift; > + values &= bitmask; > + > + return values; > + } else { > + return __gpio_get_batch(gpio, bitmask, bitwidth); > + } > +} > +#endif > + > #define gpio_cansleep __gpio_cansleep > > #define gpio_to_irq(gpio) IRQ_GPIO(gpio) > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 48f49d9..aeeb83c 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -37,6 +37,11 @@ menuconfig GPIOLIB > > if GPIOLIB > > +config GPIOLIB_BATCH > + bool "Batch GPIO support" > + help > + Say Y here to add the capability to batch set/get GPIOs. > + > config DEBUG_GPIO > bool "Debug GPIO calls" > depends on DEBUG_KERNEL > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 82020ab..7b36573 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -1056,6 +1056,128 @@ void __gpio_set_value(unsigned gpio, int value) > } > EXPORT_SYMBOL_GPL(__gpio_set_value); > > +#ifdef CONFIG_GPIOLIB_BATCH > +/** > + * __gpio_set_batch() - assign a batch of gpio pins together > + * @gpio: starting gpio pin > + * @values: values to assign, sequential and including masked bits > + * @bitmask: bitmask to be applied to values > + * @bitwidth: width inclusive of masked-off bits > + * Context: any > + * > + * This is used directly or indirectly to implement gpio_set_value(). > + * It invokes the associated gpio_chip.set_batch() method. If that > + * method does not exist for any segment that is involved, then it drops > + * back down to standard gpio_chip.set() > + */ > +void __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth) > +{ > + struct gpio_chip *chip; > + int i = 0; > + int value, width, remwidth; > + u32 mask; > + > + do { > + chip = gpio_to_chip(gpio + i); > + WARN_ON(extra_checks && chip->can_sleep); > + > + 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. > + } > + } else { > + value = values >> i; /* shift off the used data bits */ > + remwidth = ((chip->base + (int) chip->ngpio) - > + ((int) gpio + i)); > + width = min(bitwidth, remwidth); > + > + /* shift off the used mask bits */ > + mask = bitmask >> i; > + /* now adjust mask by width of set */ > + mask &= ((1 << width) - 1); > + > + chip->set_batch(chip, gpio + i - chip->base, value, > + mask); > + i += width; > + bitwidth -= width; > + } > + } while (bitwidth); > +} > +EXPORT_SYMBOL_GPL(__gpio_set_batch); > + > +/** > + * __gpio_get_batch() - get a batch of gpio pins together > + * @gpio: starting gpio pin > + * @bitmask: bitmask to be applied to values > + * @bitwidth: width inclusive of masked-off bits > + * Context: any > + * > + * This is used directly or indirectly to implement gpio_get_value(). > + * It invokes the associated gpio_chip.get_batch() method and returns > + * zero if no such method is provided. > + */ > +u32 __gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth) > +{ > + struct gpio_chip *chip; > + int i = 0; > + int width, remwidth; > + u32 mask; > + u32 values = 0; > + u32 value; > + > + do { > + chip = gpio_to_chip(gpio + i); > + WARN_ON(extra_checks && chip->can_sleep); > + > + /* > + * check if we have batch capability on this chip. > + * if not, then we use the single bit gets > + * any masked or inaccessible bits are already 0 > + */ > + if (!chip->get_batch) { > + while (((gpio + i) < (chip->base + chip->ngpio)) > + && bitwidth) { > + mask = 1 << i; > + /* skip if no get or masked out */ > + if (chip->get && (bitmask & mask)) { > + if (chip->get(chip, > + gpio + i - chip->base)) > + values |= mask; > + } > + i++; > + bitwidth--; > + } > + } else { > + remwidth = ((chip->base + (int) chip->ngpio) - > + ((int) gpio + i)); > + width = min(bitwidth, remwidth); > + > + /* shift off the used mask bits */ > + mask = bitmask >> i; > + /* now adjust mask by width of get */ > + mask &= ((1 << width) - 1); > + > + value = chip->get_batch(chip, gpio + i - chip->base, > + mask); > + /* shift result back into caller position */ > + values |= value << i; > + i += width; > + bitwidth -= width; > + } > + } while (bitwidth); > + > + return values; > +} > +EXPORT_SYMBOL_GPL(__gpio_get_batch); > +#endif > + > /** > * __gpio_cansleep() - report whether gpio value access will sleep > * @gpio: gpio in question > diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h > index 81797ec..fa716a7 100644 > --- a/include/asm-generic/gpio.h > +++ b/include/asm-generic/gpio.h > @@ -44,6 +44,8 @@ struct module; > * returns either the value actually sensed, or zero > * @direction_output: configures signal "offset" as output, or returns error > * @set: assigns output value for signal "offset" > + * @set_batch: batch assigns output values for consecutive signals starting at > + * "offset" with mask in "bitmask" all within this gpio_chip > * @to_irq: optional hook supporting non-static gpio_to_irq() mappings; > * implementation may not sleep > * @dbg_show: optional routine to show contents in debugfs; default code > @@ -84,7 +86,13 @@ struct gpio_chip { > unsigned offset, int value); > void (*set)(struct gpio_chip *chip, > unsigned offset, int value); > - > +#ifdef CONFIG_GPIOLIB_BATCH > + void (*set_batch)(struct gpio_chip *chip, > + unsigned offset, u32 values, > + u32 bitmask); > + u32 (*get_batch)(struct gpio_chip *chip, > + unsigned offset, u32 bitmask); > +#endif > int (*to_irq)(struct gpio_chip *chip, > unsigned offset); > > @@ -124,6 +132,11 @@ extern void gpio_set_value_cansleep(unsigned gpio, int value); > */ > extern int __gpio_get_value(unsigned gpio); > extern void __gpio_set_value(unsigned gpio, int value); > +#ifdef CONFIG_GPIOLIB_BATCH > +extern void __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, > + int bitwidth); > +extern u32 __gpio_get_batch(unsigned gpio, u32 bitmask, int bitwidth); > +#endif > > extern int __gpio_cansleep(unsigned gpio); > > -- > 1.5.2.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- Cheers - eric ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC 2.6.27 1/1] gpiolib: add batch set/get 2008-12-29 3:38 ` Eric Miao @ 2008-12-29 4:12 ` Jaya Kumar 2008-12-29 20:33 ` David Brownell 1 sibling, 0 replies; 4+ messages in thread From: Jaya Kumar @ 2008-12-29 4:12 UTC (permalink / raw) To: Eric Miao Cc: David Brownell, Eric Miao, Paulius Zaleckas, Geert Uytterhoeven, David Brownell, Sam Ravnborg, Haavard Skinnemoen, Philipp Zabel, Russell King, Ben Gardner, Greg KH, linux-arm-kernel, linux-fbdev-devel, linux-kernel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC 2.6.27 1/1] gpiolib: add batch set/get 2008-12-29 3:38 ` Eric Miao 2008-12-29 4:12 ` Jaya Kumar @ 2008-12-29 20:33 ` David Brownell 1 sibling, 0 replies; 4+ messages in thread From: David Brownell @ 2008-12-29 20:33 UTC (permalink / raw) To: Eric Miao, Jaya Kumar Cc: linux-fbdev-devel, Haavard Skinnemoen, Greg KH, linux-kernel, Ben Gardner, Paulius Zaleckas, Geert Uytterhoeven, Philipp Zabel, Eric Miao, Sam Ravnborg, linux-arm-kernel, Russell King I'm a bit surprised to see patches against 2.6.27, rather than a 2.6.28 (or 2.6.28-rc) kernel. ;) On Sunday 28 December 2008, Eric Miao wrote: > > @@ -200,8 +203,12 @@ static void am300_set_hdb(struct broadsheetfb_par *par, u16 data) > > { > > int i; > > > > +#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. Right ... although we don't *have* a GPIOLIB_BATCH, so that's not (yet?) an option. > > @@ -1056,6 +1056,128 @@ void __gpio_set_value(unsigned gpio, int value) > > } > > EXPORT_SYMBOL_GPL(__gpio_set_value); > > > > +#ifdef CONFIG_GPIOLIB_BATCH > > +/** > > + * __gpio_set_batch() - assign a batch of gpio pins together > > + * @gpio: starting gpio pin > > + * @values: values to assign, sequential and including masked bits > > + * @bitmask: bitmask to be applied to values > > + * @bitwidth: width inclusive of masked-off bits > > + * Context: any > > + * > > + * This is used directly or indirectly to implement gpio_set_value(). > > + * It invokes the associated gpio_chip.set_batch() method. If that > > + * method does not exist for any segment that is involved, then it drops > > + * back down to standard gpio_chip.set() > > + */ > > +void __gpio_set_batch(unsigned gpio, u32 values, u32 bitmask, int bitwidth) > > +{ > > + struct gpio_chip *chip; > > + int i = 0; > > + int value, width, remwidth; > > + u32 mask; > > + > > + do { > > + chip = gpio_to_chip(gpio + i); > > + WARN_ON(extra_checks && chip->can_sleep); > > + > > + 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. Right ... this is something I had suggested earlier: make sure that the (renamed) "batch" interfaces don't depend on some TBD extension to gpio_chip. Those extensions should be just an optimization. ------------------------------------------------------------------------------ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-12-29 20:34 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-12-27 16:24 [RFC 2.6.27 1/1] gpiolib: add batch set/get Jaya Kumar 2008-12-29 3:38 ` Eric Miao 2008-12-29 4:12 ` Jaya Kumar 2008-12-29 20:33 ` David Brownell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).