On Mon, Sep 22, 2008 at 08:32:34AM +0200, Peter Korsgaard wrote: > Structured similar to the existing QE GPIO support. > > Signed-off-by: Peter Korsgaard <[EMAIL PROTECTED]>
Looks good to me, thanks. Few comments below, might want to consider some of them if you want to. > --- > Changes since v2: > - Clarified documentation as requested by Kumar. > > Changes since v1: > Incorporated feedback from Anton and Kumar: > - Core is also used on 8572/8610 so s/83xx/8xxx/ > - Use fsl,mpc8572-gpio / fsl,mpc8610-gpio for 85xx/86xx as compatible > - Use shadowed data register to handle open drain outputs > - Expandend dts binding doc, use 8347 as example instead of 8349 > - Misc small cleanups > > .../powerpc/dts-bindings/fsl/8xxx_gpio.txt | 40 +++++ > arch/powerpc/sysdev/Kconfig | 9 + > arch/powerpc/sysdev/Makefile | 1 + > arch/powerpc/sysdev/mpc8xxx_gpio.c | 170 > ++++++++++++++++++++ > 4 files changed, 220 insertions(+), 0 deletions(-) > create mode 100644 Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt > create mode 100644 arch/powerpc/sysdev/mpc8xxx_gpio.c > > diff --git a/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt > b/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt > new file mode 100644 > index 0000000..26c29c4 > --- /dev/null > +++ b/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt > @@ -0,0 +1,40 @@ > +GPIO controllers on MPC8xxx SoCs > + > +This is for the non-QE/CPM/GUTs GPIO controllers as found on > +8349, 8572, 8610 and compatible. > + > +Every GPIO controller node must have #gpio-cells property defined, > +this information will be used to translate gpio-specifiers. > + > +Required properties: > +- compatible : "fsl,<CHIP>-gpio" followed by "fsl,mpc8349-gpio" for > + 83xx, "fsl,mpc8572-gpio" for 85xx and "fsl,mpc8610-gpio" for 86xx. > +- #gpio-cells : Should be two. The first cell is the pin number and the > + second cell is used to specify optional parameters (currently unused). > + - interrupts : Interrupt mapping for GPIO IRQ (currently unused). > + - interrupt-parent : Phandle for the interrupt controller that > + services interrupts for this device. > +- gpio-controller : Marks the port as GPIO controller. > + > +Example of gpio-controller nodes for a MPC8347 SoC: > + > + gpio1: [EMAIL PROTECTED] { > + #gpio-cells = <2>; > + compatible = "fsl,mpc8347-gpio, fsl,mpc8349-gpio"; Some quotes are missing. Should be "fsl,mpc8347-gpio", "fsl,mpc8349-gpio"; > + reg = <0xc00 0x100>; > + interrupts = <74 0x8>; > + interrupt-parent = <&ipic>; > + gpio-controller; > + }; > + > + gpio2: [EMAIL PROTECTED] { > + #gpio-cells = <2>; > + compatible = "fsl,mpc8347-gpio, fsl,mpc8349-gpio"; Ditto. > + reg = <0xd00 0x100>; > + interrupts = <75 0x8>; > + interrupt-parent = <&ipic>; > + gpio-controller; > + }; > + > +See booting-without-of.txt for details of how to specify GPIO > +information for devices. > diff --git a/arch/powerpc/sysdev/Kconfig b/arch/powerpc/sysdev/Kconfig > index 72fb35b..a11cc8f 100644 > --- a/arch/powerpc/sysdev/Kconfig > +++ b/arch/powerpc/sysdev/Kconfig > @@ -6,3 +6,12 @@ config PPC4xx_PCI_EXPRESS > bool > depends on PCI && 4xx > default n > + > +config MPC8xxx_GPIO > + bool "MPC8xxx GPIO support" > + depends on PPC_MPC831x || PPC_MPC834x || PPC_MPC837x || PPC_85xx || > PPC_86xx > + select GENERIC_GPIO > + select ARCH_REQUIRE_GPIOLIB > + help > + Say Y here if you're going to use hardware that connects to the > + MPC831x/834x/837x/8572/8610 GPIOs. > diff --git a/arch/powerpc/sysdev/Makefile b/arch/powerpc/sysdev/Makefile > index a90054b..e410764 100644 > --- a/arch/powerpc/sysdev/Makefile > +++ b/arch/powerpc/sysdev/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_FSL_SOC) += fsl_soc.o > obj-$(CONFIG_FSL_PCI) += fsl_pci.o $(fsl-msi-obj-y) > obj-$(CONFIG_FSL_LBC) += fsl_lbc.o > obj-$(CONFIG_FSL_GTM) += fsl_gtm.o > +obj-$(CONFIG_MPC8xxx_GPIO) += mpc8xxx_gpio.o > obj-$(CONFIG_RAPIDIO) += fsl_rio.o > obj-$(CONFIG_TSI108_BRIDGE) += tsi108_pci.o tsi108_dev.o > obj-$(CONFIG_QUICC_ENGINE) += qe_lib/ > diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c > b/arch/powerpc/sysdev/mpc8xxx_gpio.c > new file mode 100644 > index 0000000..3c1f608 > --- /dev/null > +++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c > @@ -0,0 +1,170 @@ > +/* > + * GPIOs on MPC8349/8572/8610 and compatible > + * > + * Copyright (C) 2008 Peter Korsgaard <[EMAIL PROTECTED]> > + * > + * 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. > + */ > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/spinlock.h> > +#include <linux/io.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/gpio.h> > + > +#define MPC8XXX_GPIO_PINS 32 > + > +#define GPIO_DIR 0x00 > +#define GPIO_ODR 0x04 > +#define GPIO_DAT 0x08 > +#define GPIO_IER 0x0c > +#define GPIO_IMR 0x10 > +#define GPIO_ICR 0x14 This is better described in a struct. Will save few characters, and just looks nicer. That is, mm->regs + GPIO_DAT vs. &mm->regs->dat > +struct mpc8xxx_gpio_chip { > + struct of_mm_gpio_chip mm_gc; > + spinlock_t lock; > + > + /* shadowed data register to be able to clear/set output pins in > + open drain mode safely */ Why not a canonical comment? /* * Multi-line * comment. */ > + u32 data; > +}; > + > +static inline u32 mpc8xxx_gpio2mask(unsigned int gpio) > +{ > + return 1u << (MPC8XXX_GPIO_PINS - 1 - gpio); `u` isn't necessary. > +} > + > +static inline struct mpc8xxx_gpio_chip * > +to_mpc8xxx_gpio_chip(struct of_mm_gpio_chip *mm) > +{ > + return container_of(mm, struct mpc8xxx_gpio_chip, mm_gc); > +} > + > +static void mpc8xxx_gpio_save_regs(struct of_mm_gpio_chip *mm) > +{ > + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); > + > + mpc8xxx_gc->data = in_be32(mm->regs + GPIO_DAT); > +} > + > +static int mpc8xxx_gpio_get(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); > + > + return !!(in_be32(mm->regs + GPIO_DAT) & mpc8xxx_gpio2mask(gpio)); No need for !!. gpio api spec says that you may return any value != 0 for the logical 1. Negative values are ok. > +} > + > +static void mpc8xxx_gpio_set(struct gpio_chip *gc, unsigned int gpio, int > val) > +{ > + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); > + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); > + unsigned long flags; > + > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + > + if (val) > + mpc8xxx_gc->data |= mpc8xxx_gpio2mask(gpio); > + else > + mpc8xxx_gc->data &= ~mpc8xxx_gpio2mask(gpio); > + > + out_be32(mm->regs + GPIO_DAT, mpc8xxx_gc->data); > + > + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > +} > + > +static int mpc8xxx_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio) > +{ > + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); > + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); > + unsigned long flags; > + > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + > + out_be32(mm->regs + GPIO_DIR, > + in_be32(mm->regs + GPIO_DIR) & ~mpc8xxx_gpio2mask(gpio)); Would look better if you'd use clrbits32(). > + > + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > + > + return 0; > +} > + > +static int mpc8xxx_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int > val) > +{ > + struct of_mm_gpio_chip *mm = to_of_mm_gpio_chip(gc); > + struct mpc8xxx_gpio_chip *mpc8xxx_gc = to_mpc8xxx_gpio_chip(mm); > + unsigned long flags; > + > + mpc8xxx_gpio_set(gc, gpio, val); > + > + spin_lock_irqsave(&mpc8xxx_gc->lock, flags); > + out_be32(mm->regs + GPIO_DIR, > + in_be32(mm->regs + GPIO_DIR) | mpc8xxx_gpio2mask(gpio)); setbits32(). > + > + spin_unlock_irqrestore(&mpc8xxx_gc->lock, flags); > + > + return 0; > +} > + > +static int __init mpc8xxx_add_controller(struct device_node *np) > +{ You don't check return value for this function. `void' return type would work. > + struct mpc8xxx_gpio_chip *mpc8xxx_gc; > + struct of_mm_gpio_chip *mm_gc; > + struct of_gpio_chip *of_gc; > + struct gpio_chip *gc; > + int ret; > + > + mpc8xxx_gc = kzalloc(sizeof(*mpc8xxx_gc), GFP_KERNEL); > + if (!mpc8xxx_gc) { > + ret = -ENOMEM; > + goto err; > + } > + > + spin_lock_init(&mpc8xxx_gc->lock); > + > + mm_gc = &mpc8xxx_gc->mm_gc; > + of_gc = &mm_gc->of_gc; > + gc = &of_gc->gc; > + > + mm_gc->save_regs = mpc8xxx_gpio_save_regs; > + of_gc->gpio_cells = 2; > + gc->ngpio = MPC8XXX_GPIO_PINS; > + gc->direction_input = mpc8xxx_gpio_dir_in; > + gc->direction_output = mpc8xxx_gpio_dir_out; > + gc->get = mpc8xxx_gpio_get; > + gc->set = mpc8xxx_gpio_set; > + > + ret = of_mm_gpiochip_add(np, mm_gc); > + if (ret) > + goto err; > + > + return 0; > + > +err: > + pr_err("%s: registration failed with status %d\n", > + np->full_name, ret); > + kfree(mpc8xxx_gc); > + > + return ret; > +} > + > +static int __init mpc8xxx_add_gpiochips(void) > +{ > + struct device_node *np; > + > + for_each_compatible_node(np, NULL, "fsl,mpc8349-gpio") > + mpc8xxx_add_controller(np); > + > + for_each_compatible_node(np, NULL, "fsl,mpc8572-gpio") > + mpc8xxx_add_controller(np); > + > + for_each_compatible_node(np, NULL, "fsl,mpc8610-gpio") > + mpc8xxx_add_controller(np); > + > + return 0; > +} > +arch_initcall(mpc8xxx_add_gpiochips); > -- > 1.5.6.3 -- 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