Re: [PATCH v6] net: sh_eth: Add support of device tree probe

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

Hello.

On 19-03-2013 17:22, Sergei Shtylyov wrote:

>> This adds support of device tree probe for Renesas sh-ether driver.

>> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@renesas.com>

>> V6: - Add renesas,sh-eth-gigabit, renesas,sh-eth-sh4 and
>>        renesas,sh-eth-sh3-sh2 to compatible string.
>>     - Remove sh-eth,register-type. This is supplemented by the
>>        compatible string.
>>      - Use the of_property_read_bool instead of of_find_property.
>>      - Add sanity chheck for of_property_read_u32.
>>      - Update document.
>> V5: - Rewrite sh_eth_parse_dt().
>>        Remove of_device_is_available() and CONFIG_OF from support OF
>>        checking function and re-add empty sh_eth_parse_dt().
>>      - Add CONFIG_PM to definition of dev_pm_ops.
>>     - Add CONFIG_OF to definition of of_device_id.
>> V4: - Remove empty sh_eth_parse_dt().
>> V3: - Remove empty sh_eth_parse_dt().
>> V3: - Removed sentnece of "needs-init" from document.
>> V2: - Removed ether_setup().
>>      - Fixed typo from "sh-etn" to "sh-eth".
>>     - Removed "needs-init" and sh-eth,endian from definition of DT.
>>     - Changed "sh-eth,edmac-endian" instead of "sh-eth,edmac-big-endain"
>>       in definition of DT.

    The version history is usually "hidded" under -- tear line.

>> ---
>>   Documentation/devicetree/bindings/net/sh_ether.txt |   48 +++++++
>>   drivers/net/ethernet/renesas/sh_eth.c              |  145
>> ++++++++++++++++----
>>   2 files changed, 168 insertions(+), 25 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/net/sh_ether.txt

[...]

>> diff --git a/drivers/net/ethernet/renesas/sh_eth.c
>> b/drivers/net/ethernet/renesas/sh_eth.c
>> index 7a6471d..d9df68e 100644
>> --- a/drivers/net/ethernet/renesas/sh_eth.c
>> +++ b/drivers/net/ethernet/renesas/sh_eth.c
[...]
>> @@ -2391,12 +2451,33 @@ static int sh_eth_drv_probe(struct platform_device
>> *pdev)
>>           goto out_release;
>>       }
>>
>> +    if (np) {
>> +        pd = sh_eth_parse_dt(&pdev->dev, ndev, np);
>> +        if (pdev->dev.platform_data && pd) {
>> +            struct sh_eth_plat_data *tmp =
>> +                pdev->dev.platform_data;
>> +            pd->set_mdio_gate = tmp->set_mdio_gate;
>> +            pd->needs_init = tmp->needs_init;

>     OK, so we can't fully convert this driver to the device tree due to
> procedural platform data. I then would advice just using OF_DEV_AUXDATA() in
> the platform data instead of trying to convert the driver to device tree.

    I meant to type "ïnstead of trying to convert the paltform data to device 
tree".

>> @@ -2515,17 +2595,32 @@ static int sh_eth_runtime_nop(struct device *dev)
[...]
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id sh_eth_match[] = {
>> +    { .compatible = "renesas,sh-eth-gigabit", },
>> +    { .compatible = "renesas,sh-eth-sh4", },
>> +    { .compatible = "renesas,sh-eth-sh3-sh2", },

>     Biut this is not really enough: the driver supports much more variations
> of the SH and ARM SoCs all of which have difference not only in register
> layout but also in the registers bits or even presence of the whole register
> blocks. All this IMO should be reflected in the different values of the
> compatible "property". BTW, it seems another register layout and instance

    I forgot to type istance of what: 'sh_eth_my_cpu_data'.

> needs to be added for the R-Car SoCs (instead of the current ugly hack).

WBR, Sergei