Date:	Wed, 15 May 2013 11:41:48 +0200
From:	Christian Ruppert <christian.ruppert@...lis.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	Stephen Warren <swarren@...dotorg.org>,
	Patrice CHOTARD <patrice.chotard@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>,
	Sascha Leuenberger <sascha.leuenberger@...lis.com>,
	Pierrick Hascoet <pierrick.hascoet@...lis.com>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>
Subject: Re: [PATCH 1/2] pinmux: Add TB10x pinmux driver

On Tue, May 14, 2013 at 02:29:46PM +0200, Linus Walleij wrote:
> On Fri, May 10, 2013 at 10:25 AM, Christian Ruppert
> <christian.ruppert@...lis.com> wrote:
> 
> >> > What do you think about the following modification to the pinctrl/GPIO
> >> > frameworks instead (not yet a formal patch, more a request for comment
> >> > to illustrate what I mean. If you agree, I will clean it up and submit a
> >> > proper patch after discussion).
> >> >
> >> > It adds a dt_gpiorange_xlate function to the pinctrl callbacks which
> >> > defaults to the conventional behaviour using kernel logical pin numbers.
> >> > However, pin controllers which provide more complex mechanisms can
> >> > define #gpio-range-cells and provide this callback in order to keep
> >> > Linux pin numbering inside the kernel.
> (...)
> > The patch does not change the default behaviour of the kernel: In case
> > no dt_gpiorange_xlate callback is defined for a given driver (e.g. for
> > pre-existing drivers), the default function simply interprets the first
> > argument as Linux pin number and the second as pin count, same as now.
> > New drivers can use the callback to translate device specific pin
> > references to Linux pin numbers (in the idea of of_xlate in the GPIO
> > framework or xlate in the irqchip framework).
> 
> I like the concept.
> 
> However I am totally opposed to the idea of making this something
> device tree-exclusive.
> 
> Look at for example
> drivers/pinctrl/pinctrl-abx500.c:
> 
> static u8 abx500_get_mode(struct pinctrl_dev *pctldev, struct gpio_chip *chip,
>                           unsigned gpio)
> {
> (...)
>         /* on ABx5xx, there is no GPIO0, so adjust the offset */
>         unsigned offset = gpio - 1;
> 
> As you can see, this driver, which does not use device tree,
> is working around the same problem. Here the problem is that
> the pins are numbered starting at 1 instead of 0, a very trivial
> numberspace shuffleing.

Let me see if I understand this right:
In ABx5xx, the pin numbering is offset by 1 wrt. GPIO numbering?

> I'd be open to this approach if you:
> 
> - Make it generic for all pinctrl drivers, i.e. add the translation
>   to the core so it does not just apply to devices using device tree.

Do you mean what would be required here is a generic way to translate
pin numbers to GPIO numbers in the pin controller?

This translation is currently achieved by the gpio_range mechanism.
Internals of gpio_range leak into the pinctrl drivers and are bypassed
in many cases. E.g. pinctrl-abx500.c uses an internal reimplementation
of gpio ranges and the offset by one instead of the information in the
pinctrl_gpio_range structure.
Mapping pin numbers to GPIO numbers in the pin controller would have the
following advantages:
 - It makes GPIO numbering well defined which is clearly an advantage
   for the /sys/class/gpio interface: GPIO numbering is now controlled
   by the pinctrl driver author and no longer needs to be kernel
   internal.
 - The device tree/acpi issue is solved since a GPIO controller could
   now define its range in GPIO number space (which becomes public)
   rather than kernel-internal pin number space. At least for our
   products, GPIO numbering generally doesn't change between different
   variants of the same chip and the /sys/class/gpio customer
   documentation would apply to device also.
 - The custom logic inside many pinctrl drivers would be confined in one
   translation function the driver provides instead of being spread out
   all over the driver.
 - "Sparse GPIO ranges" are easy to implement if required by the
   platform/driver.

> - Augment the pinctrl-abx500.c driver to show how this simplifies
>   that driver. (Does not need to be perfect, I'll help out finalizing it
>   for sure.)

The issue is that such a change is quite fundamental, all pinctrl
drivers would have to be upgraded (not just pinctrl-abx500.c) and struct
pinctrl_gpio_range would have to be removed from the gpio_request_enable
callback and friends in favour of some generic translation mechanism.

I am also afraid that the custom logic in many drivers could only be
rewritten with the help of the respective driver's author.

> - Then add DT-specific wrapper using this core feature.

GPIO ranges definitions in DT would probably be no longer be compatible
with previous definitions since the number space would change from pin
numbers to GPIO numbers. I don't think this is acceptable.

> This way the problem will be solved for everybody, including
> ACPI when they sooner or later come back with the same issue.

Given the issues highlighted above I'm not sure if I understand your
proposal correctly. Although I see the advantages, I wonder if the
migration to such a system is feasible in practise.

Greetings,
  Christian

-- 
  Christian Ruppert              ,          <christian.ruppert@...lis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists