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

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

Hi Paul,

[CC'ing Grant]

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 ?

> > 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