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
