On Thu, Nov 06, 2008 at 12:10:33PM +0000, Martyn Welch wrote: > Basic support for the GPIO available on the SBC610 VPX Single Board Computer > from GE Fanuc (PowerPC MPC8641D). > > This patch adds basic support for the GPIO in the devices I/O FPGA, the GPIO > functionality is exposed through the AFIX pins on the backplane, unless used > by an AFIX card. > > This code currently does not support switching between totem-pole and > open-drain outputs (when used as outputs, GPIOs default to totem-pole). > The interrupt capabilites of the GPIO lines is also not currently supported. > > Signed-off-by: Martyn Welch <[EMAIL PROTECTED]> > ---
Mostly looks good. Few comments below. > Sorry for the quick jump to version 2 - A stray "i" crept in to the previous > version as I was editing the email for submission. > > arch/powerpc/boot/dts/gef_sbc610.dts | 6 + > arch/powerpc/platforms/86xx/Kconfig | 2 > arch/powerpc/platforms/86xx/Makefile | 3 - > arch/powerpc/platforms/86xx/gef_gpio.c | 185 > ++++++++++++++++++++++++++++++++ > arch/powerpc/platforms/86xx/gef_gpio.h | 10 ++ > 5 files changed, 205 insertions(+), 1 deletions(-) > > diff --git a/arch/powerpc/boot/dts/gef_sbc610.dts > b/arch/powerpc/boot/dts/gef_sbc610.dts > index 6ed6083..963dd5b 100644 > --- a/arch/powerpc/boot/dts/gef_sbc610.dts > +++ b/arch/powerpc/boot/dts/gef_sbc610.dts > @@ -98,6 +98,12 @@ > interrupt-parent = <&mpic>; > > }; > + gef_gpio: [EMAIL PROTECTED],14000 { > + #gpio-cells = <1>; Don't use one-cell scheme. Two cells are convenient for GPIO flags (active-low GPIO, for example). > + compatible = "gef,fpga-gpio"; > + reg = <0x7 0x14000 0x24>; > + gpio-controller; > + }; > }; > > [EMAIL PROTECTED] { > diff --git a/arch/powerpc/platforms/86xx/Kconfig > b/arch/powerpc/platforms/86xx/Kconfig > index 77dd797..8e56939 100644 > --- a/arch/powerpc/platforms/86xx/Kconfig > +++ b/arch/powerpc/platforms/86xx/Kconfig > @@ -34,6 +34,8 @@ config MPC8610_HPCD > config GEF_SBC610 > bool "GE Fanuc SBC610" > select DEFAULT_UIMAGE > + select GENERIC_GPIO > + select ARCH_REQUIRE_GPIOLIB > select HAS_RAPIDIO > help > This option enables support for GE Fanuc's SBC610. > diff --git a/arch/powerpc/platforms/86xx/Makefile > b/arch/powerpc/platforms/86xx/Makefile > index 4a56ff6..31e540c 100644 > --- a/arch/powerpc/platforms/86xx/Makefile > +++ b/arch/powerpc/platforms/86xx/Makefile > @@ -7,4 +7,5 @@ obj-$(CONFIG_SMP) += mpc86xx_smp.o > obj-$(CONFIG_MPC8641_HPCN) += mpc86xx_hpcn.o > obj-$(CONFIG_SBC8641D) += sbc8641d.o > obj-$(CONFIG_MPC8610_HPCD) += mpc8610_hpcd.o > -obj-$(CONFIG_GEF_SBC610) += gef_sbc610.o gef_pic.o > +gef-gpio-$(CONFIG_GPIOLIB) += gef_gpio.o > +obj-$(CONFIG_GEF_SBC610) += gef_sbc610.o gef_pic.o $(gef-gpio-y) > diff --git a/arch/powerpc/platforms/86xx/gef_gpio.c > b/arch/powerpc/platforms/86xx/gef_gpio.c > new file mode 100644 > index 0000000..aafff1b > --- /dev/null > +++ b/arch/powerpc/platforms/86xx/gef_gpio.c > @@ -0,0 +1,185 @@ > +/* > + * Driver for GE Fanuc's FPGA based GPIO pins > + * > + * Author: Martyn Welch <[EMAIL PROTECTED]> > + * > + * 2008 (c) GE Fanuc Intelligent Platforms Embedded Systems, Inc. > + * > + * This file is licensed under the terms of the GNU General Public License > + * version 2. This program is licensed "as is" without any warranty of any > + * kind, whether express or implied. > + */ > + > +/* TODO > + * > + * Configuration of output modes (totem-pole/open-drain) > + * Interrupt configuration - interrupts are always generated the FPGA relies > on > + * the I/O interrupt controllers mask to stop them propergating > + */ #include <linux/compiler.h> for __iomem > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_platform.h> > +#include <linux/of_gpio.h> > +#include <linux/gpio.h> > + > +#include "gef_gpio.h" > + > +#define DEBUG > +#undef DEBUG > + > +#ifdef DEBUG > +#define DBG(fmt...) do { printk(KERN_DEBUG "gef_gpio: " fmt); } while (0) > +#else > +#define DBG(fmt...) do { } while (0) > +#endif There is pr_debug() function exists. > +#define NUM_GPIO 19 > + > +/* Let's create a common inlined function to commonise the code and so we > don't > + * have to resolve the chip structure multiple times. > + */ > +inline void _gef_gpio_set(void __iomem *reg, unsigned int offset, int value) 'static' missing. + No need for inline, gcc can decide if inlined function better or not. > +{ > + unsigned int data; > + > + data = ioread32be(reg); > + /* value: 0=low; 1=high */ > + DBG("Output Set, Pre:0x%8.8x, ", data); > + if (value & 0x1) { > + data = data | (0x1 << offset); > + DBG("OR Mask:0x%8.8x, Post:0x%8.8x\n", (0x1 << offset), data); > + } else { > + data = data & ~(0x1 << offset); > + DBG("AND Mask:0x%8.8x, Post:0x%8.8x\n", ~(0x1 << offset), data); > + } > + iowrite32be(data, reg); > +} > + > + > +static int gef_gpio_dir_in(struct gpio_chip *chip, unsigned offset) > +{ > + struct of_mm_gpio_chip *mmchip; > + unsigned int data; > + > + /* Find memory mapped gpio chip structure from gpio_chip, this contains > + * the mapped location of the GPIO controller */ Why not canonical-style comments? /* * Multi-line * comment. */ > + mmchip = to_of_mm_gpio_chip(chip); > + > + data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT); > + DBG("Direction In, Pre:0x%8.8x, ", data); > + data = data | (0x1 << offset); > + DBG("OR Mask:0x%8.8x, Post:0x%8.8x\n", (0x1 << offset), data); > + iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT); > + > + return 0; > +} > + > +static int gef_gpio_dir_out(struct gpio_chip *chip, unsigned offset, int > value) > +{ > + struct of_mm_gpio_chip *mmchip; > + unsigned int data; > + > + /* Find memory mapped gpio chip structure from gpio_chip, this contains > + * the mapped location of the GPIO controller */ > + mmchip = to_of_mm_gpio_chip(chip); > + > + /* Set direction before switching to input */ > + _gef_gpio_set(mmchip->regs + GEF_GPIO_OUT, offset, value); > + > + data = ioread32be(mmchip->regs + GEF_GPIO_DIRECT); > + DBG("Direction Out, Pre:0x%8.8x, ", data); > + data = data & ~(0x1 << offset); > + DBG("AND Mask:0x%8.8x, Post:0x%8.8x\n", ~(0x1 << offset), data); > + iowrite32be(data, mmchip->regs + GEF_GPIO_DIRECT); > + > + return 0; > +} > + > +static int gef_gpio_get(struct gpio_chip *chip, unsigned offset) > +{ > + struct of_mm_gpio_chip *mmchip; > + unsigned int data; > + int state = 0; > + > + /* Find memory mapped gpio chip structure from gpio_chip, this contains > + * the mapped location of the GPIO controller */ > + mmchip = to_of_mm_gpio_chip(chip); > + > + data = ioread32be(mmchip->regs + GEF_GPIO_IN); > + state = (int)((data >> offset) & 0x1); > + DBG("Get Input, Data:0x%8.8x\n", data); > + > + return state; > +} > + > +static void gef_gpio_set(struct gpio_chip *chip, unsigned offset, int value) > +{ > + struct of_mm_gpio_chip *mmchip; > + > + /* Find memory mapped gpio chip structure from gpio_chip, this contains > + * the mapped location of the GPIO controller */ > + mmchip = to_of_mm_gpio_chip(chip); > + > + _gef_gpio_set(mmchip->regs + GEF_GPIO_OUT, offset, value); > + > +} > + > +static int __devinit gef_gpio_probe(struct of_device *dev, > + const struct of_device_id *match) > +{ > + int retval; > + struct of_mm_gpio_chip *gef_gpio_chip; > + > + DBG("gef_gpio_probe() called for %s\n", dev->node->full_name); > + > + /* Allocate chip structure */ > + gef_gpio_chip = kzalloc(sizeof(*gef_gpio_chip), GFP_KERNEL); > + if (!gef_gpio_chip) { > + DBG("Unable to allocate GEF GPIO structure\n"); > + return -ENOMEM; > + } > + > + /* Setup pointers to chip functions */ > + gef_gpio_chip->of_gc.gpio_cells = 1; Two cells are preferred. > + gef_gpio_chip->of_gc.gc.ngpio = NUM_GPIO; > + gef_gpio_chip->of_gc.gc.direction_input = gef_gpio_dir_in; > + gef_gpio_chip->of_gc.gc.direction_output = gef_gpio_dir_out; > + gef_gpio_chip->of_gc.gc.get = gef_gpio_get; > + gef_gpio_chip->of_gc.gc.set = gef_gpio_set; > + > + /* This function adds a memory mapped GPIO chip */ > + retval = of_mm_gpiochip_add(dev->node, gef_gpio_chip); > + if (retval) > + return retval; gef_gpio_chip allocation leaked. > + > + return 0; > +}; > + > +static const struct of_device_id gef_gpio_ids[] = { > + { > + .compatible = "gef,fpga-gpio", Isn't this too generic? > + }, > + {}, > +}; > + > +static struct of_platform_driver gef_gpio_driver = { > + .name = "gef-of-gpio", > + .match_table = gef_gpio_ids, > + .probe = gef_gpio_probe, > +}; > + > +static __init int gef_gpio_init(void) > +{ > + DBG("gef_gpio_init()\n"); > + return of_register_platform_driver(&gef_gpio_driver); > +} > +subsys_initcall(gef_gpio_init); There is point in doing the of_platform_driver for this GPIO controller. More than that, of_platform bus is probed at the device_initcall time, so there is also no point in the subsys_initcall for this driver. I'd recommend to do the registration as in arch/powerpc/sysdev/qe_lib/gpio.c. > +MODULE_DESCRIPTION("GE Fanuc I/O FPGA GPIO driver"); > +MODULE_AUTHOR("Martyn Welch <[EMAIL PROTECTED]"); > +MODULE_LICENSE("GPL"); > + > diff --git a/arch/powerpc/platforms/86xx/gef_gpio.h > b/arch/powerpc/platforms/86xx/gef_gpio.h > new file mode 100755 > index 0000000..5f76a7c > --- /dev/null > +++ b/arch/powerpc/platforms/86xx/gef_gpio.h > @@ -0,0 +1,10 @@ > +#define GEF_GPIO_DIRECT 0x00 > +#define GEF_GPIO_IN 0x04 > +#define GEF_GPIO_OUT 0x08 > +#define GEF_GPIO_TRIG 0x0C > +#define GEF_GPIO_POLAR_A 0x10 > +#define GEF_GPIO_POLAR_B 0x14 > +#define GEF_GPIO_INT_STAT 0x18 > +#define GEF_GPIO_OVERRUN 0x1C > +#define GEF_GPIO_MODE 0x20 No need for this header. Just place this into the .c file. Thanks, -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev