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

[Mark Rutland <mark.rutland@arm.com>]

[...]

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