Messages in this thread | | | Date | Thu, 27 Jan 2022 17:57:51 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH v14, 2/2] net: Add dm9051 driver |
| |
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@gmail.com>
> +/* 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; > +}
| |