[PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[Peter Tyser]

This change resolves 2 issues:
- Different chips have a different number of GPIO pins per controller.
  For example, the MPC8347 has 32, the P2020 16, and the mpc8572 8.
  Previously, the mpc8xxx_gpio driver assumed every chip had 32 GPIO
  pins which resulted in some processors reporting an incorrect 'ngpio'
  field in /sys.  Additionally, users could export and "use" 32 GPIO
  pins, although in reality only a subset of the 32 pins had any real
  functionality.

- Some boards don't utilize all available GPIO pins.  Previously,
  unused GPIO pins could still be exported and "used", even though the
  pins had no real functionality.  This is somewhat confusing to a user
  and also allow a user to do something "bad", like change an unused
  floating output into a floating input.

Adding a new "fsl,gpio-mask" device tree property allows a dts file to
accurately describe what GPIO pins are available for use on a given
board.

Note that the 'ngpio' value reported in /sys will represent the
"highest" gpio pin accessible, not the total number of gpio pins
available.  For example, if a device only allowed the use of GPIO pin 3
(fsl,gpio-mask = 0x8), 'ngpio' would be reported as 4, although
only GPIO pin 3 could be exported and used.

Signed-off-by: Peter Tyser <ptyser@xes-inc.com>
---
I don't know which GPIO pins are usable on Freescale boards.  Let me
know if the default mask for the reference boards should change.

Thanks,
Peter

 .../powerpc/dts-bindings/fsl/8xxx_gpio.txt         |   11 ++++++--
 arch/powerpc/boot/dts/mpc8377_rdb.dts              |    2 +
 arch/powerpc/boot/dts/mpc8377_wlan.dts             |    2 +
 arch/powerpc/boot/dts/mpc8378_rdb.dts              |    2 +
 arch/powerpc/boot/dts/mpc8379_rdb.dts              |    2 +
 arch/powerpc/boot/dts/p2020ds.dts                  |    1 +
 arch/powerpc/boot/dts/p2020rdb.dts                 |    1 +
 arch/powerpc/boot/dts/xcalibur1501.dts             |    1 +
 arch/powerpc/boot/dts/xpedite5301.dts              |    1 +
 arch/powerpc/boot/dts/xpedite5330.dts              |    1 +
 arch/powerpc/boot/dts/xpedite5370.dts              |    1 +
 arch/powerpc/sysdev/mpc8xxx_gpio.c                 |   24 ++++++++++++++++---
 12 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt b/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt
index d015dce..a8fac7f 100644
--- a/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/8xxx_gpio.txt
@@ -11,9 +11,12 @@ Required properties:
   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.
+- interrupts : Interrupt mapping for GPIO IRQ (currently unused).
+- interrupt-parent : Phandle for the interrupt controller that
+  services interrupts for this device.
+- fsl,gpio-mask: A bitmask representing which GPIO pins are availabe for
+  use.  For example, a value of 0x13 means GPIO pins 0, 1, and 4 are
+  usable.
 - gpio-controller : Marks the port as GPIO controller.
 
 Example of gpio-controller nodes for a MPC8347 SoC:
@@ -24,6 +27,7 @@ Example of gpio-controller nodes for a MPC8347 SoC:
 		reg = <0xc00 0x100>;
 		interrupts = <74 0x8>;
 		interrupt-parent = <&ipic>;
+		fsl,gpio-mask = <0xffffffff>;
 		gpio-controller;
 	};
 
@@ -33,6 +37,7 @@ Example of gpio-controller nodes for a MPC8347 SoC:
 		reg = <0xd00 0x100>;
 		interrupts = <75 0x8>;
 		interrupt-parent = <&ipic>;
+		fsl,gpio-mask = <0xffffffff>;
 		gpio-controller;
 	};
 
