[PATCH] pinctrl: indicate GPIO direction on single GPIO request

[PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

When requesting a single GPIO pin to be muxed in, some controllers
will need to poke a different value into the control register
depending on whether the pin will be used for GPIO output or GPIO
input. So pass this info along for the gpio_request_enable()
function, we assume this is not needed for the gpio_free_disable()
function for the time being.

Suggested-by: Thomas Abraham <thomas.abraham@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/pinctrl/pinmux.c       |   16 +++++++++++-----
 include/linux/pinctrl/pinmux.h |   14 ++++++++++----
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index d27b77d..ef2f812 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -100,10 +100,13 @@ struct pinmux_hog {
  *	means that you want to mux in the pin for use as GPIO number NN
  * @gpio_range: the range matching the GPIO pin if this is a request for a
  *	single GPIO pin
+ * @gpio_direction: if the pin is muxed for GPIO, this provides the direction
+ *	of the GPIO @true means output, @false means input
  */
 static int pin_request(struct pinctrl_dev *pctldev,
 		       int pin, const char *function,
-		       struct pinctrl_gpio_range *gpio_range)
+		       struct pinctrl_gpio_range *gpio_range,
+		       bool gpio_direction)
 {
 	struct pin_desc *desc;
 	const struct pinmux_ops *ops = pctldev->desc->pmxops;
@@ -148,7 +151,8 @@ static int pin_request(struct pinctrl_dev *pctldev,
 	 */
 	if (gpio_range && ops->gpio_request_enable)
 		/* This requests and enables a single GPIO pin */
-		status = ops->gpio_request_enable(pctldev, gpio_range, pin);
+	  status = ops->gpio_request_enable(pctldev, gpio_range, pin,
+					    gpio_direction);
 	else if (ops->request)
 		status = ops->request(pctldev, pin);
 	else
@@ -218,8 +222,10 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
 /**
  * pinmux_request_gpio() - request a single pin to be muxed in as GPIO
  * @gpio: the GPIO pin number from the GPIO subsystem number space
+ * @direction: the direction of the GPIO, @true means output, @false
+ *	means input
  */
-int pinmux_request_gpio(unsigned gpio)
+int pinmux_request_gpio(unsigned gpio, bool direction)
 {
 	char gpiostr[16];
 	const char *function;
@@ -242,7 +248,7 @@ int pinmux_request_gpio(unsigned gpio)
 	if (!function)
 		return -EINVAL;
 
-	ret = pin_request(pctldev, pin, function, range);
+	ret = pin_request(pctldev, pin, function, range, direction);
 	if (ret < 0)
 		kfree(function);
 
@@ -360,7 +366,7 @@ static int acquire_pins(struct pinctrl_dev *pctldev,
 
 	/* Try to allocate all pins in this group, one by one */
 	for (i = 0; i < num_pins; i++) {
-		ret = pin_request(pctldev, pins[i], func, NULL);
+	  ret = pin_request(pctldev, pins[i], func, NULL, false);
 		if (ret) {
 			dev_err(&pctldev->dev,
 				"could not get pin %d for function %s "
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index bb7a979..7d3841f 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -54,7 +54,12 @@ struct pinctrl_dev;
  *	Implement this only if you can mux every pin individually as GPIO. The
  *	affected GPIO range is passed along with an offset(pin number) into that
  *	specific GPIO range - function selectors and pin groups are orthogonal
- *	to this, the core will however make sure the pins do not collide
+ *	to this, the core will however make sure the pins do not collide. Since
+ *	controllers may be needing different configurations depending on
+ *	whether the GPIO is configured as input or output, a direction
+ *	indicator is passed along
+ * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
+ *	@gpio_request_enable
  */
 struct pinmux_ops {
 	int (*request) (struct pinctrl_dev *pctldev, unsigned offset);
@@ -72,14 +77,15 @@ struct pinmux_ops {
 			 unsigned group_selector);
 	int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
 				    struct pinctrl_gpio_range *range,
-				    unsigned offset);
+				    unsigned offset,
+				    bool direction);
 	void (*gpio_disable_free) (struct pinctrl_dev *pctldev,
 				   struct pinctrl_gpio_range *range,
 				   unsigned offset);
 };
 
 /* External interface to pinmux */
-extern int pinmux_request_gpio(unsigned gpio);
+extern int pinmux_request_gpio(unsigned gpio, bool direction);
 extern void pinmux_free_gpio(unsigned gpio);
 extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *name);
 extern void pinmux_put(struct pinmux *pmx);
@@ -88,7 +94,7 @@ extern void pinmux_disable(struct pinmux *pmx);
 
 #else /* !CONFIG_PINMUX */
 
-static inline int pinmux_request_gpio(unsigned gpio)
+static inline int pinmux_request_gpio(unsigned gpio, bool direction)
 {
 	return 0;
 }
-- 
1.7.3.2

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Igor Grinberg]

Hi Linus,

On 11/14/11 11:10, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> When requesting a single GPIO pin to be muxed in, some controllers
> will need to poke a different value into the control register
> depending on whether the pin will be used for GPIO output or GPIO
> input. So pass this info along for the gpio_request_enable()
> function, we assume this is not needed for the gpio_free_disable()
> function for the time being.
> 
> Suggested-by: Thomas Abraham <thomas.abraham@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/pinctrl/pinmux.c       |   16 +++++++++++-----
>  include/linux/pinctrl/pinmux.h |   14 ++++++++++----
>  2 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index d27b77d..ef2f812 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -100,10 +100,13 @@ struct pinmux_hog {
>   *	means that you want to mux in the pin for use as GPIO number NN
>   * @gpio_range: the range matching the GPIO pin if this is a request for a
>   *	single GPIO pin
> + * @gpio_direction: if the pin is muxed for GPIO, this provides the direction
> + *	of the GPIO @true means output, @false means input

can this be gpio_direction_out (throughout the patch)?
so the true/false values will make more sense?

>   */
>  static int pin_request(struct pinctrl_dev *pctldev,
>  		       int pin, const char *function,
> -		       struct pinctrl_gpio_range *gpio_range)
> +		       struct pinctrl_gpio_range *gpio_range,
> +		       bool gpio_direction)
>  {
>  	struct pin_desc *desc;
>  	const struct pinmux_ops *ops = pctldev->desc->pmxops;
> @@ -148,7 +151,8 @@ static int pin_request(struct pinctrl_dev *pctldev,
>  	 */
>  	if (gpio_range && ops->gpio_request_enable)
>  		/* This requests and enables a single GPIO pin */
> -		status = ops->gpio_request_enable(pctldev, gpio_range, pin);
> +	  status = ops->gpio_request_enable(pctldev, gpio_range, pin,
> +					    gpio_direction);
>  	else if (ops->request)
>  		status = ops->request(pctldev, pin);
>  	else
> @@ -218,8 +222,10 @@ static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
>  /**
>   * pinmux_request_gpio() - request a single pin to be muxed in as GPIO
>   * @gpio: the GPIO pin number from the GPIO subsystem number space
> + * @direction: the direction of the GPIO, @true means output, @false
> + *	means input
>   */
> -int pinmux_request_gpio(unsigned gpio)
> +int pinmux_request_gpio(unsigned gpio, bool direction)
>  {
>  	char gpiostr[16];
>  	const char *function;
> @@ -242,7 +248,7 @@ int pinmux_request_gpio(unsigned gpio)
>  	if (!function)
>  		return -EINVAL;
>  
> -	ret = pin_request(pctldev, pin, function, range);
> +	ret = pin_request(pctldev, pin, function, range, direction);
>  	if (ret < 0)
>  		kfree(function);
>  
> @@ -360,7 +366,7 @@ static int acquire_pins(struct pinctrl_dev *pctldev,
>  
>  	/* Try to allocate all pins in this group, one by one */
>  	for (i = 0; i < num_pins; i++) {
> -		ret = pin_request(pctldev, pins[i], func, NULL);
> +	  ret = pin_request(pctldev, pins[i], func, NULL, false);
>  		if (ret) {
>  			dev_err(&pctldev->dev,
>  				"could not get pin %d for function %s "
> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
> index bb7a979..7d3841f 100644
> --- a/include/linux/pinctrl/pinmux.h
> +++ b/include/linux/pinctrl/pinmux.h
> @@ -54,7 +54,12 @@ struct pinctrl_dev;
>   *	Implement this only if you can mux every pin individually as GPIO. The
>   *	affected GPIO range is passed along with an offset(pin number) into that
>   *	specific GPIO range - function selectors and pin groups are orthogonal
> - *	to this, the core will however make sure the pins do not collide
> + *	to this, the core will however make sure the pins do not collide. Since
> + *	controllers may be needing different configurations depending on
> + *	whether the GPIO is configured as input or output, a direction
> + *	indicator is passed along
> + * @gpio_disable_free: free up GPIO muxing on a certain pin, the reverse of
> + *	@gpio_request_enable
>   */
>  struct pinmux_ops {
>  	int (*request) (struct pinctrl_dev *pctldev, unsigned offset);
> @@ -72,14 +77,15 @@ struct pinmux_ops {
>  			 unsigned group_selector);
>  	int (*gpio_request_enable) (struct pinctrl_dev *pctldev,
>  				    struct pinctrl_gpio_range *range,
> -				    unsigned offset);
> +				    unsigned offset,
> +				    bool direction);
>  	void (*gpio_disable_free) (struct pinctrl_dev *pctldev,
>  				   struct pinctrl_gpio_range *range,
>  				   unsigned offset);
>  };
>  
>  /* External interface to pinmux */
> -extern int pinmux_request_gpio(unsigned gpio);
> +extern int pinmux_request_gpio(unsigned gpio, bool direction);
>  extern void pinmux_free_gpio(unsigned gpio);
>  extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *name);
>  extern void pinmux_put(struct pinmux *pmx);
> @@ -88,7 +94,7 @@ extern void pinmux_disable(struct pinmux *pmx);
>  
>  #else /* !CONFIG_PINMUX */
>  
> -static inline int pinmux_request_gpio(unsigned gpio)
> +static inline int pinmux_request_gpio(unsigned gpio, bool direction)
>  {
>  	return 0;
>  }

-- 
Regards,
Igor.

RE: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Stephen Warren]

Linus Walleij wrote at Monday, November 14, 2011 2:11 AM:
> When requesting a single GPIO pin to be muxed in, some controllers
> will need to poke a different value into the control register
> depending on whether the pin will be used for GPIO output or GPIO
> input. So pass this info along for the gpio_request_enable()
> function, we assume this is not needed for the gpio_free_disable()
> function for the time being.

I'm not sure this API change makes sense.

Functions gpio_direction_{input,output} already exist to configure the
direction of a GPIO, and drivers should already be using them. These have
to work to allow drivers to toggle the direction dynamically. Requiring
them to additionally pass this same information to the pinmux driver when
setting up the pinmux seems like extra redundant work.

Instead, shouldn't it work like this:

* If the pinmux driver implementation behind pinmux_request_gpio() needs
to know the direction when configuring the HW, default to input for safety;
that will prevent the SoC driving a signal on a GPIO that's driven by some
other device.

* Rely exclusively on gpio_direction_{input,output} to allow drivers to
configure the direction.

* If the pinmux HW needs programming in response to the gpio_direction_*
calls, have the GPIO and pinmux driver internally communicate to achieve
this.

Does that seem reasonable?

-- 
nvpublic

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Thomas Abraham]

On 14 November 2011 22:48, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Monday, November 14, 2011 2:11 AM:
>> When requesting a single GPIO pin to be muxed in, some controllers
>> will need to poke a different value into the control register
>> depending on whether the pin will be used for GPIO output or GPIO
>> input. So pass this info along for the gpio_request_enable()
>> function, we assume this is not needed for the gpio_free_disable()
>> function for the time being.
>
> I'm not sure this API change makes sense.
>
> Functions gpio_direction_{input,output} already exist to configure the
> direction of a GPIO, and drivers should already be using them. These have
> to work to allow drivers to toggle the direction dynamically. Requiring
> them to additionally pass this same information to the pinmux driver when
> setting up the pinmux seems like extra redundant work.

Hardware that require pinmux controller also to be programmed for
setting the gpio direction would require this. Yes, there does seem to
be redundancy if the driver has to call both
gpio_direction_{input,output} and pinmux_request_gpio (with the new
direction parameter). Also, there seems to be a redundancy if the
driver calls both gpio_request and pinmux_request_gpio.

>
> Instead, shouldn't it work like this:
>
> * If the pinmux driver implementation behind pinmux_request_gpio() needs
> to know the direction when configuring the HW, default to input for safety;
> that will prevent the SoC driving a signal on a GPIO that's driven by some
> other device.
>
> * Rely exclusively on gpio_direction_{input,output} to allow drivers to
> configure the direction.
>
> * If the pinmux HW needs programming in response to the gpio_direction_*
> calls, have the GPIO and pinmux driver internally communicate to achieve
> this.

If gpio_direction_* does have to setup the pinmux, then it has to be
done using one the external interfaces which the pinctrl/pinmux
subsystem provides. That has been pinmux_request_gpio for now. If
adding 'direction' to pinmux_request_gpio is redundant, then there
should be another external interface provided by pinmux subsystem for
gpio_direction_{input,output} to setup the direction.

Thanks,
Thomas.

>
> Does that seem reasonable?
>
> --
> nvpublic
>
>

RE: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Stephen Warren]

Thomas Abraham wrote at Monday, November 14, 2011 11:00 AM:
> On 14 November 2011 22:48, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Monday, November 14, 2011 2:11 AM:
> >> When requesting a single GPIO pin to be muxed in, some controllers
> >> will need to poke a different value into the control register
> >> depending on whether the pin will be used for GPIO output or GPIO
> >> input. So pass this info along for the gpio_request_enable()
> >> function, we assume this is not needed for the gpio_free_disable()
> >> function for the time being.
> >
> > I'm not sure this API change makes sense.
> >
> > Functions gpio_direction_{input,output} already exist to configure the
> > direction of a GPIO, and drivers should already be using them. These have
> > to work to allow drivers to toggle the direction dynamically. Requiring
> > them to additionally pass this same information to the pinmux driver when
> > setting up the pinmux seems like extra redundant work.
> 
> Hardware that require pinmux controller also to be programmed for
> setting the gpio direction would require this. Yes, there does seem to
> be redundancy if the driver has to call both
> gpio_direction_{input,output} and pinmux_request_gpio (with the new
> direction parameter). Also, there seems to be a redundancy if the
> driver calls both gpio_request and pinmux_request_gpio.

Having all drivers call two APIs every time a GPIO needs to be configured
seems to be a Bad Thing. Perhaps we can get away with just gpio_request();
see below.

Igor Grinberg wrote at Tuesday, November 15, 2011 12:16 AM:
> On 11/14/11 19:18, Stephen Warren wrote:
...
> > Instead, shouldn't it work like this:
> >
> > * If the pinmux driver implementation behind pinmux_request_gpio() needs
> > to know the direction when configuring the HW, default to input for safety;
> > that will prevent the SoC driving a signal on a GPIO that's driven by some
> > other device.
> 
> If the GPIO has been configured for output by boot loader
> and drives a value, and now you want Linux to take control over it,
> then configuring it for input will not be safe at all.
> I think this kind of flexibility is necessary (although it can be
> implemented in different ways).

That's true; it would be better to get to the final desired state in a
single explicit step.

So I think the solution for this is for gpio_request() to be redefined
to affect the pinmux where required.

Now I know that Documentation/gpio.txt explicitly says that gpio_request()
doesn't touch the pinmux... Does anyone know the history there; perhaps we
can revisit that.

In particular, perhaps on systems that need the pinmux programmed for
GPIO direction, pinmux_request_gpio() will just reserve the pin, and be
a no-op as far as HW is convered, whereas gpio_request_*() will call
into pinmux by a back-door to do the actual pinmux configuration?

Similarly on Tegra, to select a pin as a GPIO, you actually write a bit
to a GPIO register that overrides the pinmux function selection. Right
now, I have the pinmux driver calling into the GPIO driver to do this.
I could remove this backdoor call, and rely solely on gpio_request_*()
for this.

I suppose there's still a problem on HW that can mux multiple GPIOs onto
a single pin, /and/ needs to distinguish GPIO in vs. out while doing so.
I guess this could be handled by having pinmux_request_gpio do:

If pin is already a GPIO:
    Leave it untouched
Else:
    Configure pin pin to be a GPIO on desired controller,
    picking in/out as appropriate for HW to avoid glitches

gpio_request_*() would still call into the pinmux driver by a back door
to select the final desired IO direction.

It'd be nice to define gpio_request() as always calling pinmux_gpio_request(),
but that won't work on systems where there isn't a 1:1 mapping between
pins and GPIOs: multiple controllers, or GPIOs can routed to different
pins.

-- 
nvpublic

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Linus Walleij]

On Tue, Nov 15, 2011 at 8:04 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Thomas Abraham wrote at Monday, November 14, 2011 11:00 AM:
>> On 14 November 2011 22:48, Stephen Warren <swarren@nvidia.com> wrote:
>> > Linus Walleij wrote at Monday, November 14, 2011 2:11 AM:
>> >> When requesting a single GPIO pin to be muxed in, some controllers
>> >> will need to poke a different value into the control register
>> >> depending on whether the pin will be used for GPIO output or GPIO
>> >> input. So pass this info along for the gpio_request_enable()
>> >> function, we assume this is not needed for the gpio_free_disable()
>> >> function for the time being.
>> >
>> > I'm not sure this API change makes sense.
>> >
>> > Functions gpio_direction_{input,output} already exist to configure the
>> > direction of a GPIO, and drivers should already be using them. These have
>> > to work to allow drivers to toggle the direction dynamically. Requiring
>> > them to additionally pass this same information to the pinmux driver when
>> > setting up the pinmux seems like extra redundant work.
>>
>> Hardware that require pinmux controller also to be programmed for
>> setting the gpio direction would require this. Yes, there does seem to
>> be redundancy if the driver has to call both
>> gpio_direction_{input,output} and pinmux_request_gpio (with the new
>> direction parameter). Also, there seems to be a redundancy if the
>> driver calls both gpio_request and pinmux_request_gpio.
>
> Having all drivers call two APIs every time a GPIO needs to be configured
> seems to be a Bad Thing. Perhaps we can get away with just gpio_request();
> see below.

Having all drivers call it is definately not the intention. The way
I imagine it (and have also implemented it in to-be-sent patches)
is that

gpio_request(N, "foo);
-> GPIOlib driver .request()
   calls pinmux_request_gpio()
   -> pinmux driver .gpio_request_enable() if available
   -> pinmux driver .request() if available

Here the last two callbacks may need to know the direction.

If we actively want to stop non-GPIO drivers from using this, shall
we aim for a compromise to keep the pinmux_request_gpio() call in
the local header in the pinctrl subsystem, i.e

drivers/pinctrl/pinmux.h

Then it can *only* be used by GPIO drivers migrated to the
pinctrl subsystem, and these probably know what they're doing.

> Igor Grinberg wrote at Tuesday, November 15, 2011 12:16 AM:
>> On 11/14/11 19:18, Stephen Warren wrote:
> ...
>> > Instead, shouldn't it work like this:
>> >
>> > * If the pinmux driver implementation behind pinmux_request_gpio() needs
>> > to know the direction when configuring the HW, default to input for safety;
>> > that will prevent the SoC driving a signal on a GPIO that's driven by some
>> > other device.
>>
>> If the GPIO has been configured for output by boot loader
>> and drives a value, and now you want Linux to take control over it,
>> then configuring it for input will not be safe at all.
>> I think this kind of flexibility is necessary (although it can be
>> implemented in different ways).
>
> That's true; it would be better to get to the final desired state in a
> single explicit step.
>
> So I think the solution for this is for gpio_request() to be redefined
> to affect the pinmux where required.

That was my idea all along, and the intention of that API
pinmux_request_gpio() is for GPIO drivers to use it to set up the
muxing. Maybe I need to think about how to make that clear :-/

> Now I know that Documentation/gpio.txt explicitly says that gpio_request()
> doesn't touch the pinmux... Does anyone know the history there; perhaps we
> can revisit that.

The person who knew what that meant was David Brownell, rest in
peace old friend, now we need to figure this out on our own :-(

My best guess is that this  means "please don't put pinmux drivers into
drivers/gpio/*" which is anyway false since some of them already
do that.

GPIO drivers in drivers/pinctrl is another thing.

> In particular, perhaps on systems that need the pinmux programmed for
> GPIO direction, pinmux_request_gpio() will just reserve the pin, and be
> a no-op as far as HW is convered, whereas gpio_request_*() will call
> into pinmux by a back-door to do the actual pinmux configuration?

IMO it's better to make the pinmux_request_gpio() a backend for GPIO
drivers and GPIO drivers only.

I'll fix some patch.

> It'd be nice to define gpio_request() as always calling pinmux_gpio_request(),

That is what we should do.

> but that won't work on systems where there isn't a 1:1 mapping between
> pins and GPIOs: multiple controllers, or GPIOs can routed to different
> pins.

I think we can handle this (I might be deluded as usual...) since
we have the GPIO range definitions to rely upon. These can be
runtime-assigned.

Yours,
Linus Walleij

RE: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Stephen Warren]

Linus Walleij wrote at Thursday, November 17, 2011 3:04 AM:
> On Tue, Nov 15, 2011 at 8:04 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Thomas Abraham wrote at Monday, November 14, 2011 11:00 AM:
> >> On 14 November 2011 22:48, Stephen Warren <swarren@nvidia.com> wrote:
> >> > Linus Walleij wrote at Monday, November 14, 2011 2:11 AM:
> >> >> When requesting a single GPIO pin to be muxed in, some controllers
> >> >> will need to poke a different value into the control register
> >> >> depending on whether the pin will be used for GPIO output or GPIO
> >> >> input. So pass this info along for the gpio_request_enable()
> >> >> function, we assume this is not needed for the gpio_free_disable()
> >> >> function for the time being.
> >> >
> >> > I'm not sure this API change makes sense.
> >> >
> >> > Functions gpio_direction_{input,output} already exist to configure the
> >> > direction of a GPIO, and drivers should already be using them. These have
> >> > to work to allow drivers to toggle the direction dynamically. Requiring
> >> > them to additionally pass this same information to the pinmux driver when
> >> > setting up the pinmux seems like extra redundant work.
> >>
> >> Hardware that require pinmux controller also to be programmed for
> >> setting the gpio direction would require this. Yes, there does seem to
> >> be redundancy if the driver has to call both
> >> gpio_direction_{input,output} and pinmux_request_gpio (with the new
> >> direction parameter). Also, there seems to be a redundancy if the
> >> driver calls both gpio_request and pinmux_request_gpio.
> >
> > Having all drivers call two APIs every time a GPIO needs to be configured
> > seems to be a Bad Thing. Perhaps we can get away with just gpio_request();
> > see below.
> 
> Having all drivers call it is definately not the intention. The way
> I imagine it (and have also implemented it in to-be-sent patches)
> is that
> 
> gpio_request(N, "foo);
> -> GPIOlib driver .request()
>    calls pinmux_request_gpio()
>    -> pinmux driver .gpio_request_enable() if available
>    -> pinmux driver .request() if available
> 
> Here the last two callbacks may need to know the direction.

OK, I'm fine with that concept.

However:

a) gpio_request() doesn't know the direction; only gpio_direction_*()
know that. I suggest the pinctrl API have the same set of GPIO-related
functions that gpiolib has, so each can be passed through to a pinctrl
function 1:1:

gpio_request -> pinmux_gpio_request
gpio_direction_input -> pinmux_gpio_direction_input
gpio_direction_output -> pinmux_gpio_direction_output
gpio_free -> pinmux_gpio_free

(and a pinmux driver ops entry for each too)

Rationale:

Once gpio_request returns success, the pin must be reserved for GPIO,
hence some pinmux function must be called to reserve it, and prevent
any special function from using that pin. The pinmux interaction can't
be deferred until gpio_direction_*, since those shouldn't fail due to
reservation issues.

However, gpio_request doesn't know the IO direction, so if the HW needs
to know this, it can only be programmed in response to gpio_direction_*,
hence there need to be pinmux function matching those APIs too.

For some HW, the HW programming can also happen in gpio_request, if
the pinmux HW doesn't need to know the GPIO direction, as is the case on
Tegra for example.

b) GPIO ID isn't enough for the pinmux driver to work. It covers the
following cases:

b1) A single GPIO controller, where each GPIO can only be routed to a
single pin.

b2) Multiple GPIO controllers, where each GPIO can only be routed to a
single pin; even where multiple GPIO controllers can be routed to the
same pin, the GPIO ID implies the controller, and hence gpio_request can
know which mux option to program in order to choose the correct controller.

However, it doesn't cover the following case:

b3) Some GPIO could be routed to multiple pins, just like any other pinmux
function. In this case, knowing the GPIO ID isn't enough to program the
pinmux, but you need to know both "this GPIO" (which we have) and "this
pin" (which is missing).

I'm not sure how to solve that. For such SoCs, perhaps we should treat
this as a muxing setup, and require it to be in the mapping table, and
consider all the pinmux_gpio_* functions to be "enable GPIO access to
pins" rather than "set up pin mux for GPIOs"?

> If we actively want to stop non-GPIO drivers from using this, shall
> we aim for a compromise to keep the pinmux_request_gpio() call in
> the local header in the pinctrl subsystem, i.e
> 
> drivers/pinctrl/pinmux.h
> 
> Then it can *only* be used by GPIO drivers migrated to the
> pinctrl subsystem, and these probably know what they're doing.

Tegra's GPIO driver has no reason to move into drivers/pinctrl; the HW is
entirely separate from pinmux. I'm sure many other SoCs are similar. As
such, moving the prototype like that would be unhelpful.

-- 
nvpublic

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Linus Walleij]

On Thu, Nov 17, 2011 at 6:15 PM, Stephen Warren <swarren@nvidia.com> wrote:

> a) gpio_request() doesn't know the direction; only gpio_direction_*()
> know that. I suggest the pinctrl API have the same set of GPIO-related
> functions that gpiolib has, so each can be passed through to a pinctrl
> function 1:1:
>
> gpio_request -> pinmux_gpio_request
> gpio_direction_input -> pinmux_gpio_direction_input
> gpio_direction_output -> pinmux_gpio_direction_output
> gpio_free -> pinmux_gpio_free
>
> (and a pinmux driver ops entry for each too)

You are right, the current solution is messy, and this is way more
elegant and to the point. I'll fix up the patch set and resubmit it.

> However, it doesn't cover the following case:
>
> b3) Some GPIO could be routed to multiple pins, just like any other pinmux
> function. In this case, knowing the GPIO ID isn't enough to program the
> pinmux, but you need to know both "this GPIO" (which we have) and "this
> pin" (which is missing).
>
> I'm not sure how to solve that. For such SoCs, perhaps we should treat
> this as a muxing setup, and require it to be in the mapping table, and
> consider all the pinmux_gpio_* functions to be "enable GPIO access to
> pins" rather than "set up pin mux for GPIOs"?

I think we'll survive that, since the gpio range concept is supposed to
make the final resolution of that at runtime.

Now ranges are dynamic, so if these pin allocation also want to
*change* at runtime it will be hairy, but that only reflects the actual
trouble in doing such re-arrangements I think...

Changing pin allocation at runtime seems dangerous but if you
only have a few limited GPIO blocks but many GPIO pins it could
happen, but I think we can deal with that the day it surfaces.

>> If we actively want to stop non-GPIO drivers from using this, shall
>> we aim for a compromise to keep the pinmux_request_gpio() call in
>> the local header in the pinctrl subsystem, i.e
>>
>> drivers/pinctrl/pinmux.h
>>
>> Then it can *only* be used by GPIO drivers migrated to the
>> pinctrl subsystem, and these probably know what they're doing.
>
> Tegra's GPIO driver has no reason to move into drivers/pinctrl; the HW is
> entirely separate from pinmux. I'm sure many other SoCs are similar. As
> such, moving the prototype like that would be unhelpful.

OK it never seemed like a good idea anyway, I'm keeping in in
<pinctrl/pinmux.h>

Yours,
Linus Walleij

RE: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Stephen Warren]

Linus Walleij wrote at Monday, November 21, 2011 3:47 AM:
> On Thu, Nov 17, 2011 at 6:15 PM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > a) gpio_request() doesn't know the direction; only gpio_direction_*()
> > know that. I suggest the pinctrl API have the same set of GPIO-related
> > functions that gpiolib has, so each can be passed through to a pinctrl
> > function 1:1:
> >
> > gpio_request -> pinmux_gpio_request
> > gpio_direction_input -> pinmux_gpio_direction_input
> > gpio_direction_output -> pinmux_gpio_direction_output
> > gpio_free -> pinmux_gpio_free
> >
> > (and a pinmux driver ops entry for each too)
> 
> You are right, the current solution is messy, and this is way more
> elegant and to the point. I'll fix up the patch set and resubmit it.
> 
> > However, it doesn't cover the following case:
> >
> > b3) Some GPIO could be routed to multiple pins, just like any other pinmux
> > function. In this case, knowing the GPIO ID isn't enough to program the
> > pinmux, but you need to know both "this GPIO" (which we have) and "this
> > pin" (which is missing).
> >
> > I'm not sure how to solve that. For such SoCs, perhaps we should treat
> > this as a muxing setup, and require it to be in the mapping table, and
> > consider all the pinmux_gpio_* functions to be "enable GPIO access to
> > pins" rather than "set up pin mux for GPIOs"?
> 
> I think we'll survive that, since the gpio range concept is supposed to
> make the final resolution of that at runtime.
> 
> Now ranges are dynamic, so if these pin allocation also want to
> *change* at runtime it will be hairy, but that only reflects the actual
> trouble in doing such re-arrangements I think...

Doing this with dynamic GPIO ranges sounds complex; every time the mux
setting gets changed for a pin which can be a GPIO, you'd have to adjust
the dynamic ranges; this sounds like a lot of work.

I thought I'd sent this on Friday, but perhaps I only thought it up over
the weekend... Here's my proposal:

1)

Add a new gpiolib API:

int gpio_request_at(unsigned gpio, const char *label, unsigned at);

"at" is the "pinmux location for this GPIO". The interpretation of this
value is defined by each SoC just like GPIO IDs and pinctrl pin numbers
are.

On SoCs where a GPIO could be routed to n different pinctrl pins, the
caller can provide a specific location ("at") to mux it onto. The muxing
would happen during the gpiolib to pinctrl gpio_request() or
gpio_direction_*() call.

The existing gpio_request() would just call gpio_request() with at==0.

Now, most SoCs don't support this, so would validate that at==0 in their
implementation.

Equally, since this feature probably isn't used today, most drivers can
simply keep calling plain old gpio_request().

If/when a SoC needs this functionality, the relevant drivers will need to
be updated to call gpio_request_at() instead of plain gpio_request(). The
"at" parameter should come in through platform data (or device-tree) in
exactly the same way as the existing GPIO ID does; the driver certainly
should not be defining the "at" value itself. In device-tree, this can be
handles by increasing "#gpio-cells" for the GPIO controller, think.

2)

Instead of pinctrl drivers providing a GPIO<->pin table to the pinctrl
core, and having to dynamically adjust this every time a GPIO's muxing
changes, I propose completely removing GPIO range support from pinctrl.

Instead, a pinmux driver should implement a new pinmux_op, e.g.:

int (*gpio_get_pin)(unsigned gpio, unsigned at, unsigned *pin)

At least Tegra will just implement this as:

int foo_gpio_get_pin(unsigned gpio, unsigned at, unsigned *pin)
{
    if (at != 0)
        return -EINVAL;
    if (gpio >= NUM_SOC_GPIOS)
        return -EINVAL;
    /*
     * GPIO IDs have a 1:1 mapping with pinctrl pins, and GPIO 0 == pin 0.
     * Other drivers may need to implement a range lookup here.
     */
    return gpio;
}

A pinctrl driver with a non 1:1 or offset mapping of GPIO ID to pin
number might need slightly more complexity:

    return gpio + 128; // 128 == base pin ID of first GPIO

or:

    if (gpio < 32)
        return gpio + 16;
    elif (gpio < 64)
        return gpio + 64;
    else;
        return gpio + 128;

A pinctrl driver that supports GPIOs being muxed to different locations
might have to look up the combination of gpio,at in some table.

The advantage here is that it removes a lot of core code from pinctrl
that deals with the range tables, especially if we need to start making
them dynamic. The replacement op that pinmux drivers need to implement
is likely to be trivial in most cases; perhaps even simpler than
providing a gpio range definition to the core as it must right now. And
any complexity caused by SoCs that can map a GPIO to different pins is
basically isolated to that SoC's pinctrl drivers' gpio_get_pin API, and
any drivers than run on the SoC, which only need to pass one extra piece
of data to gpiolib.

Finally, when pinctrl receives any of the pinmux_gpio_* calls, it will
simply do:

int pinmux_request_gpio(unsigned gpio, unsigned at)
{
	struct pinctrl_dev *pctldev;
	const struct pinmux_ops *ops;
	int ret;
	int pin;

	ret = pinctrl_get_gpio_dev(gpio, &pctldev);
	if (ret < 0)
		return ret;
	ops = pctldev->desc->pmxops;

	ret = ops->gpio_get_pin(gpio, at, &pin);
	if (ret < 0)
		return ret;

	/* Calls ops->gpio_request internally */
	ret = pin_request(pctldev, pin, at, "gpio%d");
	if (ret < 0)
		return ret;

      /*
	 * We'd need to cache a GPIO->pin,at mapping here to allow
	 * pinmux_gpio_direction() to call gpio_get_pin. Perhaps we
	 * could have gpiolib store the mapping, per GPIO and pass
	 * it back to other functions?
	 */
	return 0;
}

Now, you do need to write a pinctrl_get_gpio_dev() function, so I suppose
that pinctrl drivers will need to tell the pinctrl core their min/max
GPIO IDs, and I suppose there could be many ranges, so we get back to
registering ranges. Ick.

(In a system with multiple pinctrl drivers, you can't just grab "the"
pinctrl driver here, because there's more than one, and different gpiolib
GPIO ranges will probably map to different pinctrl drivers)

Perhaps we could get some help from gpiolib; for each GPIO chip that
gets registered, can gpiolib store the pctldev and pass it into the
pinctrl driver:

/* Or some equivalent first parameter */
int pinmux_request_gpio(struct pinctrl_dev *pctldev, unsigned gpio, unsigned at);

pinctrl driver probe would probably need to do something like:

pinctrl_set_gpio_driver(gpio, numgpios, <driver handle>)

which internally would do nothing but call:

gpiolib_set_gpiochip_pinctrl_driver(f(<driver handle>), gpio)

where f(<driver handle>) is whatever gets passed as the first parameter to
pinmux_request_gpio()?

-- 
nvpublic

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Linus Walleij]

On Mon, Nov 21, 2011 at 8:00 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Monday, November 21, 2011 3:47 AM:
>> [Stephen]
>> > However, it doesn't cover the following case:
>> >
>> > b3) Some GPIO could be routed to multiple pins, just like any other pinmux
>> > function. In this case, knowing the GPIO ID isn't enough to program the
>> > pinmux, but you need to know both "this GPIO" (which we have) and "this
>> > pin" (which is missing).
>> >
>> > I'm not sure how to solve that. For such SoCs, perhaps we should treat
>> > this as a muxing setup, and require it to be in the mapping table, and
>> > consider all the pinmux_gpio_* functions to be "enable GPIO access to
>> > pins" rather than "set up pin mux for GPIOs"?
>>
>> I think we'll survive that, since the gpio range concept is supposed to
>> make the final resolution of that at runtime.
>>
>> Now ranges are dynamic, so if these pin allocation also want to
>> *change* at runtime it will be hairy, but that only reflects the actual
>> trouble in doing such re-arrangements I think...
>
> Doing this with dynamic GPIO ranges sounds complex; every time the mux
> setting gets changed for a pin which can be a GPIO, you'd have to adjust
> the dynamic ranges; this sounds like a lot of work.
>
> I thought I'd sent this on Friday, but perhaps I only thought it up over
> the weekend... Here's my proposal:
>
> 1)
>
> Add a new gpiolib API:
>
> int gpio_request_at(unsigned gpio, const char *label, unsigned at);

This has a nice look to it, I must admit.

Since the pinctrl number space is per-controller this signature
will not work. It would have to pass in a pin controller reference
too, preferably as struct device * or a string. Else "at" is just
a number without meaning. "At *which* controller?"

And I think you notice below, and then suggest how to modify
gpiolib to hold a reference to its pinctrl (which is viable).

Still is looks like hm, "at" well you're anyway encoding knowledge
about which pin you want to use you definately know which
pin controller you're dealing with anyway so then you can just
pass both pieces of information:

int gpio_request_at(unsigned gpio, const char *label, const char
*pinctrl_name, unsigned at);

as in:

foo = gpio_request_at(17, "my GPIO", "pinctrl.0", 14);

Would solve that since you can look up a controller by the name
string.

> Now, you do need to write a pinctrl_get_gpio_dev() function, so I suppose
> that pinctrl drivers will need to tell the pinctrl core their min/max
> GPIO IDs, and I suppose there could be many ranges, so we get back to
> registering ranges. Ick.
>
> (In a system with multiple pinctrl drivers, you can't just grab "the"
> pinctrl driver here, because there's more than one, and different gpiolib
> GPIO ranges will probably map to different pinctrl drivers)
>
> Perhaps we could get some help from gpiolib; for each GPIO chip that
> gets registered, can gpiolib store the pctldev and pass it into the
> pinctrl driver:
>
> /* Or some equivalent first parameter */
> int pinmux_request_gpio(struct pinctrl_dev *pctldev, unsigned gpio, unsigned at);
>
> pinctrl driver probe would probably need to do something like:
>
> pinctrl_set_gpio_driver(gpio, numgpios, <driver handle>)
>
> which internally would do nothing but call:
>
> gpiolib_set_gpiochip_pinctrl_driver(f(<driver handle>), gpio)
>
> where f(<driver handle>) is whatever gets passed as the first parameter to
> pinmux_request_gpio()?

Yes letting gpio_chip hold a reference to the pin controller
is equivalent to the pin controller GPIO range holding a
reference to the struct gpio_chip * as it can do today.

Any solution like the above needs to be anchored with the
GPIO maintainer anyway.

My past attempts to patch gpio_chip have been shot down,
pinctrl was created to get away from having to introduce any kind of
control or muxing in gpiolib drivers. The current ranges are
totally encapsulated in the pinctrl subsystem and that's a pretty
nice feature I suspect.

Yours,
Linus Walleij

RE: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Stephen Warren]

Linus Walleij wrote at Monday, November 21, 2011 2:44 PM:
> On Mon, Nov 21, 2011 at 8:00 PM, Stephen Warren <swarren@nvidia.com> wrote:
...
> > I thought I'd sent this on Friday, but perhaps I only thought it up over
> > the weekend... Here's my proposal:
> >
> > 1)
> >
> > Add a new gpiolib API:
> >
> > int gpio_request_at(unsigned gpio, const char *label, unsigned at);
> 
> This has a nice look to it, I must admit.
> 
> Since the pinctrl number space is per-controller this signature
> will not work. It would have to pass in a pin controller reference
> too, preferably as struct device * or a string. Else "at" is just
> a number without meaning. "At *which* controller?"

I'm assuming that each (physical) chip will have one pinctrl driver.
Therefore, for any GPIO driver (and hence any GPIO number) on that chip,
there is a single pinctrl driver that's relevant, and hence pinctrl
driver can be determined directly and internally from the GPIO ID.

Now that I say it though, I wonder if that assumption is totally valid;
after all, we're talking about having multiple GPIO controller drivers
within a (physical) chip, so perhaps having multiple pinctrl drivers
for different subsets of the pins wouldn't be impossible, albeit pretty
unlikely and bizarre. Should we just ignore that case? We could deal
with it by internally creating an aggregate driver if it ever happened.

...
> Yes letting gpio_chip hold a reference to the pin controller
> is equivalent to the pin controller GPIO range holding a
> reference to the struct gpio_chip * as it can do today.

Yes, those are pretty much the same thing.

Assuming my assumption above is correct, then each GPIO (and GPIO chip)
maps to a single pinctrl, which makes storing the data easy and static,
but each pinctrl range might map to different GPIO chips and IDs which
makes things harder, so I still see some value pushing the data storage
into GPIO.

> Any solution like the above needs to be anchored with the
> GPIO maintainer anyway.
> 
> My past attempts to patch gpio_chip have been shot down,
> pinctrl was created to get away from having to introduce any kind of
> control or muxing in gpiolib drivers. The current ranges are
> totally encapsulated in the pinctrl subsystem and that's a pretty
> nice feature I suspect.

Yes, that's all true.

-- 
nvpublic

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Linus Walleij]

