Hello Grant,
Grant Likely wrote:
> On Mon, Jan 23, 2012 at 09:56:04AM +0100, Heiko Schocher wrote:
>> add of support for the davinci_emac driver.
>>
>> Signed-off-by: Heiko Schocher <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: Grant Likely <[email protected]>
>> Cc: Sekhar Nori <[email protected]>
>> Cc: Wolfgang Denk <[email protected]>
>> ---
>> .../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?
>> +- 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.
>> +- 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.
[...]
>> 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 @@
[...]
>> + pdata = pdev->dev.platform_data;
>> + if (!pdata) {
>> + pdata = kzalloc(sizeof(struct emac_platform_data), GFP_KERNEL);
>
> devm_kzalloc()
fixed.
>> + 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.
>> +
>> + 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.
>> +
>> + 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 ...
>> + }
>> + 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
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
