Byungho An <bh74.an@xxxxxxxxxxx> :
[...]
> +static int sxgbe_hw_init(struct sxgbe_priv_data * const priv)
> +{
struct sxgbe_ops *hw = priv->hw;
> + u32 ctrl_ids;
[...]
> +struct sxgbe_priv_data *sxgbe_dvr_probe(struct device *device,
nit: s/dvr/drv/ ?
(several occurences in the driver)
> + struct sxgbe_plat_data *plat_dat,
> + void __iomem *addr)
> +{
> + int ret = 0;
Useless init.
> + struct net_device *ndev = NULL;
Useless init.
> + struct sxgbe_priv_data *priv;
Nit: you may consider reorganizing the variables in an inverted xmas
tree fashion at some point.
> +
> + ndev = alloc_etherdev_mqs(sizeof(struct sxgbe_priv_data),
> + SXGBE_TX_QUEUES, SXGBE_RX_QUEUES);
> + if (!ndev)
> + return NULL;
> +
> + SET_NETDEV_DEV(ndev, device);
> +
> + priv = netdev_priv(ndev);
> + priv->device = device;
> + priv->dev = ndev;
> +
> + ether_setup(ndev);
Useless.
(see alloc_etherdev_mqs vs alloc_netdev_mqs)
> +
> + sxgbe_set_ethtool_ops(ndev);
> + priv->plat = plat_dat;
> + priv->ioaddr = addr;
> +
> + /* Init MAC and get the capabilities */
> + ret = sxgbe_hw_init(priv);
sxgbe_hw_init can't fail. It should return void.
[...]
> + /* MDIO bus Registration */
> + ret = sxgbe_mdio_register(ndev);
> + if (ret < 0) {
> + netdev_dbg(ndev, "%s: MDIO bus (id: %d) registration failed\n",
> + __func__, priv->plat->bus_id);
> + goto error_mdio_register;
> + }
> +
> + ret = register_netdev(ndev);
> + if (ret) {
> + pr_err("%s: ERROR %i registering the device\n", __func__, ret);
> + goto error_netdev_register;
sxgbe_mdio_register is left unbalanced in the error path.
I am more used to 'goto what_should_be_done' style than
'goto where_it_comes_from' (both require to check if the label are correctly
ordered in the unwind path though).
> + }
> +
> + sxgbe_check_ether_addr(priv);
> +
> + return priv;
> +
> +error_mdio_register:
> + clk_put(priv->sxgbe_clk);
> +error_clk_get:
> + unregister_netdev(ndev);
There's no failure point past register_netdev: unregister_netdev can't be
needed.
> +error_netdev_register:
> + netif_napi_del(&priv->napi);
> +error_free_netdev:
> + free_netdev(ndev);
> +
> + return NULL;
Nit: you may use ERR_PTR and always return 'priv'. The caller could thus
propagate the error code.
> +}
> +
> +/**
> + * sxgbe_dvr_remove
> + * @ndev: net device pointer
> + * Description: this function resets the TX/RX processes, disables the MAC RX/TX
> + * changes the link status, releases the DMA descriptor rings.
> + */
> +int sxgbe_dvr_remove(struct net_device *ndev)
> +{
> + struct sxgbe_priv_data *priv = netdev_priv(ndev);
struct sxgbe_ops *hw = priv->hw;
> +
> + netdev_info(ndev, "%s: removing driver\n", __func__);
> +
> + priv->hw->dma->stop_rx(priv->ioaddr, SXGBE_RX_QUEUES);
> + priv->hw->dma->stop_tx(priv->ioaddr, SXGBE_TX_QUEUES);
> +
> + priv->hw->mac->enable_tx(priv->ioaddr, false);
> + priv->hw->mac->enable_rx(priv->ioaddr, false);
[...]
> +static int sxgbe_mdio_read(struct mii_bus *bus, int phyaddr, int phyreg)
> +{
> + struct net_device *ndev = bus->priv;
> + struct sxgbe_priv_data *priv = netdev_priv(ndev);
> + u32 devaddr, reg_val;
Excess scope for devaddr.
> + u32 mii_addr = priv->hw->mii.addr;
> + u32 mii_data = priv->hw->mii.data;
const ?
> +
> + /* check for busy wait */
Useless comment. Pure noise for the reviewer / maintainer.
> + if (sxgbe_mdio_busy_wait(priv->ioaddr, mii_data))
> + return -EBUSY;
sxgbe_mdio_busy_wait already returns -EBUSY when it fails. Please use it.
> +
> + if (phyreg & MII_ADDR_C45) {
> + devaddr = (phyreg >> 16) & 0x1F;
> + /* set mdio address register */
> + reg_val = (phyaddr << 16) | (devaddr << 21) | (phyreg & 0xFFFF);
> + writel(reg_val, priv->ioaddr + mii_addr);
> +
> + /* set mdio control/data register */
> + reg_val = ((SXGBE_SMA_READ_CMD << 16) | SXGBE_SMA_SKIP_ADDRFRM |
> + ((priv->clk_csr & 0x7) << 19) | SXGBE_MII_BUSY);
Excess parenthesis:
reg_val = (SXGBE_SMA_READ_CMD << 16) | SXGBE_SMA_SKIP_ADDRFRM |
((priv->clk_csr & 0x7) << 19) | SXGBE_MII_BUSY;
> + writel(reg_val, priv->ioaddr + mii_data);
> + } else {
> + /* configure the port for C22
> + * ports 0-3 only supports C22
> + */
> + if (phyaddr >= 4)
> + return -ENODEV;
> + writel((1 << phyaddr),
Excess parenthesis.
> + priv->ioaddr + SXGBE_MDIO_CLAUSE22_PORT_REG);
> + /* set mdio address register */
> + reg_val = (phyaddr << 16) | (phyreg & 0x1F);
> + writel(reg_val, priv->ioaddr + mii_addr);
> +
> + /* set mdio control/data register */
> + reg_val = ((SXGBE_SMA_READ_CMD << 16) | SXGBE_SMA_SKIP_ADDRFRM |
> + ((priv->clk_csr & 0x7) << 19) | SXGBE_MII_BUSY);
> + writel(reg_val, priv->ioaddr + mii_data);
> + }
The whole 'if (phyreg & MII_ADDR_C45) { ... } else { ... }' chunk is
shared with sxgbe_mdio_write(..., phydata = 0). Factor out ?
> +
> + /* wait till operation succeds */
> + if (sxgbe_mdio_busy_wait(priv->ioaddr, mii_data))
> + return -EBUSY;
> +
> + /* read and return the data from mmi Data register */
> + reg_val = readl(priv->ioaddr + mii_data) & 0xFFFF;
> + return reg_val;
return readl(priv->ioaddr + mii_data) & 0xffff;
Then reduce scope of reg_val.
> +}
> +/**
> + * sxgbe_mdio_write
> + * @bus: points to the mii_bus structure
> + * @phyaddr: address of phy port
> + * @phyreg: address of phy registers
> + * @phydata: data to be written into phy register
> + * Description: this function is used for C45 and C22 MDIO write
> + */
> +static int sxgbe_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg,
> + u16 phydata)
> +{
See sxgbe_mdio_read for comments.
[...]
> + if (sxgbe_mdio_busy_wait(priv->ioaddr, mii_data))
> + return -EBUSY;
> +
> + return 0;
return sxgbe_mdio_busy_wait(priv->ioaddr, mii_data);
> +}
> +
> +int sxgbe_mdio_register(struct net_device *ndev)
> +{
> + int err, phy_addr, act;
Excess scope for act.
> + int *irqlist;
> + u8 phy_found = 0;
> + struct mii_bus *mdio_bus;
> + struct sxgbe_priv_data *priv = netdev_priv(ndev);
> + struct sxgbe_mdio_bus_data *mdio_data = priv->plat->mdio_bus_data;
> +
> + /* allocate the new mdio bus */
> + mdio_bus = mdiobus_alloc();
> + if (!mdio_bus) {
> + netdev_err(ndev, "%s: mii bus allocation failed\n", __func__);
> + return -ENOMEM;
> + }
> +
> + if (mdio_data->irqs)
> + irqlist = mdio_data->irqs;
> + else
> + irqlist = priv->mii_irq;
> +
> + /* assign mii bus fields */
> + mdio_bus->name = "samsxgbe";
> + mdio_bus->read = &sxgbe_mdio_read;
> + mdio_bus->write = &sxgbe_mdio_write;
> + snprintf(mdio_bus->id, MII_BUS_ID_SIZE, "%s-%x",
> + mdio_bus->name, priv->plat->bus_id);
> + mdio_bus->priv = ndev;
> + mdio_bus->phy_mask = mdio_data->phy_mask;
> + mdio_bus->parent = priv->device;
> +
> + /* register with kernel subsystem */
> + err = mdiobus_register(mdio_bus);
> + if (err != 0) {
> + netdev_err(ndev, "mdiobus register failed\n");
> + goto mdiobus_err;
> + }
> +
> + for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
> + struct phy_device *phy = mdio_bus->phy_map[phy_addr];
> + if (phy) {
Missing empty line after the declaration.
[...]
> + phy_found = 1;
> + }
> + }
> +
> + if (!phy_found) {
> + netdev_err(ndev, "PHY not found\n");
> + mdiobus_unregister(mdio_bus);
> + mdiobus_free(mdio_bus);
> + return -ENODEV;
You may remove phy_found, use "err" instead and gotoize on top of mdiobus_err.
> + }
> +
> + priv->mii = mdio_bus;
> +
> + return 0;
> +
> +mdiobus_err:
> + mdiobus_free(mdio_bus);
> + return err;
> +}
[...]
> +static void sxgbe_mtl_set_txfifosize(void __iomem *ioaddr, int queue_num,
> + int queue_fifo)
> +{
> + u32 fifo_bits, reg_val;
> + /* 0 means 256 bytes */
> + fifo_bits = (queue_fifo / SXGBE_MTL_TX_FIFO_DIV)-1;
- Missing empty line after declaration.
- ')-1' -> ') - 1'
> + reg_val = readl(ioaddr + SXGBE_MTL_TXQ_OPMODE_REG(queue_num));
> + reg_val |= (fifo_bits << SXGBE_MTL_FIFO_LSHIFT);
> + writel(reg_val, ioaddr + SXGBE_MTL_TXQ_OPMODE_REG(queue_num));
> +}
> +
> +static void sxgbe_mtl_set_rxfifosize(void __iomem *ioaddr, int queue_num,
> + int queue_fifo)
> +{
> + u32 fifo_bits, reg_val;
> + /* 0 means 256 bytes */
> + fifo_bits = (queue_fifo / SXGBE_MTL_RX_FIFO_DIV)-1;
Missing empty line after declaration.
[...]
> +static const struct sxgbe_mtl_ops mtl_ops = {
> + .mtl_set_txfifosize = sxgbe_mtl_set_txfifosize,
> + .mtl_set_rxfifosize = sxgbe_mtl_set_rxfifosize,
> + .mtl_enable_txqueue = sxgbe_mtl_enable_txqueue,
> + .mtl_disable_txqueue = sxgbe_mtl_disable_txqueue,
> + .mtl_dynamic_dma_rxqueue = sxgbe_mtl_dma_dm_rxqueue,
> + .set_tx_mtl_mode = sxgbe_set_tx_mtl_mode,
> + .set_rx_mtl_mode = sxgbe_set_rx_mtl_mode,
> + .mtl_init = sxgbe_mtl_init,
> + .mtl_fc_active = sxgbe_mtl_fc_active,
> + .mtl_fc_deactive = sxgbe_mtl_fc_deactive,
> + .mtl_fc_enable = sxgbe_mtl_fc_enable,
> + .mtl_fep_enable = sxgbe_mtl_fep_enable,
> + .mtl_fep_disable = sxgbe_mtl_fep_disable,
> + .mtl_fup_enable = sxgbe_mtl_fup_enable,
> + .mtl_fup_disable = sxgbe_mtl_fup_disable
Please use tabs before "=".
[...]
> +static int sxgbe_platform_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + int loop = 0;
> + int index1, index2;
> + struct resource *res;
> + struct device *dev = &pdev->dev;
> + void __iomem *addr = NULL;
Useless init.
[...]
> + priv = sxgbe_dvr_probe(&(pdev->dev), plat_dat, addr);
> + if (!priv) {
> + pr_err("%s: main driver probe failed\n", __func__);
> + return -ENODEV;
> + }
> +
> + /* Get MAC address if available (DT) */
> + if (mac)
> + ether_addr_copy(priv->dev->dev_addr, mac);
> +
> + /* Get the SXGBE common INT information */
> + priv->irq = platform_get_irq(pdev, loop++);
> + if (priv->irq <= 0) {
> + dev_err(dev, "sxgbe common irq parsing failed\n");
> + return -EINVAL;
No sxgbe_dvr_remove ?
> + }
> +
> + /* Get the TX/RX IRQ numbers */
> + for (index1 = 0, index2 = 0 ; loop < total_dma_channels; loop++) {
> + if (loop < SXGBE_TX_QUEUES) {
> + (priv->txq[index1])->irq_no =
> + irq_of_parse_and_map(dev->of_node, loop);
> + if ((priv->txq[index1++])->irq_no <= 0) {
> + dev_err(dev, "sxgbe tx irq parsing failed\n");
> + return -EINVAL;
> + }
> + } else {
> + (priv->rxq[index2])->irq_no =
> + irq_of_parse_and_map(dev->of_node, loop);
> + if ((priv->rxq[index2++])->irq_no <= 0) {
> + dev_err(dev, "sxgbe rx irq parsing failed\n");
> + return -EINVAL;
> + }
> + }
> + }
(yucky)
struct device_node *node = dev->of_node;
for (i = 0, chan = 0; i < SXGBE_TX_QUEUES; i++) {
priv->txq[i]->irq_no = irq_of_parse_and_map(node, chan++);
if (priv->txq[i]->irq_no <= 0) {
dev_err(dev, "sxgbe tx irq parsing failed\n");
return -EINVAL;
}
}
for (i = 0; i < SXGBE_RX_QUEUES; i++) {
priv->rxq[i]->irq_no = irq_of_parse_and_map(node, chan++);
if (priv->rxq[i]->irq_no <= 0) {
dev_err(dev, "sxgbe rx irq parsing failed\n");
return -EINVAL;
}
}
It should imho use irq_dispose_mapping in the error path (and balance
in sxgbe_platform_remove as well).
> +
> + platform_set_drvdata(pdev, priv->dev);
> +
> + pr_debug("platform driver registration completed\n");
> +
> + return 0;
> +}
> +
> +/**
> + * sxgbe_platform_remove
> + * @pdev: platform device pointer
> + * Description: this function calls the main to free the net resources
> + * and calls the platforms hook and release the resources (e.g. mem).
> + */
> +static int sxgbe_platform_remove(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + int ret = sxgbe_dvr_remove(ndev);
Declaration, empty line.
[...]
> +int sxgbe_platform_freeze(struct device *dev)
> +{
> + int ret;
> + struct net_device *ndev = dev_get_drvdata(dev);
> +
> + ret = sxgbe_freeze(ndev);
> +
> + return ret;
Is 'ret' really needed ?
[...]
> +static const struct dev_pm_ops sxgbe_platform_pm_ops = {
> + .suspend = sxgbe_platform_suspend,
> + .resume = sxgbe_platform_resume,
> + .freeze = sxgbe_platform_freeze,
> + .thaw = sxgbe_platform_restore,
> + .restore = sxgbe_platform_restore,
Please use tabs before '=' to line things up.
/megotobed
--
Ueimor
--
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
