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

[Peter Tyser <ptyser@xes-inc.com>]

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