Date:	Thu, 21 Mar 2013 16:03:39 +0900
From:	Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.com>
To:	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:	netdev@...r.kernel.org, devicetree-discuss@...ts.ozlabs.org,
	magnus.damm@...il.com, kda@...ux-powerpc.org,
	horms+renesas@...ge.net.au, mark.rutland@....com,
	grant.likely@...retlab.ca
Subject: Re: [PATCH v6] net: sh_eth: Add support of device tree probe

Hi,

Thank you for your comment.

(2013/03/19 22:22), Sergei Shtylyov wrote:
> On 19-03-2013 10:39, Nobuhiro Iwamatsu wrote:
>
>> This adds support of device tree probe for Renesas sh-ether driver.
>
>> Signed-off-by: Nobuhiro Iwamatsu <nobuhiro.iwamatsu.yj@...esas.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.
>> ---
>> 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/Documentation/devicetree/bindings/net/sh_ether.txt b/Documentation/devicetree/bindings/net/sh_ether.txt
>> new file mode 100644
>> index 0000000..d1f961c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/sh_ether.txt
>> @@ -0,0 +1,48 @@
>> +* Renesas Electronics SuperH EMAC
>> +
>> +This file provides information, what the device node
>> +for the sh_eth interface contains.
>> +
>> +Required properties:
>> +- compatible: "renesas,sh-eth-gigabit": If a device has
>> + a function of gigabit, you should
>> + set this.
>> + "renesas,sh-eth-sh4": If this is provided by
>> + SH4, you should set this.
>> + "renesas,sh-eth-sh3-sh2": If this is provided
>> + by SH3 or SH2, you should set this.
>> +
>> +- interrupt-parent: The phandle for the interrupt controller that
>> + services interrupts for this device.
>> +
>> +- reg: Offset and length of the register set for the
>> + device. If device has TSU registers, you need
>> + to set up two register information here.
>> +
>> +- interrupts: Interrupt mapping for the sh_eth interrupt
>> + sources (vector id). You can set one value.
>> +
>> +- phy-mode: String, operation mode of the PHY interface
>> + (a string that of_get_phy_mode() can understand).
>> +
>> +- sh-eth,phy-id: PHY id.
>> +
>> +Optional properties:
>> +- local-mac-address: 6 bytes, mac address
>> +- sh-eth,no-ether-link: Set link control by software. When device does
>
> I got the impression that usually the vendor name, not driver name is used as a property prefix.

Ok, I will change to vendor name.

>
>> 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
>> @@ -1,7 +1,7 @@
>> /*
>> * SuperH Ethernet device driver
>> *
>> - * Copyright (C) 2006-2012 Nobuhiro Iwamatsu
>> + * Copyright (C) 2006-2013 Nobuhiro Iwamatsu
>> * Copyright (C) 2008-2012 Renesas Solutions Corp.
>
> Don't you want to extend the copyright of Renesas also -- you seem to be still working there. :-)
>

Thanks, I will fix.

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

Yes, I knew about this.

>
> [...]
>> @@ -2422,20 +2505,16 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
> [...]
>> mdp->tsu_addr = ioremap(rtsu->start,
>> - resource_size(rtsu));
>> + resource_size(rtsu));
>
> Why? It was indented perfectly before. Anyway, you shouldn't be doing collateral whitespace changes.
>

Ok, I will remove this change from this patch.

>> mdp->port = devno % 2;
>> ndev->features = NETIF_F_HW_VLAN_FILTER;
>> }
>> @@ -2502,6 +2581,7 @@ static int sh_eth_drv_remove(struct platform_device *pdev)
>> return 0;
>> }
>>
>> +#ifdef CONFIG_PM
>
> Unrelated change.
>
>> static int sh_eth_runtime_nop(struct device *dev)
>> {
>> /*
>> @@ -2515,17 +2595,32 @@ static int sh_eth_runtime_nop(struct device *dev)
>> return 0;
>> }
>>
>> -static struct dev_pm_ops sh_eth_dev_pm_ops = {
>> +static const struct dev_pm_ops sh_eth_dev_pm_ops = {
>> .runtime_suspend = sh_eth_runtime_nop,
>> .runtime_resume = sh_eth_runtime_nop,
>> };
>> +#define SH_ETH_PM_OPS (&sh_eth_dev_pm_ops)
>
> () not needed.
>
>> +#else
>> +#define SH_ETH_PM_OPS NULL
>> +#endif
>
> Unrelated change. Should be in a different patch.

Yes, I will remove these changes from this patch.

>
>> +
>> +#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 needs to be added
 > for the R-Car SoCs (instead of the current ugly hack).

I see.
Latest source code was defined compatible as renesas,sh-eth-gigabit,
sh-eth-sh4 and sh-eth-sh3-sh2. I will change ito renesas,<CPU>-sh-eth.
And I think that we should define the sh7757-sh-eth-gitabit and
sh7757-sh-eth-fast for this. Becauase sh7757 is special device.
There is a device that supports only devices that support fast
ether and gigabit ether on single CPU.

Therefore, the compatible property of this device becomes <CPU>-sh-eth
or <CPU>-sh-eth-<ETHER TYPE>.

How about this?

>
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, sh_eth_match);
>> +#endif
>>
>> static struct platform_driver sh_eth_driver = {
>> .probe = sh_eth_drv_probe,
>> .remove = sh_eth_drv_remove,
>> .driver = {
>> .name = CARDNAME,
>> - .pm = &sh_eth_dev_pm_ops,
>> + .pm = SH_ETH_PM_OPS,
>
> Unrelated as well.
>

Yes, this is too.

> WBR, Sergei
>
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists