[v2] of_gpio: Return GPIO flags from of_get_gpio()

Submitted by Trent Piepho on Oct. 31, 2008, 2:03 a.m.

Details

Message ID 1225418589-8545-1-git-send-email-tpiepho@freescale.com
State Rejected, archived
Headers show

Commit Message

Trent Piepho Oct. 31, 2008, 2:03 a.m.
The device binding spec for OF GPIOs defines a flags field, but there is
currently no way to get it.  This patch adds a parameter to of_get_gpio()
where the flags will be returned if non-NULL.  of_get_gpio() in turn passes
the parameter to the of_gpio_chip's xlate method, which can extract any
flags present from the gpio_spec.

The default (and currently only) of_gpio_chip xlate method,
of_gpio_simple_xlate(), is modified to do this.

Signed-off-by: Trent Piepho <tpiepho@freescale.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
Since this patch changes an API, it would be nice to get it into powerpc-next
soon so that people who have new patches that use this API, like Anton, can
base off it.

 drivers/mtd/nand/fsl_upm.c              |    2 +-
 drivers/net/fs_enet/fs_enet-main.c      |    2 +-
 drivers/net/phy/mdio-ofgpio.c           |    4 ++--
 drivers/of/gpio.c                       |   13 ++++++++++---
 drivers/serial/cpm_uart/cpm_uart_core.c |    2 +-
 include/linux/of_gpio.h                 |   21 +++++++++++++++++----
 6 files changed, 32 insertions(+), 12 deletions(-)

Comments

Anton Vorontsov Nov. 26, 2008, 4:20 p.m.
On Thu, Oct 30, 2008 at 07:03:09PM -0700, Trent Piepho wrote:
> The device binding spec for OF GPIOs defines a flags field, but there is
> currently no way to get it.  This patch adds a parameter to of_get_gpio()
> where the flags will be returned if non-NULL.  of_get_gpio() in turn passes
> the parameter to the of_gpio_chip's xlate method, which can extract any
> flags present from the gpio_spec.
> 
> The default (and currently only) of_gpio_chip xlate method,
> of_gpio_simple_xlate(), is modified to do this.
> 
> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> ---
> Since this patch changes an API, it would be nice to get it into powerpc-next
> soon so that people who have new patches that use this API, like Anton, can
> base off it.

Can we apply it? Paul, Benjamin?

The patchwork url for this patch is:
http://patchwork.ozlabs.org/patch/6650/


Thanks!

>  drivers/mtd/nand/fsl_upm.c              |    2 +-
>  drivers/net/fs_enet/fs_enet-main.c      |    2 +-
>  drivers/net/phy/mdio-ofgpio.c           |    4 ++--
>  drivers/of/gpio.c                       |   13 ++++++++++---
>  drivers/serial/cpm_uart/cpm_uart_core.c |    2 +-
>  include/linux/of_gpio.h                 |   21 +++++++++++++++++----
>  6 files changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
> index 024e3ff..a25d962 100644
> --- a/drivers/mtd/nand/fsl_upm.c
> +++ b/drivers/mtd/nand/fsl_upm.c
> @@ -218,7 +218,7 @@ static int __devinit fun_probe(struct of_device *ofdev,
>  	}
>  	fun->upm_cmd_offset = *prop;
>  
> -	fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
> +	fun->rnb_gpio = of_get_gpio(ofdev->node, 0, NULL);
>  	if (fun->rnb_gpio >= 0) {
>  		ret = gpio_request(fun->rnb_gpio, ofdev->dev.bus_id);
>  		if (ret) {
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
> index cb51c1f..5a3c7ee 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -994,7 +994,7 @@ static int __devinit find_phy(struct device_node *np,
>  		goto out_put_phy;
>  	}
>  
> -	bus_id = of_get_gpio(mdionode, 0);
> +	bus_id = of_get_gpio(mdionode, 0, NULL);
>  	if (bus_id < 0) {
>  		struct resource res;
>  		ret = of_address_to_resource(mdionode, 0, &res);
> diff --git a/drivers/net/phy/mdio-ofgpio.c b/drivers/net/phy/mdio-ofgpio.c
> index 2ff9775..e3757c6 100644
> --- a/drivers/net/phy/mdio-ofgpio.c
> +++ b/drivers/net/phy/mdio-ofgpio.c
> @@ -78,8 +78,8 @@ static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
>  {
>  	struct mdio_gpio_info *bitbang = bus->priv;
>  
> -	bitbang->mdc = of_get_gpio(np, 0);
> -	bitbang->mdio = of_get_gpio(np, 1);
> +	bitbang->mdc = of_get_gpio(np, 0, NULL);
> +	bitbang->mdio = of_get_gpio(np, 1, NULL);
>  
>  	if (bitbang->mdc < 0 || bitbang->mdio < 0)
>  		return -ENODEV;
> diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
> index 7cd7301..ae14215 100644
> --- a/drivers/of/gpio.c
> +++ b/drivers/of/gpio.c
> @@ -22,11 +22,12 @@
>   * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
>   * @np:		device node to get GPIO from
>   * @index:	index of the GPIO
> + * @flags:	GPIO's flags are returned here if non-NULL
>   *
>   * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
>   * value on the error condition.
>   */
> -int of_get_gpio(struct device_node *np, int index)
> +int of_get_gpio(struct device_node *np, int index, enum of_gpio_flags *flags)
>  {
>  	int ret;
>  	struct device_node *gc;
> @@ -59,7 +60,9 @@ int of_get_gpio(struct device_node *np, int index)
>  		goto err1;
>  	}
>  
> -	ret = of_gc->xlate(of_gc, np, gpio_spec);
> +	if (flags)
> +		*flags = 0;
> +	ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
>  	if (ret < 0)
>  		goto err1;
>  
> @@ -77,19 +80,23 @@ EXPORT_SYMBOL(of_get_gpio);
>   * @of_gc:	pointer to the of_gpio_chip structure
>   * @np:		device node of the GPIO chip
>   * @gpio_spec:	gpio specifier as found in the device tree
> + * @flags:	if non-NULL flags are returned here
>   *
>   * This is simple translation function, suitable for the most 1:1 mapped
>   * gpio chips. This function performs only one sanity check: whether gpio
>   * is less than ngpios (that is specified in the gpio_chip).
>   */
>  int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
> -			 const void *gpio_spec)
> +			 const void *gpio_spec, enum of_gpio_flags *flags)
>  {
>  	const u32 *gpio = gpio_spec;
>  
>  	if (*gpio > of_gc->gc.ngpio)
>  		return -EINVAL;
>  
> +	if (flags && of_gc->gpio_cells > 1)
> +		*flags = gpio[1];
> +
>  	return *gpio;
>  }
>  EXPORT_SYMBOL(of_gpio_simple_xlate);
> diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
> index bde4b4b..7835cd4 100644
> --- a/drivers/serial/cpm_uart/cpm_uart_core.c
> +++ b/drivers/serial/cpm_uart/cpm_uart_core.c
> @@ -1094,7 +1094,7 @@ static int cpm_uart_init_port(struct device_node *np,
>  	}
>  
>  	for (i = 0; i < NUM_GPIOS; i++)
> -		pinfo->gpios[i] = of_get_gpio(np, i);
> +		pinfo->gpios[i] = of_get_gpio(np, i, NULL);
>  
>  	return cpm_uart_request_port(&pinfo->port);
>  
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 67db101..d611746 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -20,13 +20,23 @@
>  #ifdef CONFIG_OF_GPIO
>  
>  /*
> + * Flags as returned by OF GPIO chip's xlate method.
> + * The GPIO specifier in the OF device tree does not need to use this
> + * same format for its flags, but it's convenient if it does.  For
> + * example, the of_mm_gpio_chip driver works this way.
> + */
> +enum of_gpio_flags {
> +	OF_GPIO_ACTIVE_LOW = 1,
> +};
> +
> +/*
>   * Generic OF GPIO chip
>   */
>  struct of_gpio_chip {
>  	struct gpio_chip gc;
>  	int gpio_cells;
>  	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
> -		     const void *gpio_spec);
> +		     const void *gpio_spec, enum of_gpio_flags *flags);
>  };
>  
>  static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
> @@ -50,16 +60,19 @@ static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
>  	return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
>  }
>  
> -extern int of_get_gpio(struct device_node *np, int index);
> +extern int of_get_gpio(struct device_node *np, int index,
> +		       enum of_gpio_flags *flags);
>  extern int of_mm_gpiochip_add(struct device_node *np,
>  			      struct of_mm_gpio_chip *mm_gc);
>  extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
>  				struct device_node *np,
> -				const void *gpio_spec);
> +				const void *gpio_spec,
> +				enum of_gpio_flags *flags);
>  #else
>  
>  /* Drivers may not strictly depend on the GPIO support, so let them link. */
> -static inline int of_get_gpio(struct device_node *np, int index)
> +static inline int of_get_gpio(struct device_node *np, int index,
> +			      enum of_gpio_flags *flags)
>  {
>  	return -ENOSYS;
>  }
> -- 
> 1.5.4.3
>
Paul Mackerras Nov. 26, 2008, 9:38 p.m.
Anton Vorontsov writes:

