Hi Tony, Mark, Ezequiel,
Tony Lindgren <tony@xxxxxxxxxxx> wrote:
>* Ezequiel Garcia <elezegarcia@xxxxxxxxx> [130121 09:00]:
>> Hi Mark,
>>
>> On Mon, Jan 21, 2013 at 9:30 AM, Mark Rutland <mark.rutland@xxxxxxx>
>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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
