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

login
register
mail settings
Submitter Trent Piepho
Date Oct. 31, 2008, 2:03 a.m.
Message ID <1225418589-8545-1-git-send-email-tpiepho@freescale.com>
Download mbox | patch
Permalink /patch/6649/
State Rejected
Headers show

Comments

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

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