[PATCH 4/8] gpio-mm-lantiq: Use of_propery_read_u16

[PATCH 4/8] gpio-mm-lantiq: Use of_propery_read_u16

[Ricardo Ribalda Delgado]

Instead of parsing manually the shadow content, use the much simpler
helper of_property_read_u16.

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: John Crispin <blogic@openwrt.org>
Cc: devicetree@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/gpio/gpio-mm-lantiq.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-mm-lantiq.c b/drivers/gpio/gpio-mm-lantiq.c
index 7d55161..b511a6b 100644
--- a/drivers/gpio/gpio-mm-lantiq.c
+++ b/drivers/gpio/gpio-mm-lantiq.c
@@ -105,7 +105,6 @@ static void ltq_mm_save_regs(struct of_mm_gpio_chip *mm_gc)
 static int ltq_mm_probe(struct platform_device *pdev)
 {
 	struct ltq_mm *chip;
-	const __be32 *shadow;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -117,9 +116,8 @@ static int ltq_mm_probe(struct platform_device *pdev)
 	chip->mmchip.save_regs = ltq_mm_save_regs;
 
 	/* store the shadow value if one was passed by the devicetree */
-	shadow = of_get_property(pdev->dev.of_node, "lantiq,shadow", NULL);
-	if (shadow)
-		chip->shadow = be32_to_cpu(*shadow);
+	of_property_read_u16(pdev->dev.of_node, "lantiq,shadow",
+			     &chip->shadow);
 
 	return of_mm_gpiochip_add(pdev->dev.of_node, &chip->mmchip);
 }
-- 
2.1.4

Re: [PATCH 4/8] gpio-mm-lantiq: Use of_propery_read_u16

[Mark Rutland]

On Sun, Jan 18, 2015 at 11:39:29AM +0000, Ricardo Ribalda Delgado wrote:
> Instead of parsing manually the shadow content, use the much simpler
> helper of_property_read_u16.
> 
> Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: John Crispin <blogic-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpio/gpio-mm-lantiq.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mm-lantiq.c b/drivers/gpio/gpio-mm-lantiq.c
> index 7d55161..b511a6b 100644
> --- a/drivers/gpio/gpio-mm-lantiq.c
> +++ b/drivers/gpio/gpio-mm-lantiq.c
> @@ -105,7 +105,6 @@ static void ltq_mm_save_regs(struct of_mm_gpio_chip *mm_gc)
>  static int ltq_mm_probe(struct platform_device *pdev)
>  {
>  	struct ltq_mm *chip;
> -	const __be32 *shadow;
>  
>  	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>  	if (!chip)
> @@ -117,9 +116,8 @@ static int ltq_mm_probe(struct platform_device *pdev)
>  	chip->mmchip.save_regs = ltq_mm_save_regs;
>  
>  	/* store the shadow value if one was passed by the devicetree */
> -	shadow = of_get_property(pdev->dev.of_node, "lantiq,shadow", NULL);
> -	if (shadow)
> -		chip->shadow = be32_to_cpu(*shadow);
> +	of_property_read_u16(pdev->dev.of_node, "lantiq,shadow",
> +			     &chip->shadow);

The old code and new code aren't equivalent:

The binding doesn't specify the use of /bits/ 16 <...>, nor does the
only in-tree dts use it:

arch/mips/boot/dts/easy50712.dts:               lantiq,shadow = <0xfff>;

So in the DTB, lantiq-shadow will be stored as a be32 value. The byte
stream will look like: [00 00 0f ff].

The existing code reads all 4 bytes, and converts them as required,
reading the expected value of 0xfff. If you use of_property_read_u16,
this will only take the first 2 bytes, and will incorrectly read 0.

You can instead use of_property_read_u32, which will function
equivalently to the existing code.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 4/8] gpio-mm-lantiq: Use of_property_read_u32

[Ricardo Ribalda Delgado]

Instead of parsing manually the shadow content, use the much simpler
helper of_property_read_u32.

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: John Crispin <blogic@openwrt.org>
Cc: devicetree@vger.kernel.org
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---

v2: Reported by Mark Rutland <mark.rutland@arm.com>
 -use of_property_read_u32 instead of u16

 drivers/gpio/gpio-mm-lantiq.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpio-mm-lantiq.c b/drivers/gpio/gpio-mm-lantiq.c
index 7d55161..95ff180 100644
--- a/drivers/gpio/gpio-mm-lantiq.c
+++ b/drivers/gpio/gpio-mm-lantiq.c
@@ -105,7 +105,7 @@ static void ltq_mm_save_regs(struct of_mm_gpio_chip *mm_gc)
 static int ltq_mm_probe(struct platform_device *pdev)
 {
 	struct ltq_mm *chip;
-	const __be32 *shadow;
+	u32 shadow;
 
 	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
@@ -117,9 +117,8 @@ static int ltq_mm_probe(struct platform_device *pdev)
 	chip->mmchip.save_regs = ltq_mm_save_regs;
 
 	/* store the shadow value if one was passed by the devicetree */
-	shadow = of_get_property(pdev->dev.of_node, "lantiq,shadow", NULL);
-	if (shadow)
-		chip->shadow = be32_to_cpu(*shadow);
+	if (!of_property_read_u32(pdev->dev.of_node, "lantiq,shadow", &shadow))
+		chip->shadow = shadow;
 
 	return of_mm_gpiochip_add(pdev->dev.of_node, &chip->mmchip);
 }
-- 
2.1.4

Re: [PATCH v2 4/8] gpio-mm-lantiq: Use of_property_read_u32

[Linus Walleij]

On Mon, Jan 19, 2015 at 12:27 PM, Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:

> Instead of parsing manually the shadow content, use the much simpler
> helper of_property_read_u32.
>
> 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: John Crispin <blogic@openwrt.org>
> Cc: devicetree@vger.kernel.org
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>
> v2: Reported by Mark Rutland <mark.rutland@arm.com>
>  -use of_property_read_u32 instead of u16

This v2 version applied.

Yours,
Linus Walleij

Re: [PATCH 4/8] gpio-mm-lantiq: Use of_propery_read_u16

[Ricardo Ribalda Delgado]

Hello Mark

Thanks for noticing. I have just sent the v2 of the patch.

Regards!

On Mon, Jan 19, 2015 at 11:57 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Sun, Jan 18, 2015 at 11:39:29AM +0000, Ricardo Ribalda Delgado wrote:
>> Instead of parsing manually the shadow content, use the much simpler
>> helper of_property_read_u16.
>>
>> 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: John Crispin <blogic@openwrt.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>> ---
>>  drivers/gpio/gpio-mm-lantiq.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-mm-lantiq.c b/drivers/gpio/gpio-mm-lantiq.c
>> index 7d55161..b511a6b 100644
>> --- a/drivers/gpio/gpio-mm-lantiq.c
>> +++ b/drivers/gpio/gpio-mm-lantiq.c
>> @@ -105,7 +105,6 @@ static void ltq_mm_save_regs(struct of_mm_gpio_chip *mm_gc)
>>  static int ltq_mm_probe(struct platform_device *pdev)
>>  {
>>       struct ltq_mm *chip;
>> -     const __be32 *shadow;
>>
>>       chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
>>       if (!chip)
>> @@ -117,9 +116,8 @@ static int ltq_mm_probe(struct platform_device *pdev)
>>       chip->mmchip.save_regs = ltq_mm_save_regs;
>>
>>       /* store the shadow value if one was passed by the devicetree */
>> -     shadow = of_get_property(pdev->dev.of_node, "lantiq,shadow", NULL);
>> -     if (shadow)
>> -             chip->shadow = be32_to_cpu(*shadow);
>> +     of_property_read_u16(pdev->dev.of_node, "lantiq,shadow",
>> +                          &chip->shadow);
>
> The old code and new code aren't equivalent:
>
> The binding doesn't specify the use of /bits/ 16 <...>, nor does the
> only in-tree dts use it:
>
> arch/mips/boot/dts/easy50712.dts:               lantiq,shadow = <0xfff>;
>
> So in the DTB, lantiq-shadow will be stored as a be32 value. The byte
> stream will look like: [00 00 0f ff].
>
> The existing code reads all 4 bytes, and converts them as required,
> reading the expected value of 0xfff. If you use of_property_read_u16,
> this will only take the first 2 bytes, and will incorrectly read 0.
>
> You can instead use of_property_read_u32, which will function
> equivalently to the existing code.
>
> Thanks,
> Mark.



-- 
Ricardo Ribalda

[PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Ricardo Ribalda Delgado]

This way we do not need to transverse the device tree manually.

Cc: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Alexandre Courbot <gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Peter Korsgaard <jacmet-OfajU3CKLf1/SzgSGea1oA@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 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;
 
 	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);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Peter Korsgaard]

>>>>> "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>

-- 
Bye, Peter Korsgaard

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Peter Korsgaard]

>>>>> "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

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Ricardo Ribalda Delgado]

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



-- 
Ricardo Ribalda

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Peter Korsgaard]

>>>>> "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

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Ricardo Ribalda Delgado]

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



-- 
Ricardo Ribalda

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Alexandre Courbot]

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>

Re: [PATCH 7/8] gpio/mpc8xxx: Convert to platform device interface.

[Linus Walleij]

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