[PATCH v2 1/3] mtd: omap-onenand: pass device_node in platform data

[PATCH v2 1/3] mtd: omap-onenand: pass device_node in platform data

[Ezequiel Garcia]

Pass an optional device_node pointer in the platform data,
which in turn will be put into a mtd_part_parser_data.
This way, code that sets up the platform devices can pass
along the node from DT so that the partitions can be parsed.

For non-DT boards, this change has no effect.

Acked-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/onenand/omap2.c                     |    4 +++-
 include/linux/platform_data/mtd-onenand-omap2.h |    3 +++
 2 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
index 065f3fe..eec2aed 100644
--- a/drivers/mtd/onenand/omap2.c
+++ b/drivers/mtd/onenand/omap2.c
@@ -637,6 +637,7 @@ static int omap2_onenand_probe(struct platform_device *pdev)
 	struct onenand_chip *this;
 	int r;
 	struct resource *res;
+	struct mtd_part_parser_data ppdata = {};
 
 	pdata = pdev->dev.platform_data;
 	if (pdata == NULL) {
@@ -767,7 +768,8 @@ static int omap2_onenand_probe(struct platform_device *pdev)
 	if ((r = onenand_scan(&c->mtd, 1)) < 0)
 		goto err_release_regulator;
 
-	r = mtd_device_parse_register(&c->mtd, NULL, NULL,
+	ppdata.of_node = pdata->of_node;
+	r = mtd_device_parse_register(&c->mtd, NULL, &ppdata,
 				      pdata ? pdata->parts : NULL,
 				      pdata ? pdata->nr_parts : 0);
 	if (r)
diff --git a/include/linux/platform_data/mtd-onenand-omap2.h b/include/linux/platform_data/mtd-onenand-omap2.h
index 685af7e..e9a9fb1 100644
--- a/include/linux/platform_data/mtd-onenand-omap2.h
+++ b/include/linux/platform_data/mtd-onenand-omap2.h
@@ -29,5 +29,8 @@ struct omap_onenand_platform_data {
 	u8			flags;
 	u8			regulator_can_sleep;
 	u8			skip_initial_unlocking;
+
+	/* for passing the partitions */
+	struct device_node	*of_node;
 };
 #endif
-- 
1.7.8.6

[PATCH v2 2/3] arm: omap2: gpmc-onenand: drop __init annotation

[Ezequiel Garcia]

gpmc_onenand_init() will be called from another driver's probe() function,
so drop the __init annotation, in order to prevent section mismatches.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 arch/arm/mach-omap2/gpmc-onenand.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index 94a349e..fadd8743 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -356,7 +356,7 @@ static int gpmc_onenand_setup(void __iomem *onenand_base, int *freq_ptr)
 	return ret;
 }
 
-void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
+void gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 {
 	int err;
 
-- 
1.7.8.6

[PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Ezequiel Garcia]

This patch adds device tree bindings for OMAP OneNAND devices.
Tested on an OMAP3 3430 IGEPv2 board.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
Changes from v1:
 * Fix typo in Documentation/devicetree/bindings/mtd/gpmc-onenand.txt

 .../devicetree/bindings/mtd/gpmc-onenand.txt       |   43 +++++++++++++++++++
 arch/arm/mach-omap2/gpmc.c                         |   44 ++++++++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-onenand.txt

diff --git a/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt
new file mode 100644
index 0000000..deec9da
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpmc-onenand.txt
@@ -0,0 +1,43 @@
+Device tree bindings for GPMC connected OneNANDs
+
+GPMC connected OneNAND (found on OMAP boards) are represented as child nodes of
+the GPMC controller with a name of "onenand".
+
+All timing relevant properties as well as generic gpmc child properties are
+explained in a separate documents - please refer to
+Documentation/devicetree/bindings/bus/ti-gpmc.txt
+
+Required properties:
+
+ - reg:			The CS line the peripheral is connected to
+
+Optional properties:
+
+ - dma-channel:		DMA Channel index
+
+For inline partiton table parsing (optional):
+
+ - #address-cells: should be set to 1
+ - #size-cells: should be set to 1
+
+Example for an OMAP3430 board:
+
+	gpmc: gpmc@6e000000 {
+		compatible = "ti,omap3430-gpmc";
+		ti,hwmods = "gpmc";
+		reg = <0x6e000000 0x1000000>;
+		interrupts = <20>;
+		gpmc,num-cs = <8>;
+		gpmc,num-waitpins = <4>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+
+		onenand@0 {
+			reg = <0 0 0>; /* CS0, offset 0 */
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			/* partitions go here */
+		};
+	};
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 01ce462..f7de9eb 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -39,6 +39,7 @@
 #include "omap_device.h"
 #include "gpmc.h"
 #include "gpmc-nand.h"
+#include "gpmc-onenand.h"
 
 #define	DEVICE_NAME		"omap-gpmc"
 
@@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
 }
 #endif
 
+#ifdef CONFIG_MTD_ONENAND
+static int gpmc_probe_onenand_child(struct platform_device *pdev,
+				 struct device_node *child)
+{
+	u32 val;
+	struct omap_onenand_platform_data *gpmc_onenand_data;
+
+	if (of_property_read_u32(child, "reg", &val) < 0) {
+		dev_err(&pdev->dev, "%s has no 'reg' property\n",
+			child->full_name);
+		return -ENODEV;
+	}
+
+	gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
+					 GFP_KERNEL);
+	if (!gpmc_onenand_data)
+		return -ENOMEM;
+
+	gpmc_onenand_data->cs = val;
+	gpmc_onenand_data->of_node = child;
+	gpmc_onenand_data->dma_channel = -1;
+
+	if (!of_property_read_u32(child, "dma-channel", &val))
+		gpmc_onenand_data->dma_channel = val;
+
+	gpmc_onenand_init(gpmc_onenand_data);
+
+	return 0;
+}
+#else
+static int gpmc_probe_onenand_child(struct platform_device *pdev,
+				    struct device_node *child)
+{
+	return 0;
+}
+#endif
+
 static int gpmc_probe_dt(struct platform_device *pdev)
 {
 	int ret;
@@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct platform_device *pdev)
 			return ret;
 	}
 
+	for_each_node_by_name(child, "onenand") {
+		ret = gpmc_probe_onenand_child(pdev, child);
+		of_node_put(child);
+		if (ret < 0)
+			return ret;
+	}
 	return 0;
 }
 #else
-- 
1.7.8.6

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Mark Rutland]

[...]

> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index 01ce462..f7de9eb 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -39,6 +39,7 @@
>  #include "omap_device.h"
>  #include "gpmc.h"
>  #include "gpmc-nand.h"
> +#include "gpmc-onenand.h"
>  
>  #define	DEVICE_NAME		"omap-gpmc"
>  
> @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>  }
>  #endif
>  
> +#ifdef CONFIG_MTD_ONENAND
> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
> +				 struct device_node *child)
> +{
> +	u32 val;
> +	struct omap_onenand_platform_data *gpmc_onenand_data;
> +
> +	if (of_property_read_u32(child, "reg", &val) < 0) {
> +		dev_err(&pdev->dev, "%s has no 'reg' property\n",
> +			child->full_name);
> +		return -ENODEV;
> +	}
> +
> +	gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
> +					 GFP_KERNEL);
> +	if (!gpmc_onenand_data)
> +		return -ENOMEM;
> +
> +	gpmc_onenand_data->cs = val;
> +	gpmc_onenand_data->of_node = child;
> +	gpmc_onenand_data->dma_channel = -1;
> +
> +	if (!of_property_read_u32(child, "dma-channel", &val))
> +		gpmc_onenand_data->dma_channel = val;
> +
> +	gpmc_onenand_init(gpmc_onenand_data);
> +
> +	return 0;
> +}
> +#else
> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
> +				    struct device_node *child)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int gpmc_probe_dt(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>  			return ret;
>  	}
>  

This doesn't look right to me:

> +	for_each_node_by_name(child, "onenand") {
> +		ret = gpmc_probe_onenand_child(pdev, child);
> +		of_node_put(child);
> +		if (ret < 0)
> +			return ret;
> +	}

for_each_node_by_name automatically calls of_node_put on each node once passed,
and as far as I can tell, gpmc_probe_onenand_child doesn't do anything that'd
increment a node's refcount.

As far as I can see, you only need the of_node_put in the error case:

for_each_node_by_name(child, "onenand") {
	ret = gpmc_probe_onenand_child(pdev, child);
	if (ret < 0) {
		of_node_put(child);
		return ret;
	}
}

Have I missed something here?

>  	return 0;
>  }
>  #else
> -- 
> 1.7.8.6
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

Thanks,
Mark.

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Ezequiel Garcia]

Hi Mark,

On Mon, Jan 21, 2013 at 9:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> [...]
>
>> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
>> index 01ce462..f7de9eb 100644
>> --- a/arch/arm/mach-omap2/gpmc.c
>> +++ b/arch/arm/mach-omap2/gpmc.c
>> @@ -39,6 +39,7 @@
>>  #include "omap_device.h"
>>  #include "gpmc.h"
>>  #include "gpmc-nand.h"
>> +#include "gpmc-onenand.h"
>>
>>  #define      DEVICE_NAME             "omap-gpmc"
>>
>> @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
>>  }
>>  #endif
>>
>> +#ifdef CONFIG_MTD_ONENAND
>> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
>> +                              struct device_node *child)
>> +{
>> +     u32 val;
>> +     struct omap_onenand_platform_data *gpmc_onenand_data;
>> +
>> +     if (of_property_read_u32(child, "reg", &val) < 0) {
>> +             dev_err(&pdev->dev, "%s has no 'reg' property\n",
>> +                     child->full_name);
>> +             return -ENODEV;
>> +     }
>> +
>> +     gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
>> +                                      GFP_KERNEL);
>> +     if (!gpmc_onenand_data)
>> +             return -ENOMEM;
>> +
>> +     gpmc_onenand_data->cs = val;
>> +     gpmc_onenand_data->of_node = child;
>> +     gpmc_onenand_data->dma_channel = -1;
>> +
>> +     if (!of_property_read_u32(child, "dma-channel", &val))
>> +             gpmc_onenand_data->dma_channel = val;
>> +
>> +     gpmc_onenand_init(gpmc_onenand_data);
>> +
>> +     return 0;
>> +}
>> +#else
>> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
>> +                                 struct device_node *child)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  static int gpmc_probe_dt(struct platform_device *pdev)
>>  {
>>       int ret;
>> @@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>>                       return ret;
>>       }
>>
>
> This doesn't look right to me:
>
>> +     for_each_node_by_name(child, "onenand") {
>> +             ret = gpmc_probe_onenand_child(pdev, child);
>> +             of_node_put(child);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }
>
> for_each_node_by_name automatically calls of_node_put on each node once passed,
> and as far as I can tell, gpmc_probe_onenand_child doesn't do anything that'd
> increment a node's refcount.
>
> As far as I can see, you only need the of_node_put in the error case:
>
> for_each_node_by_name(child, "onenand") {
>         ret = gpmc_probe_onenand_child(pdev, child);
>         if (ret < 0) {
>                 of_node_put(child);
>                 return ret;
>         }
> }
>
> Have I missed something here?
>

Mmm... perhaps I've overlooked that code.

After some digging through source and reading for_each_node_by_name()
it seems to me you're right.

@Daniel: It seems this would also apply to the NAND binding.
What do you think?

-- 
    Ezequiel

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Tony Lindgren]

* Ezequiel Garcia <elezegarcia@gmail.com> [130121 09:00]:
> Hi Mark,
> 
> On Mon, Jan 21, 2013 at 9:30 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > [...]
> >
> >> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> >> index 01ce462..f7de9eb 100644
> >> --- a/arch/arm/mach-omap2/gpmc.c
> >> +++ b/arch/arm/mach-omap2/gpmc.c
> >> @@ -39,6 +39,7 @@
> >>  #include "omap_device.h"
> >>  #include "gpmc.h"
> >>  #include "gpmc-nand.h"
> >> +#include "gpmc-onenand.h"
> >>
> >>  #define      DEVICE_NAME             "omap-gpmc"
> >>
> >> @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct platform_device *pdev,
> >>  }
> >>  #endif
> >>
> >> +#ifdef CONFIG_MTD_ONENAND
> >> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
> >> +                              struct device_node *child)
> >> +{
> >> +     u32 val;
> >> +     struct omap_onenand_platform_data *gpmc_onenand_data;
> >> +
> >> +     if (of_property_read_u32(child, "reg", &val) < 0) {
> >> +             dev_err(&pdev->dev, "%s has no 'reg' property\n",
> >> +                     child->full_name);
> >> +             return -ENODEV;
> >> +     }
> >> +
> >> +     gpmc_onenand_data = devm_kzalloc(&pdev->dev, sizeof(*gpmc_onenand_data),
> >> +                                      GFP_KERNEL);
> >> +     if (!gpmc_onenand_data)
> >> +             return -ENOMEM;
> >> +
> >> +     gpmc_onenand_data->cs = val;
> >> +     gpmc_onenand_data->of_node = child;
> >> +     gpmc_onenand_data->dma_channel = -1;
> >> +
> >> +     if (!of_property_read_u32(child, "dma-channel", &val))
> >> +             gpmc_onenand_data->dma_channel = val;
> >> +
> >> +     gpmc_onenand_init(gpmc_onenand_data);
> >> +
> >> +     return 0;
> >> +}
> >> +#else
> >> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
> >> +                                 struct device_node *child)
> >> +{
> >> +     return 0;
> >> +}
> >> +#endif
> >> +
> >>  static int gpmc_probe_dt(struct platform_device *pdev)
> >>  {
> >>       int ret;
> >> @@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct platform_device *pdev)
> >>                       return ret;
> >>       }
> >>
> >
> > This doesn't look right to me:
> >
> >> +     for_each_node_by_name(child, "onenand") {
> >> +             ret = gpmc_probe_onenand_child(pdev, child);
> >> +             of_node_put(child);
> >> +             if (ret < 0)
> >> +                     return ret;
> >> +     }
> >
> > for_each_node_by_name automatically calls of_node_put on each node once passed,
> > and as far as I can tell, gpmc_probe_onenand_child doesn't do anything that'd
> > increment a node's refcount.
> >
> > As far as I can see, you only need the of_node_put in the error case:
> >
> > for_each_node_by_name(child, "onenand") {
> >         ret = gpmc_probe_onenand_child(pdev, child);
> >         if (ret < 0) {
> >                 of_node_put(child);
> >                 return ret;
> >         }
> > }
> >
> > Have I missed something here?
> >
> 
> Mmm... perhaps I've overlooked that code.
> 
> After some digging through source and reading for_each_node_by_name()
> it seems to me you're right.
> 
> @Daniel: It seems this would also apply to the NAND binding.
> What do you think?

Would prefer this done as a fix against the omap-for-v3.9/gpmc
branch before we apply Ezequiel's patches.

Regards,

Tony

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Daniel Mack]

Hi Tony, Mark, Ezequiel,

Tony Lindgren <tony@atomide.com> wrote:

>* Ezequiel Garcia <elezegarcia@gmail.com> [130121 09:00]:
>> Hi Mark,
>> 
>> On Mon, Jan 21, 2013 at 9:30 AM, Mark Rutland <mark.rutland@arm.com>
>wrote:
>> > [...]
>> >
>> >> diff --git a/arch/arm/mach-omap2/gpmc.c
>b/arch/arm/mach-omap2/gpmc.c
>> >> index 01ce462..f7de9eb 100644
>> >> --- a/arch/arm/mach-omap2/gpmc.c
>> >> +++ b/arch/arm/mach-omap2/gpmc.c
>> >> @@ -39,6 +39,7 @@
>> >>  #include "omap_device.h"
>> >>  #include "gpmc.h"
>> >>  #include "gpmc-nand.h"
>> >> +#include "gpmc-onenand.h"
>> >>
>> >>  #define      DEVICE_NAME             "omap-gpmc"
>> >>
>> >> @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct
>platform_device *pdev,
>> >>  }
>> >>  #endif
>> >>
>> >> +#ifdef CONFIG_MTD_ONENAND
>> >> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
>> >> +                              struct device_node *child)
>> >> +{
>> >> +     u32 val;
>> >> +     struct omap_onenand_platform_data *gpmc_onenand_data;
>> >> +
>> >> +     if (of_property_read_u32(child, "reg", &val) < 0) {
>> >> +             dev_err(&pdev->dev, "%s has no 'reg' property\n",
>> >> +                     child->full_name);
>> >> +             return -ENODEV;
>> >> +     }
>> >> +
>> >> +     gpmc_onenand_data = devm_kzalloc(&pdev->dev,
>sizeof(*gpmc_onenand_data),
>> >> +                                      GFP_KERNEL);
>> >> +     if (!gpmc_onenand_data)
>> >> +             return -ENOMEM;
>> >> +
>> >> +     gpmc_onenand_data->cs = val;
>> >> +     gpmc_onenand_data->of_node = child;
>> >> +     gpmc_onenand_data->dma_channel = -1;
>> >> +
>> >> +     if (!of_property_read_u32(child, "dma-channel", &val))
>> >> +             gpmc_onenand_data->dma_channel = val;
>> >> +
>> >> +     gpmc_onenand_init(gpmc_onenand_data);
>> >> +
>> >> +     return 0;
>> >> +}
>> >> +#else
>> >> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
>> >> +                                 struct device_node *child)
>> >> +{
>> >> +     return 0;
>> >> +}
>> >> +#endif
>> >> +
>> >>  static int gpmc_probe_dt(struct platform_device *pdev)
>> >>  {
>> >>       int ret;
>> >> @@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct
>platform_device *pdev)
>> >>                       return ret;
>> >>       }
>> >>
>> >
>> > This doesn't look right to me:
>> >
>> >> +     for_each_node_by_name(child, "onenand") {
>> >> +             ret = gpmc_probe_onenand_child(pdev, child);
>> >> +             of_node_put(child);
>> >> +             if (ret < 0)
>> >> +                     return ret;
>> >> +     }
>> >
>> > for_each_node_by_name automatically calls of_node_put on each node
>once passed,
>> > and as far as I can tell, gpmc_probe_onenand_child doesn't do
>anything that'd
>> > increment a node's refcount.
>> >
>> > As far as I can see, you only need the of_node_put in the error
>case:
>> >
>> > for_each_node_by_name(child, "onenand") {
>> >         ret = gpmc_probe_onenand_child(pdev, child);
>> >         if (ret < 0) {
>> >                 of_node_put(child);
>> >                 return ret;
>> >         }
>> > }
>> >
>> > Have I missed something here?
>> >
>> 
>> Mmm... perhaps I've overlooked that code.
>> 
>> After some digging through source and reading for_each_node_by_name()
>> it seems to me you're right.
>> 
>> @Daniel: It seems this would also apply to the NAND binding.
>> What do you think?
>
>Would prefer this done as a fix against the omap-for-v3.9/gpmc
>branch before we apply Ezequiel's patches.

I'm currently far away from my computer and can't prepare a patch for this, sorry. But I think you are right, so please just submit a patch for that, anyone :-)

Best regards,
Daniel

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Ezequiel Garcia]

