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

[Tony Lindgren <tony@atomide.com>]

* 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