Re: [PATCH v3 3/3] ARM: OMAP2+: Add GPMC DT support for Ethernet child nodes

[Jon Hunter <jon-hunter@ti.com>]

On 04/17/2013 07:05 AM, Javier Martinez Canillas wrote:

...

> Yes, in fact I just realized that for_each_node_by_name() expand to:
> 
> #define for_each_node_by_name(dn, name) \
>         for (dn = of_find_node_by_name(NULL, name); dn; \
>              dn = of_find_node_by_name(dn, name))
> 
> which means that it will search for a node with "name" on the complete
> DeviceTree and this is wrong. It should only search on GPMC childs.

Good catch. I guess we could have flash & ethernet devices connected to
other interfaces such as SPI.

> Could you please test the following patch? If it works for you I'll add a proper
> description and submit it as a PATCH.
> 
> Thanks a lot and best regards,
> Javier
> 
> From d8dab9ae9a0284f17553875c2fddd806d9f6ab2e Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> Date: Wed, 17 Apr 2013 13:50:30 +0200
> Subject: [PATCH] ARM: OMAP2+: only search for GPMC DT child nodes on
>  probe
> 
> Signed-off-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
> ---
>  arch/arm/mach-omap2/gpmc.c |   51 ++++++++++++++++++++-----------------------
>  1 files changed, 24 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
> index ed946df..58e2415 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -1520,35 +1520,32 @@ static int gpmc_probe_dt(struct platform_device *pdev)
>  		return ret;
>  	}
> 
> -	for_each_node_by_name(child, "nand") {
> -		ret = gpmc_probe_nand_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> -		}
> -	}
> -
> -	for_each_node_by_name(child, "onenand") {
> -		ret = gpmc_probe_onenand_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> -		}
> -	}
> +	for_each_child_of_node(pdev->dev.of_node, child) {
> +		if (child->name) {

Minor nit ... how about ...

+		if (!child->name)
			continue;
		
> +			if (of_node_cmp(child->name, "nand") == 0) {
> +				ret = gpmc_probe_nand_child(pdev, child);
> +				if (ret < 0) {
> +					of_node_put(child);
> +					return ret;
> +				}
> +			}
> 
> -	for_each_node_by_name(child, "nor") {
> -		ret = gpmc_probe_generic_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> -		}
> -	}
> +			if (of_node_cmp(child->name, "onenand") == 0) {
> +				ret = gpmc_probe_onenand_child(pdev, child);
> +				if (ret < 0) {
> +					of_node_put(child);
> +					return ret;
> +				}
> +			}
> 
> -	for_each_node_by_name(child, "ethernet") {
> -		ret = gpmc_probe_generic_child(pdev, child);
> -		if (ret < 0) {
> -			of_node_put(child);
> -			return ret;
> +			if (of_node_cmp(child->name, "ethernet") == 0 ||
> +			    of_node_cmp(child->name, "nor") == 0) {
> +				ret = gpmc_probe_generic_child(pdev, child);
> +				if (ret < 0) {
> +					of_node_put(child);
> +					return ret;
> +				}
> +			}
>  		}
>  	}

I am also wondering if the probe should fail if one of its children
fails. The panic that Lars reported occurred because the nand
successfully probed and so the nand device was registered, but because
the ethernet device probe failed, the gpmc probe fails, and then when
the nand device is probed by the mtd driver the kernel panics. I wonder
if it would be better to WARN on child devices that fail to probe but
not return error from the gpmc probe.

Cheers
Jon