diff --git a/arch/powerpc/boot/dts/mpc8377_rdb.dts b/arch/powerpc/boot/dts/mpc8377_rdb.dts
index 9e2264b..6152bfa 100644
--- a/arch/powerpc/boot/dts/mpc8377_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8377_rdb.dts
@@ -115,6 +115,7 @@
 			reg = <0xc00 0x100>;
 			interrupts = <74 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0xffffffff>;
 			gpio-controller;
 		};
 
@@ -124,6 +125,7 @@
 			reg = <0xd00 0x100>;
 			interrupts = <75 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0xffffffff>;
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/mpc8377_wlan.dts b/arch/powerpc/boot/dts/mpc8377_wlan.dts
index 9ea7830..a26535c 100644
--- a/arch/powerpc/boot/dts/mpc8377_wlan.dts
+++ b/arch/powerpc/boot/dts/mpc8377_wlan.dts
@@ -105,6 +105,7 @@
 			reg = <0xc00 0x100>;
 			interrupts = <74 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0xffffffff>;
 			gpio-controller;
 		};
 
@@ -114,6 +115,7 @@
 			reg = <0xd00 0x100>;
 			interrupts = <75 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0xffffffff>;
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/mpc8378_rdb.dts b/arch/powerpc/boot/dts/mpc8378_rdb.dts
index 4e6a1a4..11ba39c 100644
--- a/arch/powerpc/boot/dts/mpc8378_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8378_rdb.dts
@@ -115,6 +115,7 @@
 			reg = <0xc00 0x100>;
 			interrupts = <74 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0xffffffff>;
 			gpio-controller;
 		};
 
@@ -124,6 +125,7 @@
 			reg = <0xd00 0x100>;
 			interrupts = <75 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0xffffffff>;
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/mpc8379_rdb.dts b/arch/powerpc/boot/dts/mpc8379_rdb.dts
index 72336d5..975bdd7 100644
--- a/arch/powerpc/boot/dts/mpc8379_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8379_rdb.dts
@@ -113,6 +113,7 @@
 			reg = <0xc00 0x100>;
 			interrupts = <74 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0x3ffffff>; /* mpc8379 has 26 gpio */
 			gpio-controller;
 		};
 
@@ -122,6 +123,7 @@
 			reg = <0xd00 0x100>;
 			interrupts = <75 0x8>;
 			interrupt-parent = <&ipic>;
+			fsl,gpio-mask = <0x3ffffff>; /* mpc8379 has 26 gpio */
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/p2020ds.dts b/arch/powerpc/boot/dts/p2020ds.dts
index 1101914..e64a936 100644
--- a/arch/powerpc/boot/dts/p2020ds.dts
+++ b/arch/powerpc/boot/dts/p2020ds.dts
@@ -277,6 +277,7 @@
 			reg = <0xf000 0x100>;
 			interrupts = <47 0x2>;
 			interrupt-parent = <&mpic>;
+			fsl,gpio-mask = <0xffff>; /* p2020 has 16 gpio*/
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/p2020rdb.dts b/arch/powerpc/boot/dts/p2020rdb.dts
index da4cb0d..53f75ca 100644
--- a/arch/powerpc/boot/dts/p2020rdb.dts
+++ b/arch/powerpc/boot/dts/p2020rdb.dts
@@ -337,6 +337,7 @@
 			reg = <0xf000 0x100>;
 			interrupts = <47 0x2>;
 			interrupt-parent = <&mpic>;
+			fsl,gpio-mask = <0xffff>; /* p2020 has 16 gpio*/
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/xcalibur1501.dts b/arch/powerpc/boot/dts/xcalibur1501.dts
index ac0a617..858668c 100644
--- a/arch/powerpc/boot/dts/xcalibur1501.dts
+++ b/arch/powerpc/boot/dts/xcalibur1501.dts
@@ -598,6 +598,7 @@
 			interrupts = <47 2>;
 			interrupt-parent = <&mpic>;
 			#gpio-cells = <2>;
