lkml.org 
[lkml]   [2022]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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, &regconfigdm);
> +
> + 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, &regconfigdmbulk);
> +

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;
> +}

\
 
 \ /
  Last update: 2022-01-28 02:58    [W:0.055 / U:0.368 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site