Hello Grant,

Grant Likely wrote:

On Mon, Jan 23, 2012 at 09:56:04AM +0100, Heiko Schocher wrote:

quoted

add of support for the davinci_emac driver.

Signed-off-by: Heiko Schocher <redacted>
Cc: davinci-linux-open-source@linux.davincidsp.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: devicetree-discuss@lists.ozlabs.org
Cc: netdev@vger.kernel.org
Cc: Grant Likely <redacted>
Cc: Sekhar Nori <redacted>
Cc: Wolfgang Denk <redacted>
---
 .../bindings/arm/davinci/davinci_emac.txt          |   46 ++++++++
 drivers/net/ethernet/ti/davinci_emac.c             |  111 +++++++++++++++++++-
 2 files changed, 156 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/davinci/davinci_emac.txt

diff --git a/Documentation/devicetree/bindings/arm/davinci/davinci_emac.txt b/Documentation/devicetree/bindings/arm/davinci/davinci_emac.txt
new file mode 100644
index 0000000..4e5dc8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/davinci/davinci_emac.txt

@@ -0,0 +1,46 @@
+* Texas Instruments Davinci EMAC
+
+This file provides information, what the davice node
+for the davinci_emac interface contain.
+
+Required properties:
+- compatible: "ti,davinci-emac";
+- reg: Offset and length of the register set for the device
+- ctrl_reg_offset: offset to control register
+- ctrl_mod_reg_offset: offset to control module register
+- ctrl_ram_offset: offset to control module ram

Should these be explicit properties, or can they be discerned from the
compatible string (which should include the hardware version; see
below).

Hmm.. I do not know all davinci SoCs ... maybe someone from TI
could answer this? But I think, we could discern this from
the compatible string. I prepare this for v2. Maybe it is Ok,
if I do this only for my hardwareversion and others add this,
if needed? (maybe the better approach, as I can code it, but
have no hw for testing it ... so it maybe is buggy)

Also, any custom properties that are specific to a binding really
should include a vendor prefix ('ti,') to avoid namespace collisions
with common bindings.

Yep, is "ti,davinci-" ok? Also I should use dashes instead
underscores, right?

quoted

+- hw_ram_addr: hardware ram addr

Can this be added as a second tuple in the reg property?

No, if I know this right, this is used for DMA, and also could be RAM.

quoted

+- ctrl_ram_size: size of control module ram
+- version: 1 (EMAC_VERSION_1 for DM644x)
+           2 (EMAC_VERSION_2 for DM646x)

Don't use a version property.  Encode it into the compatible property.  ie:

compatible = "ti,davinci-dm6440-emac"; or
compatible = "ti,davinci-dm6460-emac";

You'll also note that I did not use a 'x' as a wildcard.  It is always
safer to be explicit about the part number.  And have newer parts
claim compatibility with older ones like so:

compatible = "ti,davinci-dm6443-emac", "ti,davinci-dm6440-emac"; (I am
obviously making up numbers here.  Fill in appropriate numbers for
your device.

Ok.

[...]

quoted

diff --git a/drivers/net/ethernet/ti/davinci_emac.c b/drivers/net/ethernet/ti/davinci_emac.c
index 794ac30..cad7a96 100644
--- a/drivers/net/ethernet/ti/davinci_emac.c
+++ b/drivers/net/ethernet/ti/davinci_emac.c

@@ -58,6 +58,12 @@

[...]

quoted

+	pdata = pdev->dev.platform_data;
+	if (!pdata) {
+		pdata = kzalloc(sizeof(struct emac_platform_data), GFP_KERNEL);

devm_kzalloc()

fixed.

quoted

+		if (!pdata)
+			goto nodata;
+	}
+
+	mac_addr = of_get_mac_address(np);
+	if (mac_addr)
+		memcpy(pdata->mac_addr, mac_addr, ETH_ALEN);
+
+	ret = of_property_read_u32(np, "ctrl_reg_offset", &data);
+	if (!ret)
+		pdata->ctrl_reg_offset = data;
+
+	ret = of_property_read_u32(np, "ctrl_mod_reg_offset", &data);
+	if (!ret)
+		pdata->ctrl_mod_reg_offset = data;
+
+	ret = of_property_read_u32(np, "ctrl_ram_offset", &data);
+	if (!ret)
+		pdata->ctrl_ram_offset = data;
+
+	ret = of_property_read_u32(np, "hw_ram_addr", &data);
+	if (!ret)
+		pdata->hw_ram_addr = data;
+
+	ret = of_property_read_u32(np, "ctrl_ram_size", &data);
+	if (!ret)
+		pdata->ctrl_ram_size = data;
+
+	ret = of_property_read_u32(np, "rmii_en", &data);
+	if (!ret)
+		pdata->rmii_en = data;
+
+	ret = of_property_read_u32(np, "version", &data);
+	if (!ret)
+		pdata->version = data;
+
+	ret = of_property_read_u32(np, "no_bd_ram", &data);
+	if (!ret)
+		pdata->ctrl_mod_reg_offset = data;

Not all these properties are mentioned in the documentation.

fixed.

quoted

+
+	priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
+	if (!priv->phy_node)
+		pdata->phy_id = "";
+
+	if (!of_address_to_resource(np, 0, &temp_res))
+		memcpy(&pdev->resource[0], &temp_res, sizeof(struct resource));

Don't use of_address_to_resource.  The platform device resource table
will already be populated by the core code so you don't need to call
it here.  Besides, it is illegal for drivers to overwrite the
pdev->resource[] table.

fix this.

quoted

+
+	index = 0;
+	while (index < 4) {
+		irq = irq_of_parse_and_map(np, index);
+		if (irq > 0) {
+			temp_res.start = irq;
+			temp_res.end = irq;
+			temp_res.flags = IORESOURCE_IRQ;
+			memcpy(&pdev->resource[index + 1], &temp_res,
+				sizeof(struct resource));

Same here, the core code will have already populated the irqs into the
resource table for you.

and this. Currently it don't work after this fix, searching
for the reason ...

quoted

+		}
+		index++;
+	}
+
+	pinmux_np = of_parse_phandle(np, "pinmux-handle", 0);
+	if (pinmux_np)
+		davinci_cfg_reg_of(pinmux_np);
+
+	pdev->dev.platform_data = pdata;
+nodata:
+	return  pdata;
+}
+#else
+static struct emac_platform_data
+	*davinci_emac_of_get_pdata(struct platform_device *pdev,
+	struct emac_priv *priv)
+{
+	return  pdev->dev.platform_data;
+}
+#endif
 /**
  * davinci_emac_probe: EMAC device probe
  * @pdev: The DaVinci EMAC device that we are removing

@@ -1802,7 +1910,7 @@ static int __devinit davinci_emac_probe(struct platform_device *pdev)
 
 	spin_lock_init(&priv->lock);
 
-	pdata = pdev->dev.platform_data;
+	pdata = davinci_emac_of_get_pdata(pdev, priv);
 	if (!pdata) {
 		dev_err(&pdev->dev, "no platform data\n");
 		rc = -ENODEV;

@@ -1811,6 +1919,7 @@ static int __devinit davinci_emac_probe(struct platform_device *pdev)
 
 	/* MAC addr and PHY mask , RMII enable info from platform_data */
 	memcpy(priv->mac_addr, pdata->mac_addr, 6);
+

Nit: There are a few instances of unrelated whitespace changes in this
patch.

fixed.

Thanks for the review!

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany