subject:"\[PATCH v3\] powerpc\: gpio driver for mpc8349\/8572\/8610 and compatible with OF bindings"

Re: [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

2008-09-22 Thread Peter Korsgaard

> "Anton" == Anton Vorontsov <[EMAIL PROTECTED]> writes:

 Anton> 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]>

 Anton> Looks good to me, thanks.

 Anton> Few comments below, might want to consider some of them if you want to.

 >> +   gpio1: [EMAIL PROTECTED] {
 >> +   #gpio-cells = <2>;
 >> +   compatible = "fsl,mpc8347-gpio, fsl,mpc8349-gpio";

 Anton> Some quotes are missing. Should be "fsl,mpc8347-gpio",
 Anton> "fsl,mpc8349-gpio";

Argh, sorry about that.

 >> +#define GPIO_DIR   0x00
 >> +#define GPIO_ODR   0x04
 >> +#define GPIO_DAT   0x08
 >> +#define GPIO_IER   0x0c
 >> +#define GPIO_IMR   0x10
 >> +#define GPIO_ICR   0x14

 Anton> This is better described in a struct. Will save few characters,
 Anton> and just looks nicer. That is,

I think that's basically a question about taste. Some people like
structs, some don't - I prefer the defines as it's very easy to see
exactly what addresses gets used.

 >> +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 */

 Anton> Why not a canonical comment?

 Anton> /*
 Anton>  * Multi-line
 Anton>  * comment.
 Anton>  */

Fine with me. It started as a single line comment that I later
expanded on.

 >> +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));

 Anton> No need for !!. gpio api spec says that you may return any
 Anton> value != 0 for the logical 1. Negative values are ok.

True.

 >> +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));

 Anton> Would look better if you'd use clrbits32().

Ok.

 >> +static int __init mpc8xxx_add_controller(struct device_node *np)
 >> +{

 Anton> You don't check return value for this function. `void' return type
 Anton> would work.

Yes, I kept it as we already keep track of the return value within the
function (for error reporting) - I'll get rid of it.

I'll send an updated patch shortly.

-- 
Bye, Peter Korsgaard
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

2008-09-22 Thread Anton Vorontsov

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 000..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,-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 000..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 
> +#include 
> +#include 
> +#include 
> +

[PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

2008-09-21 Thread Peter Korsgaard

Structured similar to the existing QE GPIO support.

Signed-off-by: Peter Korsgaard <[EMAIL PROTECTED]>
---
 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 000..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,-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";
+   reg = <0xc00 0x100>;
+   interrupts = <74 0x8>;
+   interrupt-parent = <&ipic>;
+   gpio-controller;
+   };
+
+   gpio2: [EMAIL PROTECTED] {
+   #gpio-cells = <2>;
+   compatible = "fsl,mpc8347-gpio, fsl,mpc8349-gpio";
+   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 000..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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
+
+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
+

Re: [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

Re: [PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

[PATCH v3] powerpc: gpio driver for mpc8349/8572/8610 and compatible with OF bindings

3 matches

Site Navigation

Mail list logo

Footer information