+			fsl,gpio-mask = <0xf0>; /* 4 LEDs */
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/xpedite5301.dts b/arch/powerpc/boot/dts/xpedite5301.dts
index db7faf5..944f08c 100644
--- a/arch/powerpc/boot/dts/xpedite5301.dts
+++ b/arch/powerpc/boot/dts/xpedite5301.dts
@@ -508,6 +508,7 @@
 			interrupts = <47 2>;
 			interrupt-parent = <&mpic>;
 			#gpio-cells = <2>;
+			fsl,gpio-mask = <0xf0>; /* 4 LEDs */
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/xpedite5330.dts b/arch/powerpc/boot/dts/xpedite5330.dts
index c364ca6..1dacacd 100644
--- a/arch/powerpc/boot/dts/xpedite5330.dts
+++ b/arch/powerpc/boot/dts/xpedite5330.dts
@@ -544,6 +544,7 @@
 			interrupts = <47 2>;
 			interrupt-parent = <&mpic>;
 			#gpio-cells = <2>;
+			fsl,gpio-mask = <0xf0>; /* 4 LEDs */
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/boot/dts/xpedite5370.dts b/arch/powerpc/boot/dts/xpedite5370.dts
index 7a8a4af..d783a50 100644
--- a/arch/powerpc/boot/dts/xpedite5370.dts
+++ b/arch/powerpc/boot/dts/xpedite5370.dts
@@ -506,6 +506,7 @@
 			interrupts = <47 2>;
 			interrupt-parent = <&mpic>;
 			#gpio-cells = <2>;
+			fsl,gpio-mask = <0xf0>; /* 4 LEDs */
 			gpio-controller;
 		};
 
diff --git a/arch/powerpc/sysdev/mpc8xxx_gpio.c b/arch/powerpc/sysdev/mpc8xxx_gpio.c
index 103eace..2c07052 100644
--- a/arch/powerpc/sysdev/mpc8xxx_gpio.c
+++ b/arch/powerpc/sysdev/mpc8xxx_gpio.c
@@ -16,8 +16,6 @@
 #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
@@ -34,11 +32,12 @@ struct mpc8xxx_gpio_chip {
 	 * open drain mode safely
 	 */
 	u32 data;
+	u32 valid_pins; /* Bitmask of valid gpio pins */
 };
 
 static inline u32 mpc8xxx_gpio2mask(unsigned int gpio)
 {
-	return 1u << (MPC8XXX_GPIO_PINS - 1 - gpio);
+	return 0x80000000 >> gpio;
 }
 
 static inline struct mpc8xxx_gpio_chip *
@@ -111,12 +110,23 @@ static int mpc8xxx_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val
 	return 0;
 }
 
