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

[Javier Martinez Canillas <javier.martinez@collabora.co.uk>]

On 04/17/2013 03:48 PM, Jon Hunter wrote:
> 
> 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.
> 

Yes, in fact this is the case for omap4-sdp.dts which has an ethernet controller
connected through SPI:

&mcspi1 {
        eth@0 {
                compatible = "ks8851";
                spi-max-frequency = <24000000>;
                reg = <0>;
                interrupt-parent = <&gpio2>;
                interrupts = <2>; /* gpio line 34 */
                vdd-supply = <&vdd_eth>;
        };
};

it just didn't fail because the device node name is "eth" and not "ethernet".

which makes me wonder if is OK to rely on a device node name or we should use
compatible properties instead such as "ti,gpmc-{eth,nand,nor,onenand}" and do
something like:

for_each_compatible_node(pdev->dev.of_node, NULL, "ti,gpmc-eth") and so on.

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

Yes, much better. I just cooked a quick patch so Lars could test it ;-)
 		
>> +			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.
> 

I wonder the same. Probably is better to just WARN so a user could at lest use
some of the peripherals connected to the GPMC. This is specially important for
flash memory devices since the rootfs could have to be mounted from there.

Do you want me to split the child node search and the WARN_ON() replacement in
two different patchs or should I send both changes in one patch?

> Cheers
> Jon
> 

Best regards,
Javier