| Message ID | 1355495185-24220-6-git-send-email-stigge@antcom.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, I have a few comments on the parsing code. On Fri, Dec 14, 2012 at 02:26:24PM +0000, Roland Stigge wrote: > This patch adds device tree support to the block GPIO API. > > Signed-off-by: Roland Stigge <stigge@antcom.de> > > --- > Documentation/devicetree/bindings/gpio/gpio-block.txt | 36 +++++++ > drivers/gpio/Makefile | 1 > drivers/gpio/gpioblock-of.c | 84 ++++++++++++++++++ > 3 files changed, 121 insertions(+) > > --- /dev/null > +++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio-block.txt > @@ -0,0 +1,36 @@ > +Block GPIO definition > +===================== > + > +This binding specifies arbitrary blocks of gpios, combining gpios from one or > +more GPIO controllers together, to form a word for r/w access. > + > +Required property: > +- compatible: must be "linux,gpio-block" > + > +Required subnodes: > +- the name will be the registered name of the block > +- property "gpios" is a list of gpios for the respective block > + > +Example: > + > + blockgpio { > + compatible = "linux,gpio-block"; > + > + block0 { > + gpios = <&gpio 3 0 0>, > + <&gpio 3 1 0>; > + }; > + block1 { > + gpios = <&gpio 4 1 0>, > + <&gpio 4 3 0>, > + <&gpio 4 2 0>, > + <&gpio 4 4 0>, > + <&gpio 4 5 0>, > + <&gpio 4 6 0>, > + <&gpio 4 7 0>, > + <&gpio 4 8 0>, > + <&gpio 4 9 0>, > + <&gpio 4 10 0>, > + <&gpio 4 19 0>; > + }; > + }; > --- linux-2.6.orig/drivers/gpio/Makefile > +++ linux-2.6/drivers/gpio/Makefile > @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG > > obj-$(CONFIG_GPIOLIB) += gpiolib.o devres.o > obj-$(CONFIG_OF_GPIO) += gpiolib-of.o > +obj-$(CONFIG_OF_GPIO) += gpioblock-of.o > > # Device drivers. Generally keep list sorted alphabetically > obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o > --- /dev/null > +++ linux-2.6/drivers/gpio/gpioblock-of.c > @@ -0,0 +1,84 @@ > +/* > + * OF implementation for Block GPIO > + * > + * Copyright (C) 2012 Roland Stigge <stigge@antcom.de> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/gpio.h> > +#include <linux/of.h> > +#include <linux/of_gpio.h> > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/err.h> > + > +static int __devinit gpioblock_of_probe(struct platform_device *pdev) > +{ > + struct device_node *block; > + unsigned *gpios; > + int ngpio; > + int ret; > + struct gpio_block *gb; > + > + for_each_available_child_of_node(pdev->dev.of_node, block) { > + int i; > + > + ngpio = of_gpio_count(block); > + gpios = kzalloc(ngpio * sizeof(*gpios), GFP_KERNEL); What if the block node is malformed? ngpio might be -ENOENT or -EINVAL. > + if (!gpios) > + return -ENOMEM; > + for (i = 0; i < ngpio; i++) { > + ret = of_get_gpio(block, i); > + if (ret < 0) > + return ret; /* expect -EPROBE_DEFER */ > + gpios[i] = ret; > + } > + gb = gpio_block_create(gpios, ngpio, block->name); > + if (IS_ERR(gb)) { > + dev_err(&pdev->dev, > + "Error creating GPIO block from device tree\n"); > + return PTR_ERR(gb); Won't this leak the memory for the gpios object we kzalloc'd earlier? > + } > + ret = gpio_block_register(gb); > + if (ret < 0) { > + gpio_block_free(gb); > + return ret; Same here. > + } > + kfree(gpios); > + dev_info(&pdev->dev, "Registered gpio block %s: %d gpios\n", > + block->name, ngpio); Any of the returns in this block will leave the block node's refcount incremented. > + } > + return 0; > +} > + > +#ifdef CONFIG_OF > +static struct of_device_id gpioblock_of_match[] __devinitdata = { > + { .compatible = "linux,gpio-block", }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, gpioblock_of_match); > +#endif > + > +static struct platform_driver gpioblock_of_driver = { > + .driver = { > + .name = "gpio-block", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(gpioblock_of_match), > + > + }, > + .probe = gpioblock_of_probe, > +}; > + > +module_platform_driver(gpioblock_of_driver); > + > +MODULE_DESCRIPTION("GPIO Block driver"); > +MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS("platform:gpioblock-of"); > Thanks, Mark.
Hi Mark, On 12/17/2012 04:51 PM, Mark Rutland wrote: >> +static int __devinit gpioblock_of_probe(struct platform_device *pdev) >> +{ >> + struct device_node *block; >> + unsigned *gpios; >> + int ngpio; >> + int ret; >> + struct gpio_block *gb; >> + >> + for_each_available_child_of_node(pdev->dev.of_node, block) { >> + int i; >> + >> + ngpio = of_gpio_count(block); >> + gpios = kzalloc(ngpio * sizeof(*gpios), GFP_KERNEL); > > What if the block node is malformed? ngpio might be -ENOENT or -EINVAL. AFAICS, of_gpio_count() always returns at least 0. Both if CONFIG_OF_GPIO is y, m or n. And called of_gpio_named_count() also currently doesn't return error values. Further, other drivers using of_gpio_count() don't expect or catch <0. However, it's reasonable to guard against of_gpio_count() < 1 since probing without provided blocks should be void. Will change this for the next patch update together with your leakage findings. Thanks for reporting! Roland
On Tue, Dec 18, 2012 at 02:30:23PM +0000, Roland Stigge wrote: > Hi Mark, > > On 12/17/2012 04:51 PM, Mark Rutland wrote: > >> +static int __devinit gpioblock_of_probe(struct platform_device *pdev) > >> +{ > >> + struct device_node *block; > >> + unsigned *gpios; > >> + int ngpio; > >> + int ret; > >> + struct gpio_block *gb; > >> + > >> + for_each_available_child_of_node(pdev->dev.of_node, block) { > >> + int i; > >> + > >> + ngpio = of_gpio_count(block); > >> + gpios = kzalloc(ngpio * sizeof(*gpios), GFP_KERNEL); > > > > What if the block node is malformed? ngpio might be -ENOENT or -EINVAL. > > AFAICS, of_gpio_count() always returns at least 0. Both if > CONFIG_OF_GPIO is y, m or n. And called of_gpio_named_count() also > currently doesn't return error values. Further, other drivers using > of_gpio_count() don't expect or catch <0. Whoops. I'd managed to misread the logic in of_gpio_named_count, sorry. > However, it's reasonable to guard against of_gpio_count() < 1 since > probing without provided blocks should be void. > > Will change this for the next patch update together with your leakage > findings. > > Thanks for reporting! > > Roland Sounds good! Thanks, Mark.
--- /dev/null +++ linux-2.6/Documentation/devicetree/bindings/gpio/gpio-block.txt @@ -0,0 +1,36 @@ +Block GPIO definition +===================== + +This binding specifies arbitrary blocks of gpios, combining gpios from one or +more GPIO controllers together, to form a word for r/w access. + +Required property: +- compatible: must be "linux,gpio-block" + +Required subnodes: +- the name will be the registered name of the block +- property "gpios" is a list of gpios for the respective block + +Example: + + blockgpio { + compatible = "linux,gpio-block"; + + block0 { + gpios = <&gpio 3 0 0>, + <&gpio 3 1 0>; + }; + block1 { + gpios = <&gpio 4 1 0>, + <&gpio 4 3 0>, + <&gpio 4 2 0>, + <&gpio 4 4 0>, + <&gpio 4 5 0>, + <&gpio 4 6 0>, + <&gpio 4 7 0>, + <&gpio 4 8 0>, + <&gpio 4 9 0>, + <&gpio 4 10 0>, + <&gpio 4 19 0>; + }; + }; --- linux-2.6.orig/drivers/gpio/Makefile +++ linux-2.6/drivers/gpio/Makefile @@ -4,6 +4,7 @@ ccflags-$(CONFIG_DEBUG_GPIO) += -DDEBUG obj-$(CONFIG_GPIOLIB) += gpiolib.o devres.o obj-$(CONFIG_OF_GPIO) += gpiolib-of.o +obj-$(CONFIG_OF_GPIO) += gpioblock-of.o # Device drivers. Generally keep list sorted alphabetically obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o --- /dev/null +++ linux-2.6/drivers/gpio/gpioblock-of.c @@ -0,0 +1,84 @@ +/* + * OF implementation for Block GPIO + * + * Copyright (C) 2012 Roland Stigge <stigge@antcom.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/gpio.h> +#include <linux/of.h> +#include <linux/of_gpio.h> +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/err.h> + +static int __devinit gpioblock_of_probe(struct platform_device *pdev) +{ + struct device_node *block; + unsigned *gpios; + int ngpio; + int ret; + struct gpio_block *gb; + + for_each_available_child_of_node(pdev->dev.of_node, block) { + int i; + + ngpio = of_gpio_count(block); + gpios = kzalloc(ngpio * sizeof(*gpios), GFP_KERNEL); + if (!gpios) + return -ENOMEM; + for (i = 0; i < ngpio; i++) { + ret = of_get_gpio(block, i); + if (ret < 0) + return ret; /* expect -EPROBE_DEFER */ + gpios[i] = ret; + } + gb = gpio_block_create(gpios, ngpio, block->name); + if (IS_ERR(gb)) { + dev_err(&pdev->dev, + "Error creating GPIO block from device tree\n"); + return PTR_ERR(gb); + } + ret = gpio_block_register(gb); + if (ret < 0) { + gpio_block_free(gb); + return ret; + } + kfree(gpios); + dev_info(&pdev->dev, "Registered gpio block %s: %d gpios\n", + block->name, ngpio); + } + return 0; +} + +#ifdef CONFIG_OF +static struct of_device_id gpioblock_of_match[] __devinitdata = { + { .compatible = "linux,gpio-block", }, + { }, +}; +MODULE_DEVICE_TABLE(of, gpioblock_of_match); +#endif + +static struct platform_driver gpioblock_of_driver = { + .driver = { + .name = "gpio-block", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(gpioblock_of_match), + + }, + .probe = gpioblock_of_probe, +}; + +module_platform_driver(gpioblock_of_driver); + +MODULE_DESCRIPTION("GPIO Block driver"); +MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:gpioblock-of");
This patch adds device tree support to the block GPIO API. Signed-off-by: Roland Stigge <stigge@antcom.de> --- Documentation/devicetree/bindings/gpio/gpio-block.txt | 36 +++++++ drivers/gpio/Makefile | 1 drivers/gpio/gpioblock-of.c | 84 ++++++++++++++++++ 3 files changed, 121 insertions(+)