> Can we apply it? Paul, Benjamin?
> 
> The patchwork url for this patch is:
> http://patchwork.ozlabs.org/patch/6650/
> 
> 
> Thanks!
> 
> >  drivers/mtd/nand/fsl_upm.c              |    2 +-
> >  drivers/net/fs_enet/fs_enet-main.c      |    2 +-
> >  drivers/net/phy/mdio-ofgpio.c           |    4 ++--
> >  drivers/of/gpio.c                       |   13 ++++++++++---
> >  drivers/serial/cpm_uart/cpm_uart_core.c |    2 +-
> >  include/linux/of_gpio.h                 |   21 +++++++++++++++++----
> >  6 files changed, 32 insertions(+), 12 deletions(-)

That would need acks from Jeff Garzik and David Woodhouse.

Alternatively you could add a new function (called, for instance,
of_get_gpio_flags) with the extra parameter to eliminate the need to
change any drivers at this stage, since they all seem to pass NULL for
the flags argument.

Paul.
Anton Vorontsov Nov. 26, 2008, 10:31 p.m.
On Thu, Nov 27, 2008 at 08:38:51AM +1100, Paul Mackerras wrote:
[...]
> > >  drivers/mtd/nand/fsl_upm.c              |    2 +-
> > >  drivers/net/fs_enet/fs_enet-main.c      |    2 +-
> > >  drivers/net/phy/mdio-ofgpio.c           |    4 ++--
> > >  drivers/of/gpio.c                       |   13 ++++++++++---
> > >  drivers/serial/cpm_uart/cpm_uart_core.c |    2 +-
> > >  include/linux/of_gpio.h                 |   21 +++++++++++++++++----
> > >  6 files changed, 32 insertions(+), 12 deletions(-)
> 
> That would need acks from Jeff Garzik and David Woodhouse.
> 
> Alternatively you could add a new function (called, for instance,
> of_get_gpio_flags) with the extra parameter to eliminate the need to
> change any drivers at this stage, since they all seem to pass NULL for
> the flags argument.

:-))

This was _exactly_ my initial approach, but Trent and Grant thought
that changing API would be easy. Hah-ha!

Here is my initial patch implementing of_get_gpio_flags():
http://lkml.org/lkml/2008/7/25/236

Trent, are you going to push your patch through all the
maintainers, or should we, after all, revert to my approach
instead?

Thanks,
Trent Piepho Nov. 26, 2008, 10:35 p.m.
On Thu, 27 Nov 2008, Paul Mackerras wrote:
> Anton Vorontsov writes:
>
>> Can we apply it? Paul, Benjamin?
>>
>> The patchwork url for this patch is:
>> http://patchwork.ozlabs.org/patch/6650/
>>
>>
>> Thanks!
>>
>>>  drivers/mtd/nand/fsl_upm.c              |    2 +-
>>>  drivers/net/fs_enet/fs_enet-main.c      |    2 +-
>>>  drivers/net/phy/mdio-ofgpio.c           |    4 ++--
>>>  drivers/of/gpio.c                       |   13 ++++++++++---
>>>  drivers/serial/cpm_uart/cpm_uart_core.c |    2 +-
>>>  include/linux/of_gpio.h                 |   21 +++++++++++++++++----
>>>  6 files changed, 32 insertions(+), 12 deletions(-)
>
> That would need acks from Jeff Garzik and David Woodhouse.
>
> Alternatively you could add a new function (called, for instance,
> of_get_gpio_flags) with the extra parameter to eliminate the need to
> change any drivers at this stage, since they all seem to pass NULL for
> the flags argument.