+static int mpc8xxx_gpio_request(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);
+
+	if (mpc8xxx_gc->valid_pins & (1 << gpio))
+		return 0;
+
+	return -1;
+}
+
 static void __init mpc8xxx_add_controller(struct device_node *np)
 {
 	struct mpc8xxx_gpio_chip *mpc8xxx_gc;
 	struct of_mm_gpio_chip *mm_gc;
 	struct of_gpio_chip *of_gc;
 	struct gpio_chip *gc;
+	const u32 *prop;
 	int ret;
 
 	mpc8xxx_gc = kzalloc(sizeof(*mpc8xxx_gc), GFP_KERNEL);
@@ -133,11 +143,17 @@ static void __init mpc8xxx_add_controller(struct device_node *np)
 
 	mm_gc->save_regs = mpc8xxx_gpio_save_regs;
 	of_gc->gpio_cells = 2;
-	gc->ngpio = MPC8XXX_GPIO_PINS;
+	prop = of_get_property(np, "fsl,gpio-mask", NULL);
+	if (prop)
+		mpc8xxx_gc->valid_pins = *prop;
+	else
+		mpc8xxx_gc->valid_pins = 0xffffffff;
+	gc->ngpio = fls(mpc8xxx_gc->valid_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;
+	gc->request = mpc8xxx_gpio_request;
 
 	ret = of_mm_gpiochip_add(np, mm_gc);
 	if (ret)
-- 
1.6.2.1

Re: [PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[Anton Vorontsov]

Hi Peter,

On Fri, Dec 04, 2009 at 01:43:40PM -0600, Peter Tyser wrote:
> This change resolves 2 issues:
> - Different chips have a different number of GPIO pins per controller.
>   For example, the MPC8347 has 32, the P2020 16, and the mpc8572 8.
>   Previously, the mpc8xxx_gpio driver assumed every chip had 32 GPIO
>   pins which resulted in some processors reporting an incorrect 'ngpio'
>   field in /sys.  Additionally, users could export and "use" 32 GPIO
>   pins, although in reality only a subset of the 32 pins had any real
>   functionality.
> 
> - Some boards don't utilize all available GPIO pins.  Previously,
>   unused GPIO pins could still be exported and "used", even though the
>   pins had no real functionality.  This is somewhat confusing to a user
>   and also allow a user to do something "bad", like change an unused
>   floating output into a floating input.

There are hundreds of other ways to screw things up.

Think of /dev/mem, you still able to change the registers.
Before changing any GPIO (whether it is a normal or reserved GPIO),
user has to consult with schematics/docs.

> Adding a new "fsl,gpio-mask" device tree property allows a dts file to
> accurately describe what GPIO pins are available for use on a given
> board.

I don't see any real usage for this. If device tree specifies a wrong
gpio in the gpios = <> property, then it's a bug in the device tree
and should be fixed (or workarounded in the platform code).

If a user fiddles with unknown gpios via sysfs interface, then it's
user's problem.

FWIW, we don't have any masks for reserved IRQs.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[Peter Tyser]

Hi Anton,
Thanks for the feedback.

> On Fri, Dec 04, 2009 at 01:43:40PM -0600, Peter Tyser wrote:
> > This change resolves 2 issues:
> > - Different chips have a different number of GPIO pins per controller.
> >   For example, the MPC8347 has 32, the P2020 16, and the mpc8572 8.
> >   Previously, the mpc8xxx_gpio driver assumed every chip had 32 GPIO
> >   pins which resulted in some processors reporting an incorrect 'ngpio'
> >   field in /sys.  Additionally, users could export and "use" 32 GPIO
> >   pins, although in reality only a subset of the 32 pins had any real
> >   functionality.
> > 
> > - Some boards don't utilize all available GPIO pins.  Previously,
> >   unused GPIO pins could still be exported and "used", even though the
> >   pins had no real functionality.  This is somewhat confusing to a user
> >   and also allow a user to do something "bad", like change an unused
> >   floating output into a floating input.
> 
> There are hundreds of other ways to screw things up.
> 
> Think of /dev/mem, you still able to change the registers.
> Before changing any GPIO (whether it is a normal or reserved GPIO),
> user has to consult with schematics/docs.

Agreed.  This is an attempt to make it just a little bit harder to
accidentally screw things up and to make the "ngpio" sysfs value
actually contain an accurate value.

> > Adding a new "fsl,gpio-mask" device tree property allows a dts file to
> > accurately describe what GPIO pins are available for use on a given
> > board.
> 
> I don't see any real usage for this. If device tree specifies a wrong
> gpio in the gpios = <> property, then it's a bug in the device tree
> and should be fixed (or workarounded in the platform code).
> 
> If a user fiddles with unknown gpios via sysfs interface, then it's
> user's problem.

Its the sysfs case that I'm concerned about.  Primarily because:
1. Users scratch their head when they see that the "ngpio" sysfs value
doesn't match their CPU manual or board vendor's manual, and
subsequently ask their board vendor's engineers (ie me:) what's up.

2. Improperly using GPIO pins could damage hardware for some boards.
For example, some of our boards have a voltage regulator controlled via
GPIO pins so that a CPU's core voltage can be changed based on its
frequency, etc.  A user could damage their CPU if they aren't careful
with those GPIO pins.  For pins like that, it'd be nice to not even let
users play with them.

#2 could be worked around by exporting GPIO pins in platform code so
that they are not available via sysfs.  And I agree that if a user is
playing with GPIO pins, they had better know what they are doing, so #1
above is my main issue.  Would it be any more acceptable to instead add
a "fsl,num-gpio" property so that "ngpio" actually reported an accurate
value and non-existent GPIO pins couldn't be used/exported?

With the patch as is, if "fsl,gpio-mask" is not given, the driver
defaults to enabling all 32 gpio pins.  Would it be any better if I
respun the patch to only add the "fsl,gpio-mask" property for the
mpc8572, p2020, and mpc8379 boards which have less than 32 gpio pins and
document the dts property as optional?

Thanks,
Peter

Re: [PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[Anton Vorontsov]

On Sat, Dec 05, 2009 at 01:32:32PM -0600, Peter Tyser wrote:
[...]
> > > Adding a new "fsl,gpio-mask" device tree property allows a dts file to
> > > accurately describe what GPIO pins are available for use on a given
> > > board.
> > 
> > I don't see any real usage for this. If device tree specifies a wrong
> > gpio in the gpios = <> property, then it's a bug in the device tree
> > and should be fixed (or workarounded in the platform code).
> > 
> > If a user fiddles with unknown gpios via sysfs interface, then it's
> > user's problem.
> 
> Its the sysfs case that I'm concerned about.  Primarily because:
> 1. Users scratch their head when they see that the "ngpio" sysfs value
> doesn't match their CPU manual or board vendor's manual, and
> subsequently ask their board vendor's engineers (ie me:) what's up.

I don't think that adding code and device tree entries just for
documentation purposes is a good idea.

> 2. Improperly using GPIO pins could damage hardware for some boards.

Well, your initial patch tried to solve a different problem: to not
let users to request non-existent GPIOs, which is usually safe.

[...]
> #2 could be worked around by exporting GPIO pins in platform code so
> that they are not available via sysfs.

Yes, badly designed hardware deserves ugly hacks in the platform
code. ;-) So for this problem, just request these gpios in the
platform code.

> Would it be any more acceptable to instead add
> a "fsl,num-gpio" property so that "ngpio" actually reported an accurate
> value and non-existent GPIO pins couldn't be used/exported?

I'd think it's actually less acceptable. fsl,gpio-mask is more generic,
since from gpio-mask you can deduce ngpio. But it's still ugly.

What would be OK to do is to describe in the device tree every
device that is using some GPIO, and then let the userspace request
*only* gpios that are described in the device-tree. That way you
can automatically exclude not-existent gpios.
And if some gpios are just headers on the board, you can still
describe them in the device tree via "gpio-header" nodes.

Still, a lot of efforts for no real gain...

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[Peter Tyser]

Hi Anton,
I've CC-ed devicetree-discuss.  The original patch is at
http://patchwork.ozlabs.org/patch/40361/ for reference.

On Sat, 2009-12-05 at 23:51 +0300, Anton Vorontsov wrote:
> On Sat, Dec 05, 2009 at 01:32:32PM -0600, Peter Tyser wrote:
> [...]
> > > > Adding a new "fsl,gpio-mask" device tree property allows a dts file to
> > > > accurately describe what GPIO pins are available for use on a given
> > > > board.
> > > 
> > > I don't see any real usage for this. If device tree specifies a wrong
> > > gpio in the gpios = <> property, then it's a bug in the device tree
> > > and should be fixed (or workarounded in the platform code).
> > > 
> > > If a user fiddles with unknown gpios via sysfs interface, then it's
> > > user's problem.
> > 
> > Its the sysfs case that I'm concerned about.  Primarily because:
> > 1. Users scratch their head when they see that the "ngpio" sysfs value
> > doesn't match their CPU manual or board vendor's manual, and
> > subsequently ask their board vendor's engineers (ie me:) what's up.
> 
> I don't think that adding code and device tree entries just for
> documentation purposes is a good idea.

Its not just for documentation purposes.  Right now, the sysfs "ngpio"
value is flat out wrong for some processors, regardless of
documentation.  Granted its not a critical bug, but I'd still consider
it a bug.

> > 2. Improperly using GPIO pins could damage hardware for some boards.
> 
> Well, your initial patch tried to solve a different problem: to not
> let users to request non-existent GPIOs, which is usually safe.

I can update the commit message with this rational if it makes a
difference.

> [...]
> > #2 could be worked around by exporting GPIO pins in platform code so
> > that they are not available via sysfs.
> 
> Yes, badly designed hardware deserves ugly hacks in the platform
> code. ;-) So for this problem, just request these gpios in the
> platform code.

I'd wager lots of boards have GPIO pins that a user shouldn't play
around with once Linux boots up.  Like GPIO pins used to program an
FPGA, or control a PLL, etc.  1 device tree property is nicer than
hacking up lots of platform code...

> > Would it be any more acceptable to instead add
> > a "fsl,num-gpio" property so that "ngpio" actually reported an accurate
> > value and non-existent GPIO pins couldn't be used/exported?
> 
> I'd think it's actually less acceptable. fsl,gpio-mask is more generic,
> since from gpio-mask you can deduce ngpio. But it's still ugly.
> 
> What would be OK to do is to describe in the device tree every
> device that is using some GPIO, and then let the userspace request
> *only* gpios that are described in the device-tree. That way you
> can automatically exclude not-existent gpios.
> And if some gpios are just headers on the board, you can still
> describe them in the device tree via "gpio-header" nodes.
> 
> Still, a lot of efforts for no real gain...

Agreed.  Seems like a clean solution, but is a chunk of work.

In any case, my high-level thought process is:
1. Currently, the "ngpio" value is wrong for some processors and should
be fixed.
2. Adding a new "fsl,gpio-mask" gpio solves #1, and has the benefit of
allowing the device tree to easily reserve GPIO pins which should not be
used in Linux.

I guess I'm not seeing the big downside of a new "fsl,gpio-mask"
property.  Its the device tree's job to describe the hardware.  The
change is pretty minimal (~15 lines), and the property can be made
optional.

Or is there another suggestion on how to resolve #1 above?  I consider
it a bug and would like to fix it.

Best,
Peter

Re: [PATCH] powerpc: mpc8xxx_gpio: Add ability to mask off unused GPIO pins

[Anton Vorontsov]

On Mon, Dec 07, 2009 at 10:23:18AM -0600, Peter Tyser wrote:
[...]
> > Yes, badly designed hardware deserves ugly hacks in the platform
> > code. ;-) So for this problem, just request these gpios in the
> > platform code.
> 
> I'd wager lots of boards have GPIO pins that a user shouldn't play
> around with once Linux boots up.  Like GPIO pins used to program an
> FPGA,

What if I do want to program/upgrade an FPGA, say from the userspace?

[...]
> In any case, my high-level thought process is:
> 1. Currently, the "ngpio" value is wrong for some processors and should
> be fixed.
> 2. Adding a new "fsl,gpio-mask" gpio solves #1, and has the benefit of
> allowing the device tree to easily reserve GPIO pins which should not be
> used in Linux.

OK, if you're so eager to fix this... I'd suggest to not put this
stuff into device tree. We have compatible property that describes
the device.

So, in the driver, you can just do

if (of_device_is_compatible("fsl,foo-gpiochip"))
	mask = ...;
else if (of_device_is_compatible("fsl,bar-gpiochip"))
	mask = ...;

If you want to solve "dangerous" GPIOs problem, then I'd say

chosen {
	linux,dangerous-gpios = <&pio1 0 0 &pio1 1 0
				 &pio2 4 0 &pio1 5 0>;
};

And then you can have a generic driver that looks for
linux,dangerous-gpios and requests them at boot.

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2