| Message ID | 1225418589-8545-1-git-send-email-tpiepho@freescale.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
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 >
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.
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,
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.
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.
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.
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; }