Re: [PATCH v3] sh_eth: add device tree support

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 02/11/2014 07:08 PM, Laurent Pinchart wrote:

> Thanks a lot for the patch. DT support for sh-eth was number one on my most
> wanted list :-)

    It's been available for several months already (though just for BOCK-W).
Since the clock DT support turned to be the requirement, I had to change to 
Lager/Koelsch where CCF is already available.

> Please see below for two minor comments.

    Thanks.
    Sigh, I had to scroll thru quite a lot of uncommented text to find them. 
Please trim such text in the future to save the readers' time.

>> Add support of the device tree probing for the Renesas SH-Mobile SoCs
>> documenting the device tree binding as necessary.

>> This work is loosely based on the original patch by Nobuhiro Iwamatsu
>> <nobuhiro.iwamatsu.yj@renesas.com>.

>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

[...]

>> Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
>> ===================================================================
>> --- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
>> +++ net-next/drivers/net/ethernet/renesas/sh_eth.c
[...]
>> @@ -2710,6 +2714,56 @@ static const struct net_device_ops sh_et
>>   	.ndo_change_mtu		= eth_change_mtu,
>>   };
>>
>> +#ifdef CONFIG_OF
>> +static struct sh_eth_plat_data *sh_eth_parse_dt(struct device *dev)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct sh_eth_plat_data *pdata;
>> +	struct device_node *phy;
>> +	const char *mac_addr;
>> +
>> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>> +	if (!pdata)
>> +		return NULL;
>> +
>> +	pdata->phy_interface = of_get_phy_mode(np);
>> +
>> +	phy = of_parse_phandle(np, "phy-handle", 0);
>> +	if (of_property_read_u32(phy, "reg", &pdata->phy)) {
>> +		devm_kfree(dev, pdata);

> No need to free memory here, this will be handled by the core after the
> probe() function fails.

    OK, we should fail the probe when we return NULL from here.

>> +		return NULL;
>> +	}
[...]
>> @@ -2778,7 +2834,19 @@ static int sh_eth_drv_probe(struct platf
>>   	mdp->ether_link_active_low = pd->ether_link_active_low;
>>
>>   	/* set cpu data */
>> -	mdp->cd = (struct sh_eth_cpu_data *)id->driver_data;
>> +	if (id) {
>> +		mdp->cd = (struct sh_eth_cpu_data *)id->driver_data;
>> +	} else	{
>> +		const struct of_device_id *match;
>> +
>> +		match = of_match_device(of_match_ptr(sh_eth_match_table),
>> +					&pdev->dev);
>> +		if (!match) {
>> +			ret = -EINVAL;
>> +			goto out_release;
>> +		}

> You can probably remove error checking, if no match is found the probe
> function wouldn't have been called in the first place.

    Yes, you seem to be correct here as well.

WBR, Sergei