Hi Mark,
2014-02-13 3:50 GMT-08:00 Mark Rutland <mark.rutland@xxxxxxx>:
> On Thu, Feb 13, 2014 at 05:29:52AM +0000, Florian Fainelli wrote:
>> This patch adds support for configuring the port multiplexer hardware
>> which resides in front of the GENET Ethernet MAC controller. This allows
>> us to support:
>>
>> - internal PHYs (using drivers/net/phy/bcm7xxx.c)
>> - MoCA PHYs which are an entirely separate hardware block not covered
>> here
>> - external PHYs and switches
>>
>> Note that MoCA and switches are currently supported using the emulated
>> "fixed PHY" driver.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
>> ---
>> Changes since v1:
>> - fixed MDIO crash/warning when Device Tree probing fails
>> - removed the use of priv->phy_type and use priv->phy_interface
>> directly
>>
>> drivers/net/ethernet/broadcom/genet/bcmmii.c | 481 +++++++++++++++++++++++++++
>> 1 file changed, 481 insertions(+)
>> create mode 100644 drivers/net/ethernet/broadcom/genet/bcmmii.c
>
> [...]
>
>> +static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
>> +{
>> + struct device_node *dn = priv->pdev->dev.of_node;
>> + struct device *kdev = &priv->pdev->dev;
>> + struct device_node *mdio_dn;
>> + const __be32 *fixed_link;
>
> This looks a bit odd. Could we not have a common parser for fixed-link
> properties?
Do you remember the fixed-link revamp that Thomas Petazzoni submitted
a while ago? I was planning on using it, but there were still some
disagreements on how to do it, so I ended up using the good old
"fixed-link" property as-is.
>
>> + u32 propval;
>> + int ret, sz;
>> +
>> + mdio_dn = of_get_next_child(dn, NULL);
>> + if (!mdio_dn) {
>> + dev_err(kdev, "unable to find MDIO bus node\n");
>> + return -ENODEV;
>> + }
>
> Could you please check that this is the node you expect (by looking at
> the compatible string list).
Makes sense.
>
>> +
>> + ret = of_mdiobus_register(priv->mii_bus, mdio_dn);
>> + if (ret) {
>> + dev_err(kdev, "failed to register MDIO bus\n");
>> + return ret;
>> + }
>> +
>> + /* Check if we have an internal or external PHY */
>> + priv->phy_dn = of_parse_phandle(dn, "phy-handle", 0);
>> + if (priv->phy_dn) {
>> + if (!of_property_read_u32(priv->phy_dn, "max-speed", &propval))
>> + priv->phy_speed = propval;
>
> Is there no way to find this out without reading values directly off of
> the PHY? It seems like something we should have an abstraction for.
In fact, this is a remnant from when I did not submit the patch doing
this in of_mdiobus_register(), so this is now useless.
>
>> + } else {
>> + /* Read the link speed from the fixed-link property */
>> + fixed_link = of_get_property(dn, "fixed-link", &sz);
>> + if (!fixed_link || sz < sizeof(*fixed_link)) {
>> + ret = -ENODEV;
>> + goto out;
>> + }
>> +
>> + priv->phy_speed = be32_to_cpu(fixed_link[2]);
>
> Similarly can we not have a common fixed-link parser? Or abstraction
> such that you query the phy regardless of what it is and how its binding
> represents this?
I do not think I need this line anymore. I will do some more testing
and will remove this parsing. I do agree that we need a common
fixed-link parser, but that is part of the discussion initiated by
Thomas Petazzoni.
Thanks for the review!
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