But if we did this every time any exported function needs to change, think
how bloated the API would be with cruft.
Anton Vorontsov Nov. 26, 2008, 10:58 p.m.
On Wed, Nov 26, 2008 at 02:35:54PM -0800, Trent Piepho wrote:
> On Thu, 27 Nov 2008, Paul Mackerras wrote:
> > Anton Vorontsov writes:
> >
> >> Can we apply it? Paul, Benjamin?
> >>
> >> The patchwork url for this patch is:
> >> http://patchwork.ozlabs.org/patch/6650/
> >>
> >>
> >> Thanks!
> >>
> >>>  drivers/mtd/nand/fsl_upm.c              |    2 +-
> >>>  drivers/net/fs_enet/fs_enet-main.c      |    2 +-
> >>>  drivers/net/phy/mdio-ofgpio.c           |    4 ++--
> >>>  drivers/of/gpio.c                       |   13 ++++++++++---
> >>>  drivers/serial/cpm_uart/cpm_uart_core.c |    2 +-
> >>>  include/linux/of_gpio.h                 |   21 +++++++++++++++++----
> >>>  6 files changed, 32 insertions(+), 12 deletions(-)
> >
> > That would need acks from Jeff Garzik and David Woodhouse.
> >
> > Alternatively you could add a new function (called, for instance,
> > of_get_gpio_flags) with the extra parameter to eliminate the need to
> > change any drivers at this stage, since they all seem to pass NULL for
> > the flags argument.
> 
> But if we did this every time any exported function needs to change, think
> how bloated the API would be with cruft.

Stable API is nonsense, yes. But we tend to change the API evolutionary,
not revolutionary. That is,

1. Implement of_get_gpio_flags();
2. Now we can start using it (no stall in development, see?);
3. Then somebody comes with the _cleanup_ patch:
   "[PATCH] Merge of_get_gpio_flags() and of_get_gpio(), convert users"

   ^^ That patch is trivial and could be applied at any appropriate
   moment (i.e. when there are no of_*_gpio*() patches queued in the
   -next trees). And as time goes by, the patch collects all the needed
   Acks, no need to hurry -- it's trivial cleanup.
Paul Mackerras Nov. 26, 2008, 11:32 p.m.
Trent Piepho writes:

> > Alternatively you could add a new function (called, for instance,
> > of_get_gpio_flags) with the extra parameter to eliminate the need to
> > change any drivers at this stage, since they all seem to pass NULL for
> > the flags argument.
> 
> But if we did this every time any exported function needs to change, think
> how bloated the API would be with cruft.

I don't buy the argument that we can't add one thing because if we
added a hundred that would be too much.  You could add
of_get_gpio_flags, get that upstream, then get the driver patches
upstream, then submit a patch to remove of_get_gpio.  Alternatively
you could make of_get_gpio a macro or inline function in of_gpio.h.

If you really want to change everything in one hit you'll have to get
acks + agreement for the change to go upstream via my tree from all
the relevant driver maintainers first.  I don't see any particular
advantage to doing it that way, though.

Paul.

Patch hide | download patch | download mbox

diff --git a/drivers/mtd/nand/fsl_upm.c b/drivers/mtd/nand/fsl_upm.c
index 024e3ff..a25d962 100644
--- a/drivers/mtd/nand/fsl_upm.c
+++ b/drivers/mtd/nand/fsl_upm.c
@@ -218,7 +218,7 @@  static int __devinit fun_probe(struct of_device *ofdev,
 	}
 	fun->upm_cmd_offset = *prop;
 
