[U-Boot] [PATCH] net: Add ag7xxx driver for Atheros MIPS

[Joe Hershberger <joe.hershberger@ni.com>]

Hi Marek,

I was looking at something else and noticed what looks like an issue
with this code you submitted.

On Tue, May 24, 2016 at 4:29 PM, Marek Vasut <marex@denx.de> wrote:
> Add ethernet driver for the AR933x and AR934x Atheros MIPS machines.
> The driver could be easily extended to other WiSoCs.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Daniel Schwierzeck <daniel.schwierzeck@gmail.com>
> Cc: Joe Hershberger <joe.hershberger@ni.com>
> Cc: Wills Wang <wills.wang@live.com>
> ---
> V2: - Drop the printf() in case malloc fails, it's pointless to try
>       and print something if we cannot allocate memory, since printf
>       also allocates memory.
> V3: - Replace magic 0x11 with MII_MIPSCR register
> ---

[...] SNIP

> +static u16 ag7xxx_mdio_rw(struct mii_dev *bus, int addr, int reg, u32 val)

Returns a u16

> +{
> +       u32 data;
> +
> +       /* Dummy read followed by PHY read/write command. */
> +       ag7xxx_switch_reg_read(bus, 0x98, &data);
> +       data = val | (reg << 16) | (addr << 21) | BIT(30) | BIT(31);
> +       ag7xxx_switch_reg_write(bus, 0x98, data);
> +
> +       /* Wait for operation to finish */
> +       do {
> +               ag7xxx_switch_reg_read(bus, 0x98, &data);
> +       } while (data & BIT(31));
> +
> +       return data & 0xffff;
> +}
> +
> +static int ag7xxx_mdio_read(struct mii_dev *bus, int addr, int devad, int reg)
> +{
> +       return ag7xxx_mdio_rw(bus, addr, reg, BIT(27));

Directly returns said u16 as an int.

> +}
> +
> +static int ag7xxx_mdio_write(struct mii_dev *bus, int addr, int devad, int reg,
> +                            u16 val)
> +{
> +       ag7xxx_mdio_rw(bus, addr, reg, val);
> +       return 0;
> +}

[...] SNIP

> +static int ag933x_phy_setup_common(struct udevice *dev)
> +{
> +       struct ar7xxx_eth_priv *priv = dev_get_priv(dev);
> +       int i, ret, phymax;
> +
> +       if (priv->model == AG7XXX_MODEL_AG933X)
> +               phymax = 4;
> +       else if (priv->model == AG7XXX_MODEL_AG934X)
> +               phymax = 5;
> +       else
> +               return -EINVAL;
> +
> +       if (priv->interface == PHY_INTERFACE_MODE_RMII) {
> +               ret = ag933x_phy_setup_reset_set(dev, phymax);
> +               if (ret)
> +                       return ret;
> +
> +               ret = ag933x_phy_setup_reset_fin(dev, phymax);
> +               if (ret)
> +                       return ret;
> +
> +               /* Read out link status */
> +               ret = ag7xxx_mdio_read(priv->bus, phymax, 0, MII_MIPSCR);

Read the link status, which can never be negative.

Another issue: Is MII_MIPSCR really the register name? It's not better
than "17" - constants should mean something, just just be a random
name with the right value.

> +               if (ret < 0)
> +                       return ret;
> +
> +               return 0;

Return 0 unconditionally. Presumably you want to actually check the
link status to be something specific if you bother to read it out.

> +       }
> +
> +       /* Switch ports */
> +       for (i = 0; i < phymax; i++) {
> +               ret = ag933x_phy_setup_reset_set(dev, i);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       for (i = 0; i < phymax; i++) {
> +               ret = ag933x_phy_setup_reset_fin(dev, i);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       for (i = 0; i < phymax; i++) {
> +               /* Read out link status */
> +               ret = ag7xxx_mdio_read(priv->bus, i, 0, MII_MIPSCR);

Same thing here.

> +               if (ret < 0)
> +                       return ret;
> +       }
> +
> +       return 0;
> +}