On Thu, Feb 13, 2014 at 05:29:51AM +0000, Florian Fainelli wrote:
> This patch adds the BCMGENET main driver file which supports the
> following:
>
> - GENET hardware from V1 to V4
> - support for reading the UniMAC MIB counters statistics
> - support for the 5 transmit queues
> - support for RX/TX checksum offload and SG
>
> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> ---
> Changes since v1:
> - use module_platform_driver boilerplate macro
> - renamed bcmgenet_plat_drv to bcmgenet_driver
> - renamed bcmgenet_drv_{probe,remove} to bcmgenet_{probe,remove}
> - removed priv->phy_type usage and use priv->phy_interface which
> contains the exact same value
> - removed debug module parameters, unused
> - added MODULDE_{AUTHOR,ALIAS,DESCRIPTION} macros
> - remove hardcoded queue index check in bcmgenet_xmit
[...]
> +static int bcmgenet_probe(struct platform_device *pdev)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct bcmgenet_priv *priv;
> + struct net_device *dev;
> + const void *macaddr;
> + struct resource *r;
> + int err = -EIO;
> +
> + /* Up to GENET_MAX_MQ_CNT + 1 TX queues and a single RX queue */
> + dev = alloc_etherdev_mqs(sizeof(*priv), GENET_MAX_MQ_CNT + 1, 1);
> + if (!dev) {
> + dev_err(&pdev->dev, "can't allocate net device\n");
> + return -ENOMEM;
> + }
> +
> + priv = netdev_priv(dev);
> + priv->irq0 = platform_get_irq(pdev, 0);
> + priv->irq1 = platform_get_irq(pdev, 1);
> + if (!priv->irq0 || !priv->irq1) {
> + dev_err(&pdev->dev, "can't find IRQs\n");
> + err = -EINVAL;
> + goto err;
> + }
The binding did not describe that there were two interrupts. What are
each of them for? Are they named in the documentation?
> +
> + macaddr = of_get_mac_address(dn);
> + if (!macaddr) {
> + dev_err(&pdev->dev, "can't find MAC address\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->base = devm_request_and_ioremap(&pdev->dev, r);
> + if (!priv->base) {
> + dev_err(&pdev->dev, "can't ioremap\n");
> + err = -EINVAL;
> + goto err;
> + }
> +
> + dev->base_addr = (unsigned long)priv->base;
Does the net core actually need this?
I can't see anywhere else it's used in this file.
[...]
> + if (of_device_is_compatible(dn, "brcm,genet-v4"))
> + priv->version = GENET_V4;
> + else if (of_device_is_compatible(dn, "brcm,genet-v3"))
> + priv->version = GENET_V3;
> + else if (of_device_is_compatible(dn, "brcm,genet-v2"))
> + priv->version = GENET_V2;
> + else if (of_device_is_compatible(dn, "brcm,genet-v1"))
> + priv->version = GENET_V1;
> + else {
> + dev_err(&pdev->dev, "unknown GENET version\n");
> + err = -EINVAL;
> + goto err;
Surely you can't have probed if none of these are in the compatible
list?
Might it make more sense to place this value in of_device_id::data? You
can get it it with of_match_node, and you only have to place the strings
in one place, so no possible typo issues.
> + }
> +
> + bcmgenet_set_hw_params(priv);
> +
> + spin_lock_init(&priv->lock);
> + spin_lock_init(&priv->bh_lock);
> + mutex_init(&priv->mib_mutex);
> + /* Mii wait queue */
> + init_waitqueue_head(&priv->wq);
> + /* Always use RX_BUF_LENGTH (2KB) buffer for all chips */
> + priv->rx_buf_len = RX_BUF_LENGTH;
> + INIT_WORK(&priv->bcmgenet_irq_work, bcmgenet_irq_task);
> +
> + priv->clk = devm_clk_get(&priv->pdev->dev, "enet");
> + if (IS_ERR(priv->clk))
> + dev_warn(&priv->pdev->dev, "failed to get enet clock\n");
This wasn't in the binding.
> +
> + priv->clk_wol = devm_clk_get(&priv->pdev->dev, "enet-wol");
> + if (IS_ERR(priv->clk_wol))
> + dev_warn(&priv->pdev->dev, "failed to get enet-wol clock\n");
This is also missing from the binding.
> + /* Turn off the clocks */
> + if (!IS_ERR(priv->clk))
> + clk_disable_unprepare(priv->clk);
Either the comment is misleading (s/clocks/clock/), or you're forgetting
about the enet-wol clock here.
Thanks,
Mark.
--
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