-	fun->rnb_gpio = of_get_gpio(ofdev->node, 0);
+	fun->rnb_gpio = of_get_gpio(ofdev->node, 0, NULL);
 	if (fun->rnb_gpio >= 0) {
 		ret = gpio_request(fun->rnb_gpio, ofdev->dev.bus_id);
 		if (ret) {
diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index cb51c1f..5a3c7ee 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -994,7 +994,7 @@  static int __devinit find_phy(struct device_node *np,
 		goto out_put_phy;
 	}
 
-	bus_id = of_get_gpio(mdionode, 0);
+	bus_id = of_get_gpio(mdionode, 0, NULL);
 	if (bus_id < 0) {
 		struct resource res;
 		ret = of_address_to_resource(mdionode, 0, &res);
diff --git a/drivers/net/phy/mdio-ofgpio.c b/drivers/net/phy/mdio-ofgpio.c
index 2ff9775..e3757c6 100644
--- a/drivers/net/phy/mdio-ofgpio.c
+++ b/drivers/net/phy/mdio-ofgpio.c
@@ -78,8 +78,8 @@  static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
 {
 	struct mdio_gpio_info *bitbang = bus->priv;
 
-	bitbang->mdc = of_get_gpio(np, 0);
-	bitbang->mdio = of_get_gpio(np, 1);
+	bitbang->mdc = of_get_gpio(np, 0, NULL);
+	bitbang->mdio = of_get_gpio(np, 1, NULL);
 
 	if (bitbang->mdc < 0 || bitbang->mdio < 0)
 		return -ENODEV;
diff --git a/drivers/of/gpio.c b/drivers/of/gpio.c
index 7cd7301..ae14215 100644
--- a/drivers/of/gpio.c
+++ b/drivers/of/gpio.c
@@ -22,11 +22,12 @@ 
  * of_get_gpio - Get a GPIO number from the device tree to use with GPIO API
  * @np:		device node to get GPIO from
  * @index:	index of the GPIO
+ * @flags:	GPIO's flags are returned here if non-NULL
  *
  * Returns GPIO number to use with Linux generic GPIO API, or one of the errno
  * value on the error condition.
  */
-int of_get_gpio(struct device_node *np, int index)
+int of_get_gpio(struct device_node *np, int index, enum of_gpio_flags *flags)
 {
 	int ret;
 	struct device_node *gc;
@@ -59,7 +60,9 @@  int of_get_gpio(struct device_node *np, int index)
 		goto err1;
 	}
 
-	ret = of_gc->xlate(of_gc, np, gpio_spec);
+	if (flags)
+		*flags = 0;
+	ret = of_gc->xlate(of_gc, np, gpio_spec, flags);
 	if (ret < 0)
 		goto err1;
 
@@ -77,19 +80,23 @@  EXPORT_SYMBOL(of_get_gpio);
  * @of_gc:	pointer to the of_gpio_chip structure
  * @np:		device node of the GPIO chip
  * @gpio_spec:	gpio specifier as found in the device tree
+ * @flags:	if non-NULL flags are returned here
  *
  * This is simple translation function, suitable for the most 1:1 mapped
  * gpio chips. This function performs only one sanity check: whether gpio
  * is less than ngpios (that is specified in the gpio_chip).
  */
 int of_gpio_simple_xlate(struct of_gpio_chip *of_gc, struct device_node *np,
-			 const void *gpio_spec)
+			 const void *gpio_spec, enum of_gpio_flags *flags)
 {
 	const u32 *gpio = gpio_spec;
 
 	if (*gpio > of_gc->gc.ngpio)
 		return -EINVAL;
 
+	if (flags && of_gc->gpio_cells > 1)
+		*flags = gpio[1];
+
 	return *gpio;
 }
 EXPORT_SYMBOL(of_gpio_simple_xlate);
diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index bde4b4b..7835cd4 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -1094,7 +1094,7 @@  static int cpm_uart_init_port(struct device_node *np,
 	}
 
 	for (i = 0; i < NUM_GPIOS; i++)
-		pinfo->gpios[i] = of_get_gpio(np, i);
+		pinfo->gpios[i] = of_get_gpio(np, i, NULL);
 
 	return cpm_uart_request_port(&pinfo->port);
 
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 67db101..d611746 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -20,13 +20,23 @@ 
 #ifdef CONFIG_OF_GPIO
 
 /*
+ * Flags as returned by OF GPIO chip's xlate method.
+ * The GPIO specifier in the OF device tree does not need to use this
+ * same format for its flags, but it's convenient if it does.  For
+ * example, the of_mm_gpio_chip driver works this way.
+ */
+enum of_gpio_flags {
+	OF_GPIO_ACTIVE_LOW = 1,
+};
+
+/*
  * Generic OF GPIO chip
  */
 struct of_gpio_chip {
 	struct gpio_chip gc;
 	int gpio_cells;
 	int (*xlate)(struct of_gpio_chip *of_gc, struct device_node *np,
-		     const void *gpio_spec);
+		     const void *gpio_spec, enum of_gpio_flags *flags);
 };
 
 static inline struct of_gpio_chip *to_of_gpio_chip(struct gpio_chip *gc)
@@ -50,16 +60,19 @@  static inline struct of_mm_gpio_chip *to_of_mm_gpio_chip(struct gpio_chip *gc)
 	return container_of(of_gc, struct of_mm_gpio_chip, of_gc);
 }
 
-extern int of_get_gpio(struct device_node *np, int index);
+extern int of_get_gpio(struct device_node *np, int index,
+		       enum of_gpio_flags *flags);
 extern int of_mm_gpiochip_add(struct device_node *np,
 			      struct of_mm_gpio_chip *mm_gc);
 extern int of_gpio_simple_xlate(struct of_gpio_chip *of_gc,
 				struct device_node *np,
-				const void *gpio_spec);
+				const void *gpio_spec,
+				enum of_gpio_flags *flags);
 #else
 
 /* Drivers may not strictly depend on the GPIO support, so let them link. */
-static inline int of_get_gpio(struct device_node *np, int index)
+static inline int of_get_gpio(struct device_node *np, int index,
+			      enum of_gpio_flags *flags)
 {
 	return -ENOSYS;
 }