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

Hi Anton,
Thanks for the feedback.

On Fri, Dec 04, 2009 at 01:43:40PM -0600, Peter Tyser wrote:

quoted

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.

quoted

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