Re: [PATCH v17, 2/2] net: Add dm9051 driver
From: Andy Shevchenko
Date: Wed Feb 02 2022 - 14:46:54 EST
On Wed, Feb 2, 2022 at 8:13 PM Joseph CHAMG <josright123@xxxxxxxxx> wrote:
>
> Add davicom dm9051 spi ethernet driver, The driver work for the
> device platform which has the spi master
...
> + ret = regmap_write(db->regmap_dm, DM9051_EPCR, 0x0);
0x0 --> 0
> + if (ret)
> + return ret;
...
> + return regmap_write(db->regmap_dm, DM9051_EPCR, 0x0);
Ditto.
...
> + ret = regmap_update_bits(db->regmap_dm, DM9051_FCR, 0xff, fcr);
GENMASK(7, 0) ?
...
> +static int dm9051_map_chipid(struct board_info *db)
> +{
> + struct device *dev = &db->spidev->dev;
> + unsigned int ret;
> + unsigned short wid;
> + u8 buff[6];
> +
> + ret = regmap_bulk_read(db->regmap_dmbulk, DM9051_VIDL, buff, 6);
sizeof(buff)
> + if (ret < 0) {
> + netif_err(db, drv, db->ndev, "%s: error %d bulk_read reg %02x\n",
> + __func__, ret, DM9051_VIDL);
> + return ret;
> + }
> + wid = buff[3] << 8 | buff[2];
get_unaligned_le16()
> + if (wid != DM9051_ID) {
> + dev_err(dev, "chipid error as %04x !\n", wid);
> + return -ENODEV;
> + }
> +
> + dev_info(dev, "chip %04x found\n", wid);
> + return 0;
> +}
...
> +static int dm9051_map_etherdev_par(struct net_device *ndev, struct board_info *db)
> +{
> + u8 addr[ETH_ALEN];
> + int ret;
> +
> + ret = regmap_bulk_read(db->regmap_dmbulk, DM9051_PAR, addr, ETH_ALEN);
sizeof(addr)
> + if (ret < 0)
> + return ret;
> +
> + if (!is_valid_ether_addr(addr)) {
> + eth_hw_addr_random(ndev);
> + ret = regmap_bulk_write(db->regmap_dmbulk, DM9051_PAR, ndev->dev_addr, ETH_ALEN);
Ditto.
> + if (ret < 0)
> + return ret;
> +
> + dev_dbg(&db->spidev->dev, "Use random MAC address\n");
> + return 0;
> + }
> +
> + eth_hw_addr_set(ndev, addr);
> + return 0;
> +}
...
> + db->mdiobus->phy_mask = (u32)~GENMASK(1, 1);
For a single bit it might be better to use BIT(1)
...
> + ret = devm_mdiobus_register(&spi->dev, db->mdiobus);
> + if (ret)
> + dev_err(&spi->dev, "Could not register MDIO bus\n");
> +
> + return 0;
return ret; ?
...
> + snprintf(phy_id, MII_BUS_ID_SIZE + 3, PHY_ID_FMT,
sizeof(phy_id) + 3
> + db->mdiobus->id, DM9051_PHY_ADDR);
...
> + if (IS_ERR(db->phydev))
> + return PTR_ERR(db->phydev);
> + return 0;
return PTR_ERR_OR_ZERO();
...
> + if ((rxbyte & 0xff) != DM9051_PKT_RDY)
GENMASK(7, 0) ?
> + break; /* exhaust-empty */
...
> + ret = regmap_write(db->regmap_dm, DM9051_ISR, 0xff); /* to stop mrcmd */
Ditto.
> + if (ret)
> + return ret;
...
> + ret = regmap_write(db->regmap_dm, DM9051_ISR, 0xff); /* to stop mrcmd */
Ditto.
Perhaps it needs its own definition after all?
> + if (ret)
> + return ret;
...
> +spi_err:
out_unlock:
> + mutex_unlock(&db->spi_lockm);
> +
> + return IRQ_HANDLED;
...
> + result = regmap_bulk_write(db->regmap_dmbulk, DM9051_MAR, db->rctl.hash_table, 8);
Is hash_table an array? Then sizeof() or ARRAY_SIZE() should work.
> + if (result < 0) {
> + netif_err(db, drv, ndev, "%s: error %d bulk writing reg %02x, len %d\n",
> + __func__, result, DM9051_MAR, 8);
Ditto.
> + goto spi_err;
> + }
...
> +spi_err:
out_unlock:
You need to describe what will be done if one goes to the label.
> + mutex_unlock(&db->spi_lockm);
...
> + /* The whole dm9051 chip registers could not be accessed within 1 ms
> + * after above GPR power on control
> + */
> + mdelay(1);
Why atomic?
...
> + ret = request_threaded_irq(spi->irq, NULL, dm9051_rx_threaded_irq,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
Why do you ignore IRQ flags from DT?
> + ndev->name, db);
...
> + /* the multicast address in Hash Table : 64 bits */
> + netdev_for_each_mc_addr(ha, ndev) {
> + hash_val = ether_crc_le(ETH_ALEN, ha->addr) & 0x3f;
GENMASK() ?
> + rxctrl.hash_table[hash_val / 16] |= 1U << (hash_val % 16);
BIT() ?
> + }
...
> + /* schedule work to do the actual set of the data if needed */
> + if (memcmp(&db->rctl, &rxctrl, sizeof(rxctrl))) {
> + memcpy(&db->rctl, &rxctrl, sizeof(rxctrl));
Hmm... This is interesting...
> + schedule_work(&db->rxctrl_work);
> + }
...
> + ret = regmap_bulk_write(db->regmap_dmbulk, DM9051_PAR, ndev->dev_addr, ETH_ALEN);
sizeof() ?
...
> + int ret = 0;
Redundant assignment.
...
> + ret = devm_register_netdev(dev, ndev);
> + if (ret) {
> + dev_err(dev, "failed to register network device\n");
> + phy_disconnect(db->phydev);
> + return ret;
phy_disconnect();
return dev_err_probe();
> + }
...
> +err_stopthread:
> + return ret;
Useless. Return in-place.
...
> +static struct spi_driver dm9051_driver = {
> + .driver = {
> + .name = DRVNAME_9051,
> + .of_match_table = dm9051_match_table,
> + },
> + .probe = dm9051_probe,
> + .remove = dm9051_drv_remove,
> + .id_table = dm9051_id_table,
> +};
> +
Redundant blank line.
> +module_spi_driver(dm9051_driver);
...
+ bits.h
> +#include <linux/netdevice.h>
> +#include <linux/mii.h>
How is that header being used in this header?
> +#include <linux/types.h>
...
> +#define INTCR_POL_LOW BIT(0)
> +#define INTCR_POL_HIGH (0 << 0)
In this case, be consistent:
#define INTCR_POL_LOW (1 << 0)
#define INTCR_POL_HIGH (0 << 0)
--
With Best Regards,
Andy Shevchenko