Re: [PATCH] [V5] net: emaclite: adding MDIO and phy lib support

[Grant Likely <grant.likely@secretlab.ca>]

On Thu, Feb 11, 2010 at 3:12 PM, John Linn <john.linn@xilinx.com> wrote:
> These changes add MDIO and phy lib support to the driver as the
> IP core now supports the MDIO bus.
>
> The MDIO bus and phy are added as a child to the emaclite in the device
> tree as illustrated below.
>
> mdio {
>        #address-cells = <1>;
>        #size-cells = <0>;
>        phy0: phy@7 {
>                compatible = "marvell,88e1111";
>                reg = <7>;
>        } ;
> }
>
> Signed-off-by: Sadanand Mutyala <Sadanand.Mutyala@xilinx.com>
> Signed-off-by: John Linn <john.linn@xilinx.com>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

I've noticed one more problem below, but I'm okay with this being
merged as-is and fixed up with a follow-on patch later.

g.

> +/**
> + * xemaclite_mdio_setup - Register mii_bus for the Emaclite device
> + * @lp:                Pointer to the Emaclite device private data
> + * @ofdev:     Pointer to OF device structure
> + *
> + * This function enables MDIO bus in the Emaclite device and registers a
> + * mii_bus.
> + *
> + * Return:     0 upon success or a negative error upon failure
> + */
> +static int xemaclite_mdio_setup(struct net_local *lp, struct device *dev)
> +{
> +       struct mii_bus *bus;
> +       int rc;
> +       struct resource res;
> +       struct device_node *np = of_get_parent(lp->phy_node);
> +
> +       /* Don't register the MDIO bus if the phy_node or its parent node
> +        * can't be found.
> +        */
> +       if (!np)
> +               return -ENODEV;

This doesn't make sense.  The MDIO bus registration should not be
conditional on whether or not this particular xemaclite instance has a
phy on this particular bus.  Instead of following the mac
--(phandle)--> phy-device --(parent)--> mdio node linkage, this
function should look at the mac nodes children to find the mdio node.

In fact, now that I think about it, the code is actually dangerous,
because it may try and register a different mdio bus node as its own.

> +
> +       /* Enable the MDIO bus by asserting the enable bit in MDIO Control
> +        * register.
> +        */
> +       out_be32(lp->base_addr + XEL_MDIOCTRL_OFFSET,
> +                XEL_MDIOCTRL_MDIOEN_MASK);
> +
> +       bus = mdiobus_alloc();
> +       if (!bus)
> +               return -ENOMEM;
> +
> +       of_address_to_resource(np, 0, &res);
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%.8llx",
> +                (unsigned long long)res.start);
> +       bus->priv = lp;
> +       bus->name = "Xilinx Emaclite MDIO";
> +       bus->read = xemaclite_mdio_read;
> +       bus->write = xemaclite_mdio_write;
> +       bus->reset = xemaclite_mdio_reset;
> +       bus->parent = dev;
> +       bus->irq = lp->mdio_irqs; /* preallocated IRQ table */
> +
> +       lp->mii_bus = bus;
> +
> +       rc = of_mdiobus_register(bus, np);
> +       if (rc)
> +               goto err_register;
> +
> +       return 0;
> +
> +err_register:
> +       mdiobus_free(bus);
> +       return rc;
> +}


-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.