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

2008-11-06 Thread Anton Vorontsov
On Thu, Nov 06, 2008 at 12:10:33PM +, 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 000..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);

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

2008-11-06 Thread Anton Vorontsov
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..'
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev