On Thu, 27 Jan 2022 11:27:01 +0800 Joseph CHAMG wrote:
> Add davicom dm9051 spi ethernet driver, The driver work for the
> device platform which has the spi master
>
> Signed-off-by: Joseph CHAMG <josright123@xxxxxxxxx>
> +/* event: write into the mac registers and eeprom directly
> + */
> +static int dm9051_set_mac_address(struct net_device *ndev, void *p)
> +{
> + struct board_info *db = to_dm9051_board(ndev);
> + int ret;
> +
> + ret = eth_mac_addr(ndev, p);
You should not be using this helper if the write can fail. See what
this function does internally and:
- put the eth_prepare_mac_addr_change() call here
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_bulk_write(db->regmap_dmbulk, DM9051_PAR, ndev->dev_addr, ETH_ALEN);
> + if (ret < 0)
> + netif_err(db, drv, ndev, "%s: error %d bulk writing reg %02x, len %d\n",
> + __func__, ret, DM9051_PAR, ETH_ALEN);
- put the eth_commit_mac_addr_change() call here
> + return ret;
> +}
> +static void dm9051_netdev(struct net_device *ndev)
> +{
> + ndev->mtu = 1500;
Unnecessary, ether_setup() does this already.
> + ndev->if_port = IF_PORT_100BASET;
Why set this? The if_port API is a leftover from very old 10Mbit
Ethernet days, we have ethtool link APIs now.
> + ndev->netdev_ops = &dm9051_netdev_ops;
> + ndev->ethtool_ops = &dm9051_ethtool_ops;
Just inline there two lines into the caller and remove the helper.
dm9051_netdev() does not sound like a function that does setup.
> +}
> +
> +static int dm9051_map_init(struct spi_device *spi, struct board_info *db)
> +{
> + /* create two regmap instances,
> + * run read/write and bulk_read/bulk_write individually,
> + * to resolve regmap execution confliction problem
> + */
> + regconfigdm.lock_arg = db;
> + db->regmap_dm = devm_regmap_init_spi(db->spidev, ®configdm);
> +
> + if (IS_ERR(db->regmap_dm))
> + return PTR_ERR_OR_ZERO(db->regmap_dm);
> +
> + regconfigdmbulk.lock_arg = db;
> + db->regmap_dmbulk = devm_regmap_init_spi(db->spidev, ®configdmbulk);
> +
Please remove all the empty lines between function call and error
checking the result.
> + if (IS_ERR(db->regmap_dmbulk))
> + return PTR_ERR_OR_ZERO(db->regmap_dmbulk);
Why _OR_ZERO() when you're in a IS_ERR() condition already?
> + return 0;
> + ret = devm_register_netdev(dev, ndev);
> + if (ret) {
> + dev_err(dev, "failed to register network device\n");
> + kthread_stop(db->kwr_task_kw);
> + phy_disconnect(db->phydev);
> + return ret;
> + }
> +
> + skb_queue_head_init(&db->txq);
All the state must be initialized before netdev is registered,
otherwise another thread may immediately open the device and
start to transmit.
> + return 0;
> +
> +err_stopthread:
> + kthread_stop(db->kwr_task_kw);
> + return ret;
> +}
