Re: [PATCH v11, 2/2] net: Add dm9051 driver

From: Andy Shevchenko
Date: Thu Jan 13 2022 - 05:18:31 EST


Thanks for update, my comments below.

On Thu, Jan 13, 2022 at 9:46 AM Joseph CHAMG <josright123@xxxxxxxxx> wrote:

Missed commit message.

...

> v1-v4
> v5
> v6
> v7
> v8
> v9
> v10

Changelog should go after the cutter '--- ' line below, it's strange
that you did it correctly only for v11 changelog and not for the rest.

...

> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: Andrew Lunn <andrew@xxxxxxx>
> Cc: Leon Romanovsky <leon@xxxxxxxxxx>
> Cc: andy Shevchenko <andy.shevchenko@xxxxxxxxx>

Instead, utilize --cc parameter to `git send-email ...`

...

> Reported-by: kernel test robot <lkp@xxxxxxxxx>

New driver can't be reported.

> Signed-off-by: Joseph CHAMG <josright123@xxxxxxxxx>
> ---
> v11

...

> +config DM9051
> + tristate "DM9051 SPI support"
> + select PHYLIB
> + depends on SPI
> + select CRC32

Please group dependencies first followed by selections.
Also, you missed to select corresponding regmap APIs (SPI and MDIO you
mentioned).

...

> +static int dm9051_map_read(struct board_info *db, u8 reg, unsigned int *prb)
> +{
> + struct net_device *ndev = db->ndev;
> + int ret = regmap_read(db->regmap, reg, prb);
> +
> + if (unlikely(ret))
> + netif_err(db, drv, ndev, "%s: error %d reading reg %02x\n",
> + __func__, ret, reg);
> + return ret;
> +}
> +
> +static int dm9051_map_write(struct board_info *db, u8 reg, u16 val)
> +{
> + struct net_device *ndev = db->ndev;
> + int ret = regmap_write(db->regmap, reg, val);
> +
> + if (unlikely(ret))
> + netif_err(db, drv, ndev, "%s: error %d writing reg %02x=%04x\n",
> + __func__, ret, reg, val);
> + return ret;
> +}

Redefining callbacks for the sake of printing messages? Hmm... dunno
if it's a good idea here.

...

> + ret = dm9051_map_write(db, DM9051_EPDRL, val & 0xff); /* write ctl must once 8-bit */
> + if (ret < 0)
> + return ret;
> + ret = dm9051_map_write(db, DM9051_EPDRH, val >> 8); /* write ctl must once 8-bit */
> + if (ret < 0)
> + return ret;

Wouldn't be better to use bulk write for these?

...

> + ret = dm9051_map_read(db, DM9051_EPDRH, &eph); /* read ctl must once 8-bit */
> + if (ret)
> + return ret;
> + ret = dm9051_map_read(db, DM9051_EPDRL, &epl); /* read ctl must once 8-bit */
> + if (ret)
> + return ret;
> + *val = (eph << 8) | epl;

Wouldn't be better to use bulk read for these?

...

> +static bool dm9051_regmap_volatile(struct device *dev, unsigned int reg)
> +{
> + return true; /* true, register can not be cached */
> +}

Do you need this wrapper?

...

> + .lock = dm9051_reg_lock_mutex,
> + .unlock = dm9051_reg_unlock_mutex,

But this doesn't protect against interleaved accesses. Is it fine?

...

> +static int dm9051_map_updbits(struct board_info *db, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned int set_mask = val & mask;
> + unsigned int readd = 0; /* clear all insided bits */
> + int ret = 0;
> +
> + ret = regmap_read(db->regmap, reg, &readd);
> + if (ret < 0)
> + return ret;
> +
> + if ((readd & mask) != set_mask) {
> + readd &= ~mask;
> + readd |= set_mask;
> +
> + ret = regmap_write(db->regmap, reg, readd);
> + if (ret < 0)
> + return ret;
> + }
> + return ret;
> +}

NIH regmap_update_bits().


...

> +static bool dm9051_phymap_volatile(struct device *dev, unsigned int reg)
> +{
> + return true; /* true, register can not be cached */
> +}

Do you really need this?

...

> +static int dm9051_map_phy_updbits(struct board_info *db, unsigned int reg,
> + unsigned int mask, unsigned int val)
> +{
> + unsigned int set_mask = mask & val;
> + unsigned int readd = 0;
> + int ret;
> +
> + ret = ctrl_dm9051_phyread(db, reg, &readd);
> + if (ret)
> + return ret;
> +
> + if ((readd & mask) != set_mask) {
> + readd &= ~mask;
> + readd |= set_mask;
> + ret = ctrl_dm9051_phywrite(db, reg, readd);
> + if (ret)
> + return ret;
> + }
> + return ret;
> +}

NIH regmap_update_bits().

...

> + ret = dm9051_map_read(db, DM9051_EPDRL, &mval); /* must read once 8-bit */
> + if (ret < 0)
> + return ret;
> + to[0] = mval;
> + ret = dm9051_map_read(db, DM9051_EPDRH, &mval); /* must read once 8-bit */
> + if (ret < 0)
> + return ret;

Why not _bulk operation?

...

> + dm9051_map_write(db, DM9051_EPDRH, data[1]); /* must write once 8-bit */
> + dm9051_map_write(db, DM9051_EPDRL, data[0]); /* must write once 8-bit */