On Mon, Nov 21, 2011 at 10:54 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Monday, November 21, 2011 2:44 PM:
>> On Mon, Nov 21, 2011 at 8:00 PM, Stephen Warren <swarren@nvidia.com> wrote:
>> >
>> > int gpio_request_at(unsigned gpio, const char *label, unsigned at);
>>
>> This has a nice look to it, I must admit.
>>
>> Since the pinctrl number space is per-controller this signature
>> will not work. It would have to pass in a pin controller reference
>> too, preferably as struct device * or a string. Else "at" is just
>> a number without meaning. "At *which* controller?"
>
> I'm assuming that each (physical) chip will have one pinctrl driver.
> Therefore, for any GPIO driver (and hence any GPIO number) on that chip,
> there is a single pinctrl driver that's relevant, and hence pinctrl
> driver can be determined directly and internally from the GPIO ID.

Yep that's the assumption behind the current range concept
as well. (And the ranges are used to look up pin controllers for
a certain GPIO.)

> Now that I say it though, I wonder if that assumption is totally valid;
> after all, we're talking about having multiple GPIO controller drivers
> within a (physical) chip, so perhaps having multiple pinctrl drivers
> for different subsets of the pins wouldn't be impossible, albeit pretty
> unlikely and bizarre. Should we just ignore that case?

No but we could postpone dealing with it until there is a
piece of hardware that needs the feature. :-)

> We could deal
> with it by internally creating an aggregate driver if it ever happened.

That's one way, the agregate driver will be more beautiful
if we spread it out across a few separate files in that case.
But let's take that when it hits us.

>> Yes letting gpio_chip hold a reference to the pin controller
>> is equivalent to the pin controller GPIO range holding a
>> reference to the struct gpio_chip * as it can do today.
>
> Yes, those are pretty much the same thing.
>
> Assuming my assumption above is correct, then each GPIO (and GPIO chip)
> maps to a single pinctrl, which makes storing the data easy and static,
> but each pinctrl range might map to different GPIO chips and IDs which
> makes things harder, so I still see some value pushing the data storage
> into GPIO.

I think it's possible to make a patch for that and refactor
the current range concept into that pretty cleanly while
moving along with the current codebase. It would take some time
though, not due to writing code but due to unavoidable discussions
about gpiolib, since people are full of opinions regarding that...

Yours,
Linus Walleij

Re: [PATCH] pinctrl: indicate GPIO direction on single GPIO request

[Igor Grinberg]

On 11/14/11 19:18, Stephen Warren wrote:
> Linus Walleij wrote at Monday, November 14, 2011 2:11 AM:
>> When requesting a single GPIO pin to be muxed in, some controllers
>> will need to poke a different value into the control register
>> depending on whether the pin will be used for GPIO output or GPIO
>> input. So pass this info along for the gpio_request_enable()
>> function, we assume this is not needed for the gpio_free_disable()
>> function for the time being.
> 
> I'm not sure this API change makes sense.
> 
> Functions gpio_direction_{input,output} already exist to configure the
> direction of a GPIO, and drivers should already be using them. These have
> to work to allow drivers to toggle the direction dynamically. Requiring
> them to additionally pass this same information to the pinmux driver when
> setting up the pinmux seems like extra redundant work.
> 
> Instead, shouldn't it work like this:
> 
> * If the pinmux driver implementation behind pinmux_request_gpio() needs
> to know the direction when configuring the HW, default to input for safety;
> that will prevent the SoC driving a signal on a GPIO that's driven by some
> other device.

If the GPIO has been configured for output by boot loader
and drives a value, and now you want Linux to take control over it,
then configuring it for input will not be safe at all.
I think this kind of flexibility is necessary (although it can be
implemented in different ways).

> 
> * Rely exclusively on gpio_direction_{input,output} to allow drivers to
> configure the direction.
> 
> * If the pinmux HW needs programming in response to the gpio_direction_*
> calls, have the GPIO and pinmux driver internally communicate to achieve
> this.
> 
> Does that seem reasonable?
> 

-- 
Regards,
Igor.