Re: [PATCH/RFC 1/3] pinctrl: sh-pfc: support sparse GPIO numbers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thursday 14 February 2013 15:23:04 Laurent Pinchart wrote:
> On Thursday 14 February 2013 21:38:11 Paul Mundt wrote:
> > On Thu, Feb 14, 2013 at 01:09:09PM +0100, Laurent Pinchart wrote:
> > > On Thursday 14 February 2013 20:55:16 Paul Mundt wrote:
> > > > On Tue, Feb 12, 2013 at 04:50:02PM +0100, Guennadi Liakhovetski wrote:
> > > > > Not on all sh-/r-mobile platfotms all pins are numbered contiguously
> > > > > from 0 to N-1. On all ARM-based platforms datasheets use simple
> > > > > numbers to identify them, unlike some SuperH-based SoC, naming pins,
> > > > > using strings, e.g. A31:A0, B11:B0, C31:C0, etc. So far the sg-pfc
> > > > > pinctrl driver supported only contiguous pin numbering. This patch
> > > > > adds support for sparse pin numbers to support sh73a0 and any other
> > > > > similar SoCs.
> > > > > 
> > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > > > 
> > > > The sh-pfc pinctrl driver is not the place where this sort of lookup
> > > > should be happening, especially as the pinctrl subsystem already
> > > > tracks GPIO ranges for us. Is there some reason you can't just expand
> > > > the platform definition and tie in to pinctrl_add_gpio_ranges() for
> > > > supporting multiple ranges instead?
> > > 
> > > Using pinctrl_add_gpio_ranges() was also my initial idea. While I think
> > > it's still useful, there's one issue it doesn't fix: GPIO numbering in
> > > DT and sysfs. On the sh73a0 platform the GPIO_PORT* enumerated values
> > > don't match the port number. While that's not a major issue in board
> > > code that uses the enum names, this would become very confusing in DT
> > > and sysfs. GPIO 128, for instance, would be referenced by the number 119
> > > in DT.
> > 
> > If that's the case, then why are you using integers in DT instead of
> > something like a string that you can map back?
> 
> Don't ask me :-) The standard DT GPIO binding has been using integers for a
> long time, the API is pretty much set in stone.
> 
> > Granted I haven't seen the bindings documentation or proposal, so I have
> > no idea what the interface looks like, but I'm not convinced that it's
> > worthwhile adding an additional translation layer simply because the DT
> > interface can't do 1:1 mappings for whatever reason (although if necessary
> > I suppose this translation layer could always be implemented purely in
> > the OF side of the sh-pfc support, handing off sane ranges to the core at
> > registration time).
> 
> It wouldn't help for the sysfs interface though.
> 
> I've been thinking about the problem for some time now and I really don't
> know what the best solution to this problem is. Using the port number as
> specified in the datasheet is the less confusing option API-wise, but it
> brings an additional level of complexity.
> 
> gpiolib doesn't handle ranges, so I would need to either register a gpiochip
> for every range, or return an error in the request function. I don't know
> what option is prefered.
> 
> On the pinctrl side the situation is a bit easier, as pinctrl explicitly
> supports GPIO ranges.
> 
> What are the best practice rules for this kind of situation, when a GPIO
> controller handles discontiguous ranges of pins and GPIOs ?

For the sake of clarity, by discontiguous ranges of pins and GPIOs, I mean a 
single device (in the hardware IP block, struct device and DT node sense) that 
implements both pinctrl and GPIO functions. The device handles a large number 
of pins and GPIOs (called ports in the datasheet) with discontiguous numbers. 

In this particular case the device has 269 controllable pins that can all act 
as GPIOs. There are numbered 0-118, 128-164, 192-282 and 288-309.

I can't expose one gpiochip instance for each of those ranges as they would 
all share the same DT node, making of_get_named_gpio_flags() always return the 
first instance. I thus see 3 possible options:

- Number the GPIOs contiguously from 0 to 268. This will be pretty confusing 
in DT and in sysfs, as port 128 would be mapped to GPIO number 119 for 
instance.

- Register a single gpiochip for GPIOs 0-309 and return an error from the 
request() operation when requesting a non-existing GPIO.

- Add ranges support to the gpiolib API to allow a single gpiochip to allow 
sparse numbering in the whole range handled by a gpiochip.

> > > I think we should fix that and use the port number in the GPIO API. This
> > > requires translating from the port number to the GPIO entry index in
> > > internal pinmux_pins[] array. There are several way to do so, one is to
> > > index the array by the port number (and thus making it sparse, using
> > > more
> > > memory), the other is to add a list of ranges (either explicitly as in
> > > Guennadi's patch, or implicitly by adding the port number to each entry
> > > in the pinmux_pins[] array and computing the list of ranges at init
> > > time). I haven't decided yet which solution I like the best, I'm
> > > experimenting with both.
> > 
> > If this is the way things need to go, then doing it sparsely makes sense,
> > but not in the proposed form, and there's no real need for it to use
> > excessive amounts of memory. We already went through all of this once in
> > the irq_desc[] to sparseirq conversion, and there's no reason why the
> > same approach wouldn't be able to carry over for this, either.
> > 
> > At the very simplest you can simply allocate an array of pointers and
> > lazily map those back to the pin mappings, so you only waste a pointer
> > rather than a pinmux_pin struct. Alternately you can insert in to a radix
> > tree, which would permit you to stash the mapping directly, as well as
> > any additional translation logic in the node's slot (you could also
> > handle mixing and matching DT vs non-DT GPIOs by tagging the entry), and
> > eliminate the sparse array overhead entirely.

-- 
Regards,

Laurent Pinchart