> +++ b/drivers/net/ethernet/ni/Kconfig
> @@ -0,0 +1,27 @@
> +#
> +# National Instuments network device configuration
> +#
> +
> +config NET_VENDOR_NI
> + bool "National Instruments Devices"
> + default y
> + ---help---
> + If you have a network (Ethernet) device belonging to this class, say
> Y.
> +
> + Note that the answer to this question doesn't directly affect the
> + kernel: saying N will just cause the configurator to skip all
> + the questions about National Instrument devices.
> + If you say Y, you will be asked for your specific device in the
> + following questions.
> +
> +if NET_VENDOR_NI
> +
> +config NI_XGE_MANAGEMENT_ENET
> + tristate "National Instruments XGE management enet support"
> + depends on ARCH_ZYNQ
Consider also adding COMPILE_TEST, if possible.
> +#define nixge_ctrl_poll_timeout(priv, addr, val, cond, sleep_us, timeout_us)
> \
> + readl_poll_timeout((priv)->ctrl_regs + (addr), (val), cond, \
> + (sleep_us), (timeout_us))
Seems odd not having cond inside (), especially since cond could be a
complex expression.
> +static void nixge_handle_link_change(struct net_device *ndev)
> +{
> + struct nixge_priv *priv = netdev_priv(ndev);
> + struct phy_device *phydev = ndev->phydev;
> + unsigned long flags;
> + int status_change = 0;
> +
> + spin_lock_irqsave(&priv->lock, flags);
> +
> + if (phydev->link != priv->link || phydev->speed != priv->speed ||
> + phydev->duplex != priv->duplex) {
> + priv->link = phydev->link;
> + priv->speed = phydev->speed;
> + priv->duplex = phydev->duplex;
> + status_change = 1;
> + }
> +
> + spin_unlock_irqrestore(&priv->lock, flags);
> +
> + if (status_change)
> + phy_print_status(phydev);
> +}
As Florian pointed out, you don't make use of any of this
information. So maybe don't bother, just have a return statement.
> +static int nixge_stop(struct net_device *ndev)
> +{
> + u32 cr;
> + struct nixge_priv *priv = netdev_priv(ndev);
> +
> + cr = nixge_dma_read_reg(priv, XAXIDMA_RX_CR_OFFSET);
> + nixge_dma_write_reg(priv, XAXIDMA_RX_CR_OFFSET,
> + cr & (~XAXIDMA_CR_RUNSTOP_MASK));
> + cr = nixge_dma_read_reg(priv, XAXIDMA_TX_CR_OFFSET);
> + nixge_dma_write_reg(priv, XAXIDMA_TX_CR_OFFSET,
> + cr & (~XAXIDMA_CR_RUNSTOP_MASK));
> +
> + tasklet_kill(&priv->dma_err_tasklet);
> +
> + free_irq(priv->tx_irq, ndev);
> + free_irq(priv->rx_irq, ndev);
> +
> + nixge_dma_bd_release(ndev);
> +
> + if (ndev->phydev) {
Do you need this condition? You bail out with ENODEV if of_phy_connect fails?
> + phy_stop(ndev->phydev);
> + phy_disconnect(ndev->phydev);
> + }
> +
> + return 0;
> +}
> +
> +static int nixge_change_mtu(struct net_device *ndev, int new_mtu)
> +{
> + struct nixge_priv *priv = netdev_priv(ndev);
> +
> + if (netif_running(ndev))
> + return -EBUSY;
> +
> + if ((new_mtu + VLAN_ETH_HLEN +
> + NIXGE_TRL_SIZE) > priv->rxmem)
> + return -EINVAL;
> +
> + ndev->mtu = new_mtu;
> +
> + return 0;
> +}
> +
> +static s32 __nixge_hw_set_mac_address(struct net_device *ndev)
> +{
> + struct nixge_priv *priv = netdev_priv(ndev);
> +
> + nixge_ctrl_write_reg(priv, NIXGE_REG_MAC_LSB,
> + (ndev->dev_addr[2]) << 24 |
> + (ndev->dev_addr[3] << 16) |
> + (ndev->dev_addr[4] << 8) |
> + (ndev->dev_addr[5] << 0));
> +
> + nixge_ctrl_write_reg(priv, NIXGE_REG_MAC_MSB,
> + (ndev->dev_addr[1] | (ndev->dev_addr[0] << 8)));
> +
> + return 0;
> +}
> +
> +static int nixge_net_set_mac_address(struct net_device *ndev, void *p)
> +{
> + int err;
> +
> + err = eth_mac_addr(ndev, p);
> + if (!err)
> + __nixge_hw_set_mac_address(ndev);
> +
> + return err;
> +}
Much better, thanks.
> +
> +static int nixge_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16
> val)
> +{
> + struct nixge_priv *priv = bus->priv;
> + u32 status, tmp;
> + int err;
> + u16 device;
> +
> + /* FIXME: Currently don't do writes */
> + if (reg & MII_ADDR_C45)
> + return -EOPNOTSUPP;
> +
> + device = reg & 0x1f;
> +
> + tmp = NIXGE_MDIO_CLAUSE22 | NIXGE_MDIO_OP(MDIO_C22_WRITE) |
> + NIXGE_MDIO_ADDR(phy_id) | NIXGE_MDIO_MMD(device);
> +
> + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_DATA, val);
> + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_OP, tmp);
> + nixge_ctrl_write_reg(priv, NIXGE_REG_MDIO_CTRL, 1);
> +
> + err = nixge_ctrl_poll_timeout(priv, NIXGE_REG_MDIO_CTRL, status,
> + !status, 10, 1000);
> + if (err) {
> + dev_err(priv->dev, "timeout setting write command");
> + return -ETIMEDOUT;
return err;
> + }
> +
> + dev_dbg(priv->dev, "%x %x <- %x\n", phy_id, reg, val);
> +
> + return 0;
> +}
> +
> +static int nixge_mdio_setup(struct nixge_priv *priv, struct device_node *np)
> +{
> + struct mii_bus *bus;
> + struct resource res;
> + int err;
> +
> + bus = mdiobus_alloc();
> + if (!bus)
> + return -ENOMEM;
> +
> + of_address_to_resource(np, 0, &res);
This can fail.
Err, why are you actually doing it anyway? You don't make use of res,
you don't ioremap() it, etc.
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s-mii", dev_name(priv->dev));
> + bus->priv = priv;
> + bus->name = "nixge_mii_bus";
> + bus->read = nixge_mdio_read;
> + bus->write = nixge_mdio_write;
> + bus->parent = priv->dev;
> +
> + priv->mii_bus = bus;
> + err = of_mdiobus_register(bus, np);
> + if (err)
> + goto err_register;
> +
> + dev_info(priv->dev, "MDIO bus registered\n");
> +
> + return 0;
> +
> +err_register:
> + mdiobus_free(bus);
> + return err;
> +}
> +
> +static int nixge_probe(struct platform_device *pdev)
> +{
> + int err;
> + struct nixge_priv *priv;
> + struct net_device *ndev;
> + struct resource *dmares;
> + const char *mac_addr;
> +
> + ndev = alloc_etherdev(sizeof(*priv));
> + if (!ndev)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, ndev);
> + SET_NETDEV_DEV(ndev, &pdev->dev);
> +
> + ndev->flags &= ~IFF_MULTICAST; /* clear multicast */
Could you explain this a bit better please. Does this imply that IPv6
neighbour discovery is not supported? That is a severe restriction.
> +static int nixge_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct nixge_priv *priv = netdev_priv(ndev);
> +
> + if (ndev->phydev)
> + phy_disconnect(ndev->phydev);
nixge_stop() disconnects the phy. I don't think you need it twice.
> + ndev->phydev = NULL;
> +
> + mdiobus_unregister(priv->mii_bus);
> + mdiobus_free(priv->mii_bus);
> + priv->mii_bus = NULL;
> +
> + unregister_netdev(ndev);
> +
> + free_netdev(ndev);
> +
> + return 0;
Andrew
