Re: [PATCH 2/2] net: Add DM9051 driver

[Andrew Lunn <andrew@lunn.ch>]

> +static int burst_xfer(struct board_info *db, u8 cmdphase, u8 *txb, u8 *rxb, unsigned int len)
> +static int dm_phy_read_func(struct board_info *db, int reg)
> +static int dm9051_phy_read_lock(struct net_device *dev, int phy_reg_unused, int reg)

Please be consistent with your namespace. Please use the dm9051_
prefix everywhere.

> +{
> +	int ret;
> +
> +	iow(db, DM9051_EPAR, DM9051_PHY | reg);
> +	iow(db, DM9051_EPCR, EPCR_ERPRR | EPCR_EPOS);
> +	while (ior(db, DM9051_EPCR) & EPCR_ERRE)
> +		;

include/linux/iopoll.h No potentially endless loops please.

> +static unsigned int dm9051_chipid(struct device *dev, struct board_info *db)
> +{
> +	unsigned int chipid;
> +
> +	chipid = iior(dev, db, DM9051_PIDL);
> +	chipid |= (unsigned int)iior(dev, db, DM9051_PIDH) << 8;
> +	if (chipid == (DM9051_ID >> 16))
> +		return chipid;
> +	chipid = iior(dev, db, DM9051_PIDL);
> +	chipid |= (unsigned int)iior(dev, db, DM9051_PIDH) << 8;
> +	if (chipid == (DM9051_ID >> 16))
> +		return chipid;
> +	dev_info(dev, "Read [DM9051_PID] = %04x\n", chipid);
> +	dev_info(dev, "Read [DM9051_PID] error!\n");
> +	return 0;

If this is an error case, returning 0 is not a good idea. -ENODEV?

> +static void dm9051_phy_advertise_pausecap_func(struct board_info *db)
> +{
> +	int phy4 = dm_phy_read_func(db, MII_ADVERTISE); //DBG_20140407
> +
> +	dm_phy_write_func(db, MII_ADVERTISE, phy4 | ADVERTISE_PAUSE_CAP); // dm95 flow-control RX!
> +}

Use seem to be using the deprecated mii code. Please replace it with
phylib. phylib will then do things like advertising.

> +static void dm9051_reset(struct board_info *db)
> +{
> +	mdelay(2); //delay 2 ms any need before NCR_RST (20170510)

Since this is a new driver, there is no history. Comments like
20170510 don't add anything useful.

> +static void IMR_DISABLE_LOCK_ESSENTIAL(struct board_info *db)

Don't use upper case for functions.

> +{
> +	ADDR_LOCK_HEAD_ESSENTIAL(db);
> +	imr_reg_stop(db);
> +	ADDR_LOCK_TAIL_ESSENTIAL(db);
> +}

> +static int dm9051_read_mac_to_dev(struct device *dev, struct net_device *ndev,
> +				  struct board_info *db)
> +{
> +	int i;
> +	u8 mac_fix[ETH_ALEN] = { 0x00, 0x60, 0x6e, 0x90, 0x51, 0xee };
> +
> +	for (i = 0; i < ETH_ALEN; i++)
> +		ndev->dev_addr[i] = ior(db, DM9051_PAR + i);
> +	if (!is_valid_ether_addr(ndev->dev_addr)) {

You should use a random MAC address. eth_hw_addr_random(ndev);


> +		for (i = 0; i < ETH_ALEN; i++)
> +			iow(db, DM9051_PAR + i, mac_fix[i]);
> +		for (i = 0; i < ETH_ALEN; i++)
> +			ndev->dev_addr[i] = ior(db, DM9051_PAR + i);
> +		dev_info(dev, "dm9 [reg_netdev][%s][chip MAC: %pM (%s)]\n",
> +			 ndev->name, ndev->dev_addr, "FIX-1");

dev_dbg() ?

> +		return 0;
> +	}
> +	return 1;

If this is an error, please use an error code.

> +static void dm_set_mac_lock(struct board_info *db)
> +{
> +	struct net_device *ndev = db->ndev;
> +
> +	if (db->enter_setmac) {
> +		int i, oft;
> +
> +		db->enter_setmac = 0;
> +		netdev_info(ndev, "set_mac_address %02x %02x %02x  %02x %02x %02x\n",
> +			    ndev->dev_addr[0], ndev->dev_addr[1], ndev->dev_addr[2],
> +			    ndev->dev_addr[3], ndev->dev_addr[4], ndev->dev_addr[5]);

netdev_dbg()

Also, %pM can be used here.

> +static
> +const struct net_device_ops dm9051_netdev_ops = {
> +	.ndo_open = dm9051_open,
> +	.ndo_stop = dm9051_stop,
> +	.ndo_start_xmit = DM9051_START_XMIT,
> +	.ndo_set_rx_mode = dm9051_set_multicast_list_schedule,

_schedule? This is called in a context you can make blocking calls. So
i don't see why you need the work queue.

> +static void dm9051_get_drvinfo(struct net_device *dev,
> +			       struct ethtool_drvinfo *info)
> +{
> +	struct board_info *dm = to_dm9051_board(dev);
> +
> +	strscpy(info->driver, DRVNAME_9051, sizeof(info->driver));
> +	strscpy(info->version, dm->DRV_VERSION, sizeof(info->version));

version is meaningless. Please leave it empty, and the core will fill
it in with the kernel version.

> +	strscpy(info->bus_info, dev_name(dev->dev.parent), sizeof(info->bus_info));
> +}
> +
> +/* LNX_KERNEL_v58 */

58? We are only at version 5 at the moment?

> +/* Tips: reset and increase the RST counter
> + */

Tips? 

> +static void dm9051_fifo_reset(u8 state, u8 *hstr, struct board_info *db)
> +{
> +	db->bc.DO_FIFO_RST_counter++;
> +	dm9051_reset(db);
> +}
> +
> +static void dm9051_reset_dm9051(struct board_info *db, int rxlen)
> +{
> +	struct net_device *ndev = db->ndev;
> +	char *sbuff = (char *)db->prxhdr;
> +	char hstr[72];
> +
> +	netdev_info(ndev, "dm9-pkt-Wrong RxLen over-range (%x= %d > %x= %d)\n",
> +		    rxlen, rxlen, DM9051_PKT_MAX, DM9051_PKT_MAX);
> +
> +	db->bc.large_err_counter++;
> +	db->bc.mac_ovrsft_counter++; //increase the MAC over_shift counter
> +	dm9051_fifo_reset(11, hstr, db);
> +	sprintf(hstr, "dmfifo_reset( 11 RxLenErr ) rxhdr %02x %02x %02x %02x (quick)",
> +		sbuff[0], sbuff[1], sbuff[2], sbuff[3]);
> +	netdev_info(ndev, "%s\n", hstr);
> +	netdev_info(ndev, "dm9 reset-done: of LargeRxLen\n");
> +	netdev_info(ndev, " RxLenErr&MacOvrSft_Er %d, RST_c %d\n",
> +		    db->bc.mac_ovrsft_counter,
> +		    db->bc.DO_FIFO_RST_counter);

There is a lot of kernel log spamming going on here. Please either
remove this, of use netdev_dbg(). Please look throught all the uses of
_info() and see if they are actually useful, or should be removed.

> +#define dm_msg_open_done(nd, maddr, irqn) \
> +	netdev_info(nd, "[dm_open] %pM irq_no %d ACTIVE_LOW\n", maddr, irqn)

Please don't use macros like this.

> +#define init_sched_phy(db) schedule_delayed_work(&(db)->phy_poll, HZ * 1)

No, no, no.

This code feels like it is a vendor crap driver, with an abstraction
over the OS. That is not how Linux drivers should work. If you need to
lock a mutux, call the mutex_lock(), not a wrapper around
mutex_lock. Please remove all these wrapper.

	    Andrew