Re: [PATCH] gpio: tegra186: Add support for T186 GPIO

[Thierry Reding]

On Tue, Nov 08, 2016 at 12:07:55PM -0700, Stephen Warren wrote:
> On 11/02/2016 04:48 AM, Suresh Mangipudi wrote:
> > Add GPIO driver for T186 based platforms.
> > Adds support for MAIN and AON GPIO's from T186.
> 
> I'm not sure how you/Thierry will approach merging this with the other GPIO
> driver he has, but here's a very quick review of this one in case it's
> useful.

This puts me in an unfortunate situation. I'd really like to avoid being
the maintainer for this driver, but on the other hand, the version of
the driver that I wrote is pretty much what we'd end up if Stephen's
comments were all addressed. Suresh's driver does a couple of things in
addition (like the accessibility checks), but then I find my driver to
be more easily related to the TRM because it uses the register names
from that.

So I don't really know how to go about merging both. I'll reply to this
email later with a copy of the patch that I wrote, maybe we can take it
from there.

> > +	tgi->gc.ngpio			= tgi->soc->nports * 8;
> 
> This will leave some gaps in the GPIO numbering, since not all ports have 8
> GPIOs. I think this is the correct thing to do, but IIRC Thierry found this
> caused some issues in the GPIO core since it attempts to query initial
> status of each GPIO. Did you see this issue during testing?

I think the immediate issue that I was seeing is avoided in this driver
by the call to gpio_is_accessible() in the ->get_direction() callback.
In the driver that I have there's no such check, and hence I would get
an exception on probe.

However there's another problem with this patch. If you try and export
a non-existing GPIO via sysfs and try to read the value file you can
easily make the driver hang a CPU. This only seems to happen for the
AON GPIO controller.

The approach that I chose was to compact the range of GPIOs that the
GPIO subsystem knows about to the ones that actually exist. This has the
slight disadvantage that we can't use a simple port * 8 + offset to
compute the pin number anymore. However for the primary use-case (GPIO
specifier in DT) that's not a problem because we can translate the pin
number into the compacted space. That means the only issue will be with
sysfs support because if we use the simple formula we'll eventually get
a pin number that's outside of the range.

One way to solve this is to make a massive change to the GPIO subsystem
to check for the validity of a GPIO before any access. I'm not sure if
that's desirable, maybe Linus has some thoughts about that.

If we stick with a compacted number space, there are two solutions that
I can think of to remedy the sysfs problem. One would be to register a
separate struct gpio_chip for each controller. That's kind of a sledge-
hammer solution because it will create multiple number spaces and hence
completely avoid the sparse number space for the whole controller. I
think Stephen had originally proposed this as a solution.

The other possibility would be for the GPIO subsystem to gain per-chip
GPIO export via sysfs. That is, instead of the global export file that
you write a global GPIO number to, each per-chip directory would get
an export file. Values written into that file could get translated via
driver-specific callbacks (much like the ->xlate() callback for GPIO
specifiers). I think that's a change that makes sense anyway. Usually
users will know what GPIO controller they want to access and the offset
of the pin therein. Currently they have to somewhat jump through hoops
to get at the right pin (find controller, read GPIO base, add offset to
base and write that to the export file). The new sequence would be much
more straightforward: find controller, write offset to export file. The
new per-chip export file would be flexible enough to deal with compacted
number spaces, which is obviously something we can't do with the global
export file.

Thierry

Attachment: signature.asc
Description: PGP signature