[v2] powerpc: Basic GPIO support for GE Fanuc SBC610

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 <martyn.welch@gefanuc.com>
---

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(-)

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 <martyn.welch@gefanuc.com>
> ---

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: gpio@7,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;
> +		};
>  	};
>  
>  	soc@fef00000 {
> 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 <martyn.welch@gefanuc.com>
> + *
> + * 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 <martyn.welch@gefanuc.com");
> +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,

On Thu, Nov 06, 2008 at 06:17:23PM +0300, Anton Vorontsov wrote:
[...]
> There is point in doing the of_platform_driver for this GPIO

Please read as 'There is no point..'