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

[Ezequiel Garcia <elezegarcia@gmail.com>]

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