On 03/07/2017 05:08 PM, Iyappan Subramanian wrote:
> This patch adds,
>
> - probe, remove, shutdown
> - open, close and stats
> - create and delete ring
> - request and delete irq
>
> Signed-off-by: Iyappan Subramanian <isubraman...@apm.com>
> Signed-off-by: Keyur Chudgar <kchud...@apm.com>
> ---
> + pdata->resources.phy_mode = phy_mode;
> +
> + if (pdata->resources.phy_mode != PHY_INTERFACE_MODE_RGMII) {
> + dev_err(dev, "Incorrect phy-connection-type specified\n");
> + return -ENODEV;
> + }
This does not take into account all other PHY_INTERFACE_MODE_RGMII
variants, is that really intentional here?
> +
> + ret = platform_get_irq(pdev, 0);
> + if (ret <= 0) {
0 can be a valid interrupt on some platforms AFAIR, so you may want to
just check < 0.
> + dev_err(dev, "Unable to get ENET IRQ\n");
> + ret = ret ? : -ENXIO;
> + return ret;
> + }
> + pdata->resources.irq = ret;
> +
> +static int xge_request_irq(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + struct device *dev = &pdata->pdev->dev;
> + int ret;
> +
> + snprintf(pdata->irq_name, IRQ_ID_SIZE, "%s", ndev->name);
> +
> + ret = devm_request_irq(dev, pdata->resources.irq, xge_irq,
> + 0, pdata->irq_name, pdata);
> + if (ret)
> + netdev_err(ndev, "Failed to request irq %s\n", pdata->irq_name);
The preference for network driver is to manage the request_irq() in the
ndo_open() callback and free_irq() in the ndo_close() which kind of
defeats the purpose of using devm_* functions for that purpose.
> +static int xge_open(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> + int ret;
> +
> + ret = xge_create_desc_rings(ndev);
> + if (ret)
> + return ret;
> +
> + napi_enable(&pdata->napi);
> + ret = xge_request_irq(ndev);
> + if (ret)
> + return ret;
> +
> + xge_intr_enable(pdata);
> + xge_wr_csr(pdata, DMARXCTRL, 1);
> + xge_mac_enable(pdata);
> + netif_start_queue(ndev);
> + netif_carrier_on(ndev);
Can't you use PHYLIB to get the link indication and not manage the link
state manually here? Setting netif_carrier_on() without checking the
actualy physical medium is just plain wrong/
> +static void xge_timeout(struct net_device *ndev)
> +{
> + struct xge_pdata *pdata = netdev_priv(ndev);
> +
> + rtnl_lock();
> +
> + if (netif_running(ndev)) {
Reduce indention here.
> + netif_carrier_off(ndev);
> + netif_stop_queue(ndev);
> + xge_intr_disable(pdata);
> + napi_disable(&pdata->napi);
> +
> +static int xge_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct net_device *ndev;
> + struct xge_pdata *pdata;
> + int ret;
> +
> + ndev = alloc_etherdev(sizeof(struct xge_pdata));
sizeof(*pdata).
--
Florian
