| Message ID | 1421581173-28416-8-git-send-email-ricardo.ribalda@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
>>>>> "Ricardo" == Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> writes: > This way we do not need to transverse the device tree manually. > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Peter Korsgaard <jacmet@sunsite.dk> > Cc: devicetree@vger.kernel.org > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> Looks sensible. Acked-by: Peter Korsgaard <peter@korsgaard.com>
>>>>> "Ricardo" == Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> writes: > This way we do not need to transverse the device tree manually. > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Peter Korsgaard <jacmet@sunsite.dk> > Cc: devicetree@vger.kernel.org > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > --- > drivers/gpio/gpio-mpc8xxx.c | 48 +++++++++++++++++++++------------------------ > 1 file changed, 22 insertions(+), 26 deletions(-) > diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c > index d1ff879..57eb794 100644 > --- a/drivers/gpio/gpio-mpc8xxx.c > +++ b/drivers/gpio/gpio-mpc8xxx.c > @@ -15,6 +15,7 @@ > #include <linux/of.h> > #include <linux/of_gpio.h> > #include <linux/of_irq.h> > +#include <linux/of_platform.h> > #include <linux/gpio.h> > #include <linux/slab.h> > #include <linux/irq.h> > @@ -342,8 +343,9 @@ static struct of_device_id mpc8xxx_gpio_ids[] __initdata = { > {} > }; > -static void __init mpc8xxx_add_controller(struct device_node *np) > +static int mpc8xxx_probe(struct platform_device *pdev) > { > + struct device_node *np = pdev->dev.of_node; > struct mpc8xxx_gpio_chip *mpc8xxx_gc; > struct of_mm_gpio_chip *mm_gc; > struct gpio_chip *gc; > @@ -351,11 +353,9 @@ static void __init mpc8xxx_add_controller(struct device_node *np) > unsigned hwirq; > int ret; > - mpc8xxx_gc = kzalloc(sizeof(*mpc8xxx_gc), GFP_KERNEL); > - if (!mpc8xxx_gc) { > - ret = -ENOMEM; > - goto err; > - } > + mpc8xxx_gc = devm_kzalloc(&pdev->dev, sizeof(*mpc8xxx_gc), GFP_KERNEL); > + if (!mpc8xxx_gc) > + return -ENOMEM; > spin_lock_init(&mpc8xxx_gc->lock); > @@ -375,16 +375,16 @@ static void __init mpc8xxx_add_controller(struct device_node *np) > ret = of_mm_gpiochip_add(np, mm_gc); > if (ret) > - goto err; > + return ret; > hwirq = irq_of_parse_and_map(np, 0); > if (hwirq == NO_IRQ) > - goto skip_irq; > + return 0; Actually, looking closer - Isn't this leaking mm_gc? It was as well before, but it would be good to get it fixed. > mpc8xxx_gc-> irq = irq_domain_add_linear(np, MPC8XXX_GPIO_PINS, > &mpc8xxx_gpio_irq_ops, mpc8xxx_gc); > if (!mpc8xxx_gc->irq) > - goto skip_irq; > + return 0; And here as well?
Hello Peter I thought the logic behind the original driver was: 1) Create gpiochip so it can be used by other modules/userland 2) if there is also a irq available for it, create a irqdomain. otherwise continue. I can change the code so it does not continue, and exit cleanly if the irqdomain fails. You are the original developer of the driver. So: do you think it makes sense to continue only as a gpiochip? :P If I need to change it I rather add it as a another patch to the patchset, so if it causes an issue somewhere it can be found easier via bisect. Regards! On Mon, Jan 19, 2015 at 11:48 PM, Peter Korsgaard <peter@korsgaard.com> wrote: >>>>>> "Ricardo" == Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> writes: > > > This way we do not need to transverse the device tree manually. > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Alexandre Courbot <gnurou@gmail.com> > > Cc: Grant Likely <grant.likely@linaro.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Peter Korsgaard <jacmet@sunsite.dk> > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> > > --- > > drivers/gpio/gpio-mpc8xxx.c | 48 +++++++++++++++++++++------------------------ > > 1 file changed, 22 insertions(+), 26 deletions(-) > > > diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c > > index d1ff879..57eb794 100644 > > --- a/drivers/gpio/gpio-mpc8xxx.c > > +++ b/drivers/gpio/gpio-mpc8xxx.c > > @@ -15,6 +15,7 @@ > > #include <linux/of.h> > > #include <linux/of_gpio.h> > > #include <linux/of_irq.h> > > +#include <linux/of_platform.h> > > #include <linux/gpio.h> > > #include <linux/slab.h> > > #include <linux/irq.h> > > @@ -342,8 +343,9 @@ static struct of_device_id mpc8xxx_gpio_ids[] __initdata = { > > {} > > }; > > > -static void __init mpc8xxx_add_controller(struct device_node *np) > > +static int mpc8xxx_probe(struct platform_device *pdev) > > { > > + struct device_node *np = pdev->dev.of_node; > > struct mpc8xxx_gpio_chip *mpc8xxx_gc; > > struct of_mm_gpio_chip *mm_gc; > > struct gpio_chip *gc; > > @@ -351,11 +353,9 @@ static void __init mpc8xxx_add_controller(struct device_node *np) > > unsigned hwirq; > > int ret; > > > - mpc8xxx_gc = kzalloc(sizeof(*mpc8xxx_gc), GFP_KERNEL); > > - if (!mpc8xxx_gc) { > > - ret = -ENOMEM; > > - goto err; > > - } > > + mpc8xxx_gc = devm_kzalloc(&pdev->dev, sizeof(*mpc8xxx_gc), GFP_KERNEL); > > + if (!mpc8xxx_gc) > > + return -ENOMEM; > > > spin_lock_init(&mpc8xxx_gc->lock); > > > @@ -375,16 +375,16 @@ static void __init mpc8xxx_add_controller(struct device_node *np) > > > ret = of_mm_gpiochip_add(np, mm_gc); > > if (ret) > > - goto err; > > + return ret; > > > hwirq = irq_of_parse_and_map(np, 0); > > if (hwirq == NO_IRQ) > > - goto skip_irq; > > + return 0; > > Actually, looking closer - Isn't this leaking mm_gc? It was as well > before, but it would be good to get it fixed. > > > > mpc8xxx_gc-> irq = irq_domain_add_linear(np, MPC8XXX_GPIO_PINS, > > &mpc8xxx_gpio_irq_ops, mpc8xxx_gc); > > if (!mpc8xxx_gc->irq) > > - goto skip_irq; > > + return 0; > > And here as well? > > > -- > Bye, Peter Korsgaard
On Sun, Jan 18, 2015 at 8:39 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > This way we do not need to transverse the device tree manually. ... and this makes things much more legible. Acked-by: Alexandre Courbot <acourbot@nvidia.com> -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jan 18, 2015 at 12:39 PM, Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> wrote: > This way we do not need to transverse the device tree manually. > > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Alexandre Courbot <gnurou@gmail.com> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Peter Korsgaard <jacmet@sunsite.dk> > Cc: devicetree@vger.kernel.org > Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> Patch applied with the ACKs, hope nothing explodes. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Ricardo" == Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> writes: > Hello Peter > I thought the logic behind the original driver was: > 1) Create gpiochip so it can be used by other modules/userland > 2) if there is also a irq available for it, create a irqdomain. > otherwise continue. Indeed - Sorry, I must have been confused yesterday.
No worries :) On Tue, Jan 20, 2015 at 11:41 AM, Peter Korsgaard <peter@korsgaard.com> wrote: >>>>>> "Ricardo" == Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> writes: > > > Hello Peter > > I thought the logic behind the original driver was: > > > 1) Create gpiochip so it can be used by other modules/userland > > 2) if there is also a irq available for it, create a irqdomain. > > otherwise continue. > > Indeed - Sorry, I must have been confused yesterday. > > -- > Bye, Peter Korsgaard
diff --git a/drivers/gpio/gpio-mpc8xxx.c b/drivers/gpio/gpio-mpc8xxx.c index d1ff879..57eb794 100644 --- a/drivers/gpio/gpio-mpc8xxx.c +++ b/drivers/gpio/gpio-mpc8xxx.c @@ -15,6 +15,7 @@ #include <linux/of.h> #include <linux/of_gpio.h> #include <linux/of_irq.h> +#include <linux/of_platform.h> #include <linux/gpio.h> #include <linux/slab.h> #include <linux/irq.h> @@ -342,8 +343,9 @@ static struct of_device_id mpc8xxx_gpio_ids[] __initdata = { {} }; -static void __init mpc8xxx_add_controller(struct device_node *np) +static int mpc8xxx_probe(struct platform_device *pdev) { + struct device_node *np = pdev->dev.of_node; struct mpc8xxx_gpio_chip *mpc8xxx_gc; struct of_mm_gpio_chip *mm_gc; struct gpio_chip *gc; @@ -351,11 +353,9 @@ static void __init mpc8xxx_add_controller(struct device_node *np) unsigned hwirq; int ret; - mpc8xxx_gc = kzalloc(sizeof(*mpc8xxx_gc), GFP_KERNEL); - if (!mpc8xxx_gc) { - ret = -ENOMEM; - goto err; - } + mpc8xxx_gc = devm_kzalloc(&pdev->dev, sizeof(*mpc8xxx_gc), GFP_KERNEL); + if (!mpc8xxx_gc) + return -ENOMEM; spin_lock_init(&mpc8xxx_gc->lock); @@ -375,16 +375,16 @@ static void __init mpc8xxx_add_controller(struct device_node *np) ret = of_mm_gpiochip_add(np, mm_gc); if (ret) - goto err; + return ret; hwirq = irq_of_parse_and_map(np, 0); if (hwirq == NO_IRQ) - goto skip_irq; + return 0; mpc8xxx_gc->irq = irq_domain_add_linear(np, MPC8XXX_GPIO_PINS, &mpc8xxx_gpio_irq_ops, mpc8xxx_gc); if (!mpc8xxx_gc->irq) - goto skip_irq; + return 0; id = of_match_node(mpc8xxx_gpio_ids, np); if (id) @@ -397,24 +397,20 @@ static void __init mpc8xxx_add_controller(struct device_node *np) irq_set_handler_data(hwirq, mpc8xxx_gc); irq_set_chained_handler(hwirq, mpc8xxx_gpio_irq_cascade); -skip_irq: - return; - -err: - pr_err("%s: registration failed with status %d\n", - np->full_name, ret); - kfree(mpc8xxx_gc); - - return; + return 0; } -static int __init mpc8xxx_add_gpiochips(void) -{ - struct device_node *np; - - for_each_matching_node(np, mpc8xxx_gpio_ids) - mpc8xxx_add_controller(np); +static struct platform_driver mpc8xxx_plat_driver = { + .probe = mpc8xxx_probe, + .driver = { + .name = "gpio-mpc8xxx", + .of_match_table = mpc8xxx_gpio_ids, + }, +}; - return 0; +static int __init mpc8xxx_init(void) +{ + return platform_driver_register(&mpc8xxx_plat_driver); } -arch_initcall(mpc8xxx_add_gpiochips); + +arch_initcall(mpc8xxx_init);
This way we do not need to transverse the device tree manually. Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Alexandre Courbot <gnurou@gmail.com> Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Peter Korsgaard <jacmet@sunsite.dk> Cc: devicetree@vger.kernel.org Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> --- drivers/gpio/gpio-mpc8xxx.c | 48 +++++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 26 deletions(-)