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

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

[Nobuhiro Iwamatsu]

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.
---
 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
+                                not support ether-link, set.
+- sh-eth,ether-link-active-low: Set link check method.
+                                When detection of link is treated as active-low,
+                                set.
+- sh-eth,edmac-big-endian:      Set big endian for sh_eth dmac.
+                                It work as little endian when this is not set.
+
+Example (armadillo800eva):
+	sh-eth@e9a00000 {
+		compatible = "renesas,sh-eth-gigabit";
+		interrupt-parent = <&intca>;
+		reg = <0xe9a00000 0x800>, <0xe9a01800 0x800>;
+		interrupts = <0x500>;
+		phy-mode = "mii";
+		sh-eth,phy-id = <0>;
+	};
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.
  *
  *  This program is free software; you can redistribute it and/or modify it
@@ -31,6 +31,12 @@
 #include <linux/platform_device.h>
 #include <linux/mdio-bitbang.h>
 #include <linux/netdevice.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_net.h>
 #include <linux/phy.h>
 #include <linux/cache.h>
 #include <linux/io.h>
@@ -2340,26 +2346,88 @@ static const struct net_device_ops sh_eth_netdev_ops = {
 	.ndo_change_mtu		= eth_change_mtu,
 };
 
+#ifdef CONFIG_OF
+static struct sh_eth_plat_data *
+sh_eth_parse_dt(struct device *dev, struct net_device *ndev,
+				struct device_node *np)
+{
+	struct sh_eth_plat_data *pdata;
+
+	pdata = devm_kzalloc(dev, sizeof(struct sh_eth_plat_data),
+					GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "%s: failed to allocate config data\n", __func__);
+		return NULL;
+	}
+
+	pdata->phy_interface = of_get_phy_mode(np);
+
+	if (of_property_read_u32(np, "sh-eth,phy-id", &pdata->phy))
+		goto error;
+
+	pdata->edmac_endian =
+		of_property_read_bool(np, "sh-eth,edmac-big-endian");
+
+	pdata->no_ether_link =
+		of_property_read_bool(np, "sh-eth,no-ether-link");
+
+	pdata->ether_link_active_low =
+		of_property_read_bool(np, "sh-eth,ether-link-active-low");
+
+	if (of_device_is_compatible(np, "renesas,sh-eth-gigabit"))
+		pdata->register_type = SH_ETH_REG_GIGABIT;
+	else if (of_device_is_compatible(np, "renesas,sh-eth-sh4"))
+		pdata->register_type = SH_ETH_REG_FAST_SH4;
+	else if (of_device_is_compatible(np, "renesas,sh-eth-sh3-sh2"))
+		pdata->register_type = SH_ETH_REG_FAST_SH3_SH2;
+	else
+		pdata->register_type = -EINVAL;
+
+#ifdef CONFIG_OF_NET
+	if (!is_valid_ether_addr(ndev->dev_addr)) {
+		const char *macaddr = of_get_mac_address(np);
+		if (macaddr)
+			memcpy(pdata->mac_addr, macaddr, ETH_ALEN);
+	}
+#endif
+
+	return pdata;
+
+error:
+	devm_kfree(dev, pdata);
+	return NULL;
+}
+#else
+static inline struct sh_eth_plat_data *
+sh_eth_parse_dt(struct device *dev, struct net_device *ndev,
+				struct device_node *np)
+{
+	return NULL;
+}
+#endif
+
 static int sh_eth_drv_probe(struct platform_device *pdev)
 {
-	int ret, devno = 0;
+	int ret = 0, devno = 0;
 	struct resource *res;
 	struct net_device *ndev = NULL;
 	struct sh_eth_private *mdp = NULL;
 	struct sh_eth_plat_data *pd;
+	struct device_node *np = pdev->dev.of_node;
+
+	ndev = alloc_etherdev(sizeof(struct sh_eth_private));
+	if (!ndev) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
+	mdp = netdev_priv(ndev);
 	/* get base addr */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (unlikely(res == NULL)) {
 		dev_err(&pdev->dev, "invalid resource\n");
 		ret = -EINVAL;
-		goto out;
-	}
-
-	ndev = alloc_etherdev(sizeof(struct sh_eth_private));
-	if (!ndev) {
-		ret = -ENOMEM;
-		goto out;
+		goto out_release;
 	}
 
 	/* The sh Ether-specific entries in the device structure. */
@@ -2376,14 +2444,6 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	}
 	ndev->irq = ret;
 
-	SET_NETDEV_DEV(ndev, &pdev->dev);
-
-	/* Fill in the fields of the device structure with ethernet values. */
-	ether_setup(ndev);
-
-	mdp = netdev_priv(ndev);
-	mdp->num_tx_ring = TX_RING_SIZE;
-	mdp->num_rx_ring = RX_RING_SIZE;
 	mdp->addr = ioremap(res->start, resource_size(res));
 	if (mdp->addr == NULL) {
 		ret = -ENOMEM;
@@ -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;
+		}
+	} else
+		pd = pdev->dev.platform_data;
+
+	if (!pd) {
+		dev_err(&pdev->dev, "no setup data defined\n");
+		ret = -EINVAL;
+		goto out_release;
+	}
+
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	mdp->num_tx_ring = TX_RING_SIZE;
+	mdp->num_rx_ring = RX_RING_SIZE;
+
 	spin_lock_init(&mdp->lock);
 	mdp->pdev = pdev;
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_resume(&pdev->dev);
 
-	pd = (struct sh_eth_plat_data *)(pdev->dev.platform_data);
 	/* get PHY ID */
 	mdp->phy_id = pd->phy;
 	mdp->phy_interface = pd->phy_interface;
@@ -2405,6 +2486,8 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	mdp->no_ether_link = pd->no_ether_link;
 	mdp->ether_link_active_low = pd->ether_link_active_low;
 	mdp->reg_offset = sh_eth_get_register_offset(pd->register_type);
+	/* read and set MAC address */
+	read_mac_address(ndev, pd->mac_addr);
 
 	/* set cpu data */
 #if defined(SH_ETH_HAS_BOTH_MODULES)
@@ -2422,20 +2505,16 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 	/* debug message level */
 	mdp->msg_enable = SH_ETH_DEF_MSG_ENABLE;
 
-	/* read and set MAC address */
-	read_mac_address(ndev, pd->mac_addr);
-
 	/* ioremap the TSU registers */
 	if (mdp->cd->tsu) {
 		struct resource *rtsu;
 		rtsu = platform_get_resource(pdev, IORESOURCE_MEM, 1);
 		if (!rtsu) {
 			dev_err(&pdev->dev, "Not found TSU resource\n");
-			ret = -ENODEV;
 			goto out_release;
 		}
 		mdp->tsu_addr = ioremap(rtsu->start,
-					resource_size(rtsu));
+				resource_size(rtsu));
 		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
 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)
+#else
+#define SH_ETH_PM_OPS NULL
+#endif
+
+#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", },
+	{},
+};
+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,
+		   .of_match_table = of_match_ptr(sh_eth_match),
 	},
 };
 
-- 
1.7.10.4

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

[Sergei Shtylyov]

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

> 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. :-)

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

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

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

> +
> +#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).

> +	{},
> +};
> +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.

WBR, Sergei

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

[Sergei Shtylyov]

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

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

[Nobuhiro Iwamatsu]

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

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

[Sergei Shtylyov]

Hello.

On 21-03-2013 11:03, Nobuhiro Iwamatsu wrote:

    Sorry, I have noticed your reply only last Friday. I probably should do 
something with my mail filters, so that they leave the personal mail in my 
inbox and not toss it to the list folders where I may ignore it...

>>> 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.
>>> ---
[...]

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

    /sdata/code/.

> instead of trying to convert the driver to device tree.

    Convert the platfrom data, I meant. But I already wrote about that I think.

> Yes, I knew about this.

    But still attempted to document and use the data-only device tree 
properties (which was pointless in the light of procediral platfrom data)?

>>> +
>>> +#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've already added it now, it's in the 'net-next' tree.

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

    Yes, this is the step in the right direction. Though I'd drop the '-sh' 
infix -- the driver is usable not only on SH platforms.

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

    Yes, I saw that.

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

> How about this?

    Sounds better. I think however that the conversion of this driver to 
device tree shouldn't be done without getting rid of the current #ifdef mess 
in it (which is still on my agenda). I think that the 'register_type' field 
should move from the platform data to the 'struct sh_eth_cpu_data' in the process.

WBR, Sergei

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

[Nobuhiro Iwamatsu]

Hi,

(2013/04/15 5:06), Sergei Shtylyov wrote:
> Hello.
>
> On 21-03-2013 11:03, Nobuhiro Iwamatsu wrote:
>
> Sorry, I have noticed your reply only last Friday. I probably should do something with my mail filters, so that they leave the personal mail in my inbox and not toss it to the list folders where I may ignore it...
>
>>>> 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.
>>>> ---
> [...]
>
>>>> @@ -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
>
> /sdata/code/.
>
>> instead of trying to convert the driver to device tree.
>
> Convert the platfrom data, I meant. But I already wrote about that I think.
>
>> Yes, I knew about this.
>
> But still attempted to document and use the data-only device tree properties (which was pointless in the light of procediral platfrom data)?
>

Yes, I added this information to next 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've already added it now, it's in the 'net-next' tree.
>

Yes, I know. Thank you for this work.

>> 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.
>
> Yes, this is the step in the right direction. Though I'd drop the '-sh' infix -- the driver is usable not only on SH platforms.
>

OK, I will drop "-sh" from SH platform too.

>> 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.
>
> Yes, I saw that.
>
>> Therefore, the compatible property of this device becomes <CPU>-sh-eth
>> or <CPU>-sh-eth-<ETHER TYPE>.
>
>> How about this?
>
> Sounds better. I think however that the conversion of this driver to device tree shouldn't be done without getting rid of the current #ifdef mess in it (which is still on my agenda). I think that the 'register_type' field should move from the platform data to the 'struct sh_eth_cpu_data' in the process.
>

I see. this problem will fix next step, I think.

> WBR, Sergei
>
>

Best regards,
   Nobuhiro

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

[Sergei Shtylyov]

Hello.

On 15-04-2013 6:17, Nobuhiro Iwamatsu 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.
>>>>> ---
>> [...]

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

>> /sdata/code/.

>>> instead of trying to convert the driver to device tree.

>> Convert the platfrom data, I meant. But I already wrote about that I think.

>>> Yes, I knew about this.

>> But still attempted to document and use the data-only device tree properties
>> (which was pointless in the light of procediral platfrom data)?

> Yes, I added this information to next patch.

    WHich information, I didn't understand?

>>>>> +
>>>>> +#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", },

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

>>> How about this?

>> Sounds better. I think however that the conversion of this driver to device
>> tree shouldn't be done without getting rid of the current #ifdef mess in it
>> (which is still on my agenda). I think that the 'register_type' field should
>> move from the platform data to the 'struct sh_eth_cpu_data' in the process.

> I see. this problem will fix next step, I think.

    No, not at all. If we don't get rid of the #ifdef mess, adding device tree 
support will have to add even more of it.

> Best regards,
>    Nobuhiro

WBR, Sergei

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

[Nobuhiro Iwamatsu]

Hi,

(2013/04/16 3:52), Sergei Shtylyov wrote:
> Hello.
>
> On 15-04-2013 6:17, Nobuhiro Iwamatsu 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.
>>>>>> ---
>>> [...]
>
>>>>>> @@ -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
>
>>> /sdata/code/.
>
>>>> instead of trying to convert the driver to device tree.
>
>>> Convert the platfrom data, I meant. But I already wrote about that I think.
>
>>>> Yes, I knew about this.
>
>>> But still attempted to document and use the data-only device tree properties
>>> (which was pointless in the light of procediral platfrom data)?
>
>> Yes, I added this information to next patch.
>
> WHich information, I didn't understand?

sorry, I was not understand your mail by mistake.
You believe that without OF_DEV_AUXDATA, that it should be fully migrated to the DT from platformdata. right?

>
>>>>>> +
>>>>>> +#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", },
>
>>>> Therefore, the compatible property of this device becomes <CPU>-sh-eth
>>>> or <CPU>-sh-eth-<ETHER TYPE>.
>
>>>> How about this?
>
>>> Sounds better. I think however that the conversion of this driver to device
>>> tree shouldn't be done without getting rid of the current #ifdef mess in it
>>> (which is still on my agenda). I think that the 'register_type' field should
>>> move from the platform data to the 'struct sh_eth_cpu_data' in the process.
>
>> I see. this problem will fix next step, I think.
>
> No, not at all. If we don't get rid of the #ifdef mess, adding device tree support will have to add even more of it.
>

But sh-eth was used by SH and sh-mobile.
I think we can migrate to DT.
However, I think it is not possible to remove idfef completely immediately.

>> Best regards,
>> Nobuhiro
>
> WBR, Sergei
>
>

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

[Sergei Shtylyov]

Hello.

On 17-04-2013 11:54, Nobuhiro Iwamatsu wrote:

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

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

[...]

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

>>>> /sdata/code/.

>>>>> instead of trying to convert the driver to device tree.

>>>> Convert the platfrom data, I meant. But I already wrote about that I think.

>>>>> Yes, I knew about this.

>>>> But still attempted to document and use the data-only device tree properties
>>>> (which was pointless in the light of procediral platfrom data)?

>>> Yes, I added this information to next patch.

>> WHich information, I didn't understand?

> sorry, I was not understand your mail by mistake.
> You believe that without OF_DEV_AUXDATA, that it should be fully migrated to
> the DT from platformdata. right?

    No, the contrary. OF_DEV_AUXDATA should be used and platfrom data 
therefore *not* be convereted to DT propertiies.

>>>>>>> +
>>>>>>> +#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", },

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

>>>>> How about this?

>>>> Sounds better. I think however that the conversion of this driver to device
>>>> tree shouldn't be done without getting rid of the current #ifdef mess in it
>>>> (which is still on my agenda). I think that the 'register_type' field should
>>>> move from the platform data to the 'struct sh_eth_cpu_data' in the process.

>>> I see. this problem will fix next step, I think.

>> No, not at all. If we don't get rid of the #ifdef mess, adding device tree
>> support will have to add even more of it.

> But sh-eth was used by SH and sh-mobile.

    So what?

> I think we can migrate to DT.

    I don't think we should do it before removing the #ifdef's.

> However, I think it is not possible to remove idfef completely immediately.

    Immediately or not quite so, it was the task I was given.

>>> Best regards,
>>> Nobuhiro

WBR, Sergei