Re: [PATCH V11 2/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi,

I have reviewed the non-net-specific parts of this driver, e.g. platform 
driver and Device Tree code. Please see my comments inline.

On 22.03.2014 07:23, Byungho An wrote:
> From: Siva Reddy <siva.kallam@samsung.com>
>
> This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
>
> - sxgbe core initialization
> - Tx and Rx support
> - MDIO support
> - ISRs for Tx and Rx
> - ifconfig support to driver

[snip]

> diff --git a/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c
> new file mode 100644
> index 0000000..95e0977
> --- /dev/null
> +++ b/drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c

[snip]

> +#ifdef CONFIG_OF
> +static int sxgbe_probe_config_dt(struct platform_device *pdev,
> +				 struct sxgbe_plat_data *plat,
> +				 const char **mac)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sxgbe_dma_cfg *dma_cfg;
> +
> +	if (!np)
> +		return -ENODEV;
> +
> +	*mac = of_get_mac_address(np);
> +	plat->interface = of_get_phy_mode(np);
> +
> +	plat->bus_id = of_alias_get_id(np, "ethernet");
> +	if (plat->bus_id < 0)
> +		plat->bus_id = 0;
> +
> +	plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> +					   sizeof(struct sxgbe_mdio_bus_data),
> +					   GFP_KERNEL);

If plat->mdio_bus_data is assumed to be of the same type as the data 
allocated here, then the following would be preferred:

sizeof(*plat->mdio_bus_data)

Also you should probably check for allocation failure.

> +
> +	dma_cfg = devm_kzalloc(&pdev->dev, sizeof(*dma_cfg), GFP_KERNEL);
> +	if (!dma_cfg)
> +		return -ENOMEM;
> +
> +	plat->dma_cfg = dma_cfg;
> +	of_property_read_u32(np, "samsung,pbl", &dma_cfg->pbl);
> +	if (of_property_read_u32(np, "samsung,burst-map", &dma_cfg->burst_map) == 0)
> +		dma_cfg->fixed_burst = true;
> +
> +	return 0;
> +}

[snip]

> +static int sxgbe_platform_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	int loop = 0;
> +	int i, chan;
> +	struct resource *res;
> +	struct device *dev = &pdev->dev;
> +	void __iomem *addr;
> +	struct sxgbe_priv_data *priv = NULL;
> +	struct sxgbe_plat_data *plat_dat = NULL;
> +	const char *mac = NULL;
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct device_node *node = dev->of_node;
> +
> +	/* Get memory resource */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +
> +	addr = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(addr))
> +		return PTR_ERR(addr);
> +
> +	if (pdev->dev.of_node) {
> +		plat_dat = devm_kzalloc(&pdev->dev,
> +					sizeof(struct sxgbe_plat_data),
> +					GFP_KERNEL);
> +		if (!plat_dat)
> +			return  -ENOMEM;
> +
> +		ret = sxgbe_probe_config_dt(pdev, plat_dat, &mac);
> +		if (ret) {
> +			pr_err("%s: main dt probe failed\n", __func__);
> +			return ret;
> +		}
> +	}
> +
> +	priv = sxgbe_drv_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++);

The name "loop" of the variable is quite misleading here. Probably 
something like "irq_num", would be more meaningful.

Anyway, it doesn't look like it's used anywhere else in this function, 
so platform_get_irq(pdev, 0) could be simply used.

> +	if (priv->irq <= 0) {
> +		dev_err(dev, "sxgbe common irq parsing failed\n");
> +		sxgbe_drv_remove(ndev);
> +		return -EINVAL;
> +	}
> +
> +	/* Get the TX/RX IRQ numbers */
> +	for (i = 0, chan = 0; i < SXGBE_TX_QUEUES; i++) {
> +		priv->txq[i]->irq_no = irq_of_parse_and_map(node, chan++);

Hmm, this call looks suspicious. The "chan" variable starts here as 0 
and so the first call to irq_of_parse_and_map() will end up with parsing 
the first (zeroth) entry of "interrupts" property, which would be the 
same as returned by platform_get_irq(..., 0) above. Maybe this was the 
point where the "loop" variable should be used?

Anyway, why you couldn't simply use platform_get_irq() here as well?

> +		if (priv->txq[i]->irq_no <= 0) {
> +			dev_err(dev, "sxgbe tx irq parsing failed\n");

Shouldn't you do some clean-up here, like calling sxgbe_drv_remove()? 
Maybe moving the call to sxgbe_drv_probe() after all the resources are 
successfully retrieved would be a better idea?

> +			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");

Same comments as for TX IRQs above.

Best regards,
Tomasz