Ditto.

...

> + ret = dm9051_map_read(db, DM9051_PIDH, &wpidh);
> + if (ret < 0)
> + return ret;
> + ret = dm9051_map_read(db, DM9051_PIDL, &wpidl);
> + if (ret < 0)
> + return ret;

> + id = (wpidh << 8) | wpidl;

Ditto.

...

> + if (id == DM9051_ID) {
> + dev_info(dev, "chip %04x found\n", id);
> + return 0;
> + }
> +
> + dev_info(dev, "chipid error as %04x !\n", id);

Why not dev_err()?

> + return -ENODEV;

Please, use standard pattern, i.e. check for errors first:

if (error condition) {
...
return err;
}

...

> + for (i = 0; i < ETH_ALEN; i++) {
> + ret = dm9051_map_read(db, DM9051_PAR + i, &mval);
> + if (unlikely(ret))
> + return ret;
> + addr[i] = mval;
> + }

_bulk?

...

> + if (is_valid_ether_addr(addr)) {
> + eth_hw_addr_set(ndev, addr);
> + return 0;
> + }
> +
> + eth_hw_addr_random(ndev);
> + dev_dbg(&db->spidev->dev, "Use random MAC address\n");
> + return 0;

Check for (kinda) error first.

...

> + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
> + db->mdiobus->id, DM9051_PHY_ID);

(1)

> + db->phydev = phy_connect(db->ndev, phy_id, dm9051_handle_link_change,
> + PHY_INTERFACE_MODE_MII);

> +

This blank line is misplaced. Should be in (1).

> + if (IS_ERR(db->phydev))
> + return PTR_ERR(db->phydev);
> + return 0;

return PTR_ERR_OR_ZERO(...);

> +}

...

> + rdptr = (u8 *)skb_put(skb, rxlen - 4);

Do you need this casting?

...

> + ret = dm9051_map_write(db, DM9051_TXPLL, len);
> + if (ret < 0)
> + return ret;
> + ret = dm9051_map_write(db, DM9051_TXPLH, len >> 8);
> + if (ret < 0)
> + return ret;

_bulk?

...

> + /* these registers can't write by inblk, must write one by one
> + */

Why? regmap bulk does exactly this (if properly configured).

> + for (i = 0; i < ETH_ALEN; i++) {
> + result = dm9051_map_write(db, DM9051_PAR + i, ndev->dev_addr[i]);
> + if (result < 0)
> + goto spi_err;
> + }

...

> + /* these registers can't write by inblk, must write one by one
> + */

Ditto.

> + for (oft = DM9051_MAR, i = 0; i < 4; i++) {
> + result = dm9051_map_write(db, oft++, db->hash_table[i]);
> + if (result < 0)
> + goto spi_err;
> + result = dm9051_map_write(db, oft++, db->hash_table[i] >> 8);
> + if (result < 0)
> + goto spi_err;
> + }

...

> + db->hash_table[hash_val / 16] |= (u16)1 << (hash_val % 16);

Do you need casting? Can you use 1U or BIT()?

...

> +static int devm_regmap_init_dm9051(struct spi_device *spi, struct board_info *db)
> +{
> + regconfig.lock_arg = db; /* regmap lock/unlock essential */
> +
> + db->regmap = devm_regmap_init_spi(db->spidev, &regconfig);

> + if (IS_ERR(db->regmap))
> + return PTR_ERR(db->regmap);
> + return 0;

return PTR_ERR_OR_ZERO(...);

> +}

...

> + db->mdiobus->phy_mask = (u32)~BIT(1);

GENMASK()

...

> + ret = devm_mdiobus_register(&spi->dev, db->mdiobus);
> + if (ret < 0) {

What does > 0 mean?

> + dev_err(&spi->dev, "Could not register MDIO bus\n");

> + return ret;
> + }
> + return 0;

Can it be

return ret;

?

...

> + if (IS_ERR(db->phymap))
> + return PTR_ERR(db->phymap);
> + return 0;

As above.

...

> + db->kwr_task_kw = kthread_run(kthread_worker_fn, &db->kw, "dm9051");
> + if (IS_ERR(db->kwr_task_kw))
> + return PTR_ERR(db->kwr_task_kw);
> +
> + mutex_init(&db->spi_lock);
> + mutex_init(&db->reg_mutex);

Are you sure it's good to have thread running without initializations
of locks, etc?

...

> +static int dm9051_drv_remove(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct net_device *ndev = dev_get_drvdata(dev);
> + struct board_info *db = to_dm9051_board(ndev);
> +
> + phy_disconnect(db->phydev);
> + kthread_stop(db->kwr_task_kw);
> + return 0;
> +}

Seems like a wrong order of the resource freeing.

...

> +++ b/drivers/net/ethernet/davicom/dm9051.h

Do oyu need it to be a separate header?

...

> +#include <linux/bitfield.h>

Not sure I saw the user of this header below.

> +#include <linux/mutex.h>

> +#include <linux/netdevice.h>

...

> +struct rx_ctl_mach {
> + u16 status_err_counter; /* 'Status Err' counter */
> + u16 large_err_counter; /* 'Large Err' counter */
> + u16 DO_FIFO_RST_counter; /* 'fifo_reset' counter */

Can you rather have these comments in kernel doc?

> +};

...

> +struct board_info

Why do you need this definition in the header?

--
With Best Regards,
Andy Shevchenko