Thread (10 messages) 10 messages, 2 authors, 2017-10-19

Re: [PATCH net-next v2 2/2] net: ethernet: socionext: add AVE ethernet driver

flat view

From: Masahiro Yamada <hidden>
Date: 2017-10-15 15:09:12
Also in: linux-arm-kernel, linux-devicetree, lkml

2017-10-13 9:35 GMT+09:00 Kunihiko Hayashi [off-list ref]:
quoted hunk ↗ jump to hunk
+static int ave_probe(struct platform_device *pdev)
+{
+       struct device *dev = &pdev->dev;
+       struct device_node *np = dev->of_node;
+       u32 ave_id;
+       struct ave_private *priv;
+       const struct ave_soc_data *data;
+       phy_interface_t phy_mode;
+       struct net_device *ndev;
+       struct resource *res;
+       void __iomem *base;
+       int irq, ret = 0;
+       char buf[ETHTOOL_FWVERS_LEN];
+       const void *mac_addr;
+
+       data = of_device_get_match_data(dev);
+       if (WARN_ON(!data))
+               return -EINVAL;
+
+       phy_mode = of_get_phy_mode(np);
+       if (phy_mode < 0) {
+               dev_err(dev, "phy-mode not found\n");
+               return -EINVAL;
+       }
+       if ((!phy_interface_mode_is_rgmii(phy_mode)) &&
+           phy_mode != PHY_INTERFACE_MODE_RMII &&
+           phy_mode != PHY_INTERFACE_MODE_MII) {
+               dev_err(dev, "phy-mode is invalid\n");
+               return -EINVAL;
+       }
+
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {
+               dev_err(dev, "IRQ not found\n");
+               return irq;
+       }
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(base))
+               return PTR_ERR(base);
+
+       /* alloc netdevice */
+       ndev = alloc_etherdev(sizeof(struct ave_private));
+       if (!ndev) {
+               dev_err(dev, "can't allocate ethernet device\n");
+               return -ENOMEM;
+       }
+
+       ndev->netdev_ops = &ave_netdev_ops;
+       ndev->ethtool_ops = &ave_ethtool_ops;
+       SET_NETDEV_DEV(ndev, dev);
+
+       /* support hardware checksum */
+       ndev->features    |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
+       ndev->hw_features |= (NETIF_F_IP_CSUM | NETIF_F_RXCSUM);
+
+       ndev->max_mtu = AVE_MAX_ETHFRAME - (ETH_HLEN + ETH_FCS_LEN);
+
+       /* get mac address */
+       mac_addr = of_get_mac_address(np);
+       if (mac_addr)
+               ether_addr_copy(ndev->dev_addr, mac_addr);
+
+       /* if the mac address is invalid, use random mac address */
+       if (!is_valid_ether_addr(ndev->dev_addr)) {
+               eth_hw_addr_random(ndev);
+               dev_warn(dev, "Using random MAC address: %pM\n",
+                        ndev->dev_addr);
+       }
+
+       priv = netdev_priv(ndev);
+       priv->base = base;
+       priv->irq = irq;
+       priv->ndev = ndev;
+       priv->msg_enable = netif_msg_init(-1, AVE_DEFAULT_MSG_ENABLE);
+       priv->phy_mode = phy_mode;
+       priv->data = data;
+
+       if (IS_DESC_64BIT(priv)) {
+               priv->desc_size = AVE_DESC_SIZE_64;
+               priv->tx.daddr  = AVE_TXDM_64;
+               priv->rx.daddr  = AVE_RXDM_64;
+       } else {
+               priv->desc_size = AVE_DESC_SIZE_32;
+               priv->tx.daddr  = AVE_TXDM_32;
+               priv->rx.daddr  = AVE_RXDM_32;
+       }
+       priv->tx.ndesc = AVE_NR_TXDESC;
+       priv->rx.ndesc = AVE_NR_RXDESC;
+
+       u64_stats_init(&priv->stats_rx.syncp);
+       u64_stats_init(&priv->stats_tx.syncp);
+
+       /* get clock */
Please remove this super-obvious comment.

+       priv->clk = clk_get(dev, NULL);
Missing clk_put() in the failure path.

Why don't you use devm?


+       if (IS_ERR(priv->clk))
+               priv->clk = NULL;
So, clk is optional, but
you need to check EPROBE_DEFER.



+       /* get reset */
Remove.

quoted hunk ↗ jump to hunk
+       priv->rst = reset_control_get(dev, NULL);
+       if (IS_ERR(priv->rst))
+               priv->rst = NULL;

reset_control_get() is deprecated.  Do not use it in a new driver.

Again, missing reset_control_put().   devm?


The reset seems optional (again, ignoring EPROBE_DEFER)
but you did not use reset_control_get_optional, why?
From your code, this reset is used as shared.

priv->rst = devm_reset_control_get_optional_shared(dev, NULL);
if (IS_ERR(priv->rst))
          return PTR_ERR(priv->rst);








-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help