On Mon, Jan 21, 2013 at 10:32 PM, Daniel Mack <zonque@gmail.com> wrote:
> Hi Tony, Mark, Ezequiel,
>
> Tony Lindgren <tony@atomide.com> wrote:
>
>>* Ezequiel Garcia <elezegarcia@gmail.com> [130121 09:00]:
>>> Hi Mark,
>>>
>>> On Mon, Jan 21, 2013 at 9:30 AM, Mark Rutland <mark.rutland@arm.com>
>>wrote:
>>> > [...]
>>> >
>>> >> diff --git a/arch/arm/mach-omap2/gpmc.c
>>b/arch/arm/mach-omap2/gpmc.c
>>> >> index 01ce462..f7de9eb 100644
>>> >> --- a/arch/arm/mach-omap2/gpmc.c
>>> >> +++ b/arch/arm/mach-omap2/gpmc.c
>>> >> @@ -39,6 +39,7 @@
>>> >>  #include "omap_device.h"
>>> >>  #include "gpmc.h"
>>> >>  #include "gpmc-nand.h"
>>> >> +#include "gpmc-onenand.h"
>>> >>
>>> >>  #define      DEVICE_NAME             "omap-gpmc"
>>> >>
>>> >> @@ -1259,6 +1260,43 @@ static int gpmc_probe_nand_child(struct
>>platform_device *pdev,
>>> >>  }
>>> >>  #endif
>>> >>
>>> >> +#ifdef CONFIG_MTD_ONENAND
>>> >> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>> >> +                              struct device_node *child)
>>> >> +{
>>> >> +     u32 val;
>>> >> +     struct omap_onenand_platform_data *gpmc_onenand_data;
>>> >> +
>>> >> +     if (of_property_read_u32(child, "reg", &val) < 0) {
>>> >> +             dev_err(&pdev->dev, "%s has no 'reg' property\n",
>>> >> +                     child->full_name);
>>> >> +             return -ENODEV;
>>> >> +     }
>>> >> +
>>> >> +     gpmc_onenand_data = devm_kzalloc(&pdev->dev,
>>sizeof(*gpmc_onenand_data),
>>> >> +                                      GFP_KERNEL);
>>> >> +     if (!gpmc_onenand_data)
>>> >> +             return -ENOMEM;
>>> >> +
>>> >> +     gpmc_onenand_data->cs = val;
>>> >> +     gpmc_onenand_data->of_node = child;
>>> >> +     gpmc_onenand_data->dma_channel = -1;
>>> >> +
>>> >> +     if (!of_property_read_u32(child, "dma-channel", &val))
>>> >> +             gpmc_onenand_data->dma_channel = val;
>>> >> +
>>> >> +     gpmc_onenand_init(gpmc_onenand_data);
>>> >> +
>>> >> +     return 0;
>>> >> +}
>>> >> +#else
>>> >> +static int gpmc_probe_onenand_child(struct platform_device *pdev,
>>> >> +                                 struct device_node *child)
>>> >> +{
>>> >> +     return 0;
>>> >> +}
>>> >> +#endif
>>> >> +
>>> >>  static int gpmc_probe_dt(struct platform_device *pdev)
>>> >>  {
>>> >>       int ret;
>>> >> @@ -1276,6 +1314,12 @@ static int gpmc_probe_dt(struct
>>platform_device *pdev)
>>> >>                       return ret;
>>> >>       }
>>> >>
>>> >
>>> > This doesn't look right to me:
>>> >
>>> >> +     for_each_node_by_name(child, "onenand") {
>>> >> +             ret = gpmc_probe_onenand_child(pdev, child);
>>> >> +             of_node_put(child);
>>> >> +             if (ret < 0)
>>> >> +                     return ret;
>>> >> +     }
>>> >
>>> > for_each_node_by_name automatically calls of_node_put on each node
>>once passed,
>>> > and as far as I can tell, gpmc_probe_onenand_child doesn't do
>>anything that'd
>>> > increment a node's refcount.
>>> >
>>> > As far as I can see, you only need the of_node_put in the error
>>case:
>>> >
>>> > for_each_node_by_name(child, "onenand") {
>>> >         ret = gpmc_probe_onenand_child(pdev, child);
>>> >         if (ret < 0) {
>>> >                 of_node_put(child);
>>> >                 return ret;
>>> >         }
>>> > }
>>> >
>>> > Have I missed something here?
>>> >
>>>
>>> Mmm... perhaps I've overlooked that code.
>>>
>>> After some digging through source and reading for_each_node_by_name()
>>> it seems to me you're right.
>>>
>>> @Daniel: It seems this would also apply to the NAND binding.
>>> What do you think?
>>
>>Would prefer this done as a fix against the omap-for-v3.9/gpmc
>>branch before we apply Ezequiel's patches.
>
> I'm currently far away from my computer and can't prepare a patch for this, sorry. But I think you are right, so please just submit a patch for that, anyone :-)
>

Ok, I'll try to submit a patch as soon as possible. If anyone wants to
do it instead, fine by me.

-- 
    Ezequiel

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Tony Lindgren]

* Ezequiel Garcia <elezegarcia@gmail.com> [130122 10:17]:
> On Mon, Jan 21, 2013 at 10:32 PM, Daniel Mack <zonque@gmail.com> wrote:
> >
> > I'm currently far away from my computer and can't prepare a patch for this, sorry. But I think you are right, so please just submit a patch for that, anyone :-)
> >
> 
> Ok, I'll try to submit a patch as soon as possible. If anyone wants to
> do it instead, fine by me.

No please go ahead as it seems that you can easily test it too.

Regards,

Tony

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Ezequiel Garcia]

On Tue, Jan 22, 2013 at 3:27 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Ezequiel Garcia <elezegarcia@gmail.com> [130122 10:17]:
>> On Mon, Jan 21, 2013 at 10:32 PM, Daniel Mack <zonque@gmail.com> wrote:
>> >
>> > I'm currently far away from my computer and can't prepare a patch for this, sorry. But I think you are right, so please just submit a patch for that, anyone :-)
>> >
>>
>> Ok, I'll try to submit a patch as soon as possible. If anyone wants to
>> do it instead, fine by me.
>
> No please go ahead as it seems that you can easily test it too.
>

No problem.

I now wonder if it's okey to exit upon probe failure.
In particular, the for_each should be like this:

        for_each_node_by_name(child, "nand") {
                ret = gpmc_probe_nand_child(pdev, child);
                if (ret < 0) {
                        of_node_put(child);
                        return ret;
                }
        }

or like this:

        for_each_node_by_name(child, "nand") {
                ret = gpmc_probe_nand_child(pdev, child);
                WARN_ON(ret < 0);
        }

Ideas?

-- 
    Ezequiel

Re: [PATCH v2 3/3] arm: omap2: gpmc: add DT bindings for OneNAND

[Tony Lindgren]

* Ezequiel Garcia <elezegarcia@gmail.com> [130122 11:46]:
> On Tue, Jan 22, 2013 at 3:27 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Ezequiel Garcia <elezegarcia@gmail.com> [130122 10:17]:
> >> On Mon, Jan 21, 2013 at 10:32 PM, Daniel Mack <zonque@gmail.com> wrote:
> >> >
> >> > I'm currently far away from my computer and can't prepare a patch for this, sorry. But I think you are right, so please just submit a patch for that, anyone :-)
> >> >
> >>
> >> Ok, I'll try to submit a patch as soon as possible. If anyone wants to
> >> do it instead, fine by me.
> >
> > No please go ahead as it seems that you can easily test it too.
> >
> 
> No problem.
> 
> I now wonder if it's okey to exit upon probe failure.
> In particular, the for_each should be like this:
> 
>         for_each_node_by_name(child, "nand") {
>                 ret = gpmc_probe_nand_child(pdev, child);
>                 if (ret < 0) {
>                         of_node_put(child);
>                         return ret;
>                 }
>         }
> 
> or like this:
> 
>         for_each_node_by_name(child, "nand") {
>                 ret = gpmc_probe_nand_child(pdev, child);
>                 WARN_ON(ret < 0);
>         }
> 
> Ideas?

Well I would return and make sure the resources are freed.

However, if this relates to using bootloader configured values
for the few cases where we don't have the timing information
for calculations available, then just doing a warning is
the way to go.

Regards,

Tony