Message-ID: <20140321235526.GA7727@electric-eye.fr.zoreil.com>
Date:	Sat, 22 Mar 2014 00:55:26 +0100
From:	Francois Romieu <romieu@...zoreil.com>
To:	Byungho An <bh74.an@...sung.com>
Cc:	netdev@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
	devicetree@...r.kernel.org, "'David Miller'" <davem@...emloft.net>,
	"'GIRISH K S'" <ks.giri@...sung.com>,
	"'SIVAREDDY KALLAM'" <siva.kallam@...sung.com>,
	"'Vipul Chandrakant'" <vipul.pandya@...sung.com>,
	"'Ilho Lee'" <ilho215.lee@...sung.com>
Subject: Re: [PATCH V10 2/7] net: sxgbe: add basic framework for Samsung 10Gb
 ethernet driver

Byungho An <bh74.an@...sung.com> :
[...]
> +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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists