Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support

+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/cache.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#include <linux/spi/spi.h>
+#include <linux/of_net.h>
+
+#define MSG_DEFAULT	(NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_LINK | \
+			 NETIF_MSG_TIMER)
+
+#define DRV_NAME	"mse102x"
+
+#define DET_CMD		0x0001
+#define DET_SOF		0x0002
+#define DET_DFT		0x55AA
+
+#define CMD_SHIFT	12
+#define CMD_RTS		(0x1 << CMD_SHIFT)
+#define CMD_CTR		(0x2 << CMD_SHIFT)
+
+#define CMD_MASK	GENMASK(15, CMD_SHIFT)
+#define LEN_MASK	GENMASK(CMD_SHIFT - 1, 0)
+
+#define	DET_CMD_LEN	4
+#define	DET_SOF_LEN	2
+#define	DET_DFT_LEN	2

Looks like these tabs should be spaces?

+static int msg_enable;
+module_param_named(message, msg_enable, int, 0);
+MODULE_PARM_DESC(message, "Message verbosity level (0=none, 31=all)");

I know a lot of drivers do this, but module parameters are not
liked. There is a well used ethtool setting for this, msglvl, which
should be used instead. Which in fact, you have support for.

+static void mse102x_init_mac(struct mse102x_net *mse, struct device_node *np)
+{
+	struct net_device *ndev = mse->ndev;
+	int ret = of_get_mac_address(np, ndev->dev_addr);
+
+	if (ret) {
+		eth_hw_addr_random(ndev);
+		netdev_err(ndev, "Using random MAC address: %pM\n",
+			   ndev->dev_addr);
+	}
+}

No need to tell the hardware? Does it work in promiscuous mode by
default?

+static int mse102x_net_stop(struct net_device *ndev)
+{
+	struct mse102x_net *mse = netdev_priv(ndev);
+	struct mse102x_net_spi *mses = to_mse102x_spi(mse);
+
+	netif_info(mse, ifdown, ndev, "shutting down\n");
+
+	netif_stop_queue(ndev);
+
+	/* stop any outstanding work */
+	flush_work(&mses->tx_work);
+
+	/* ensure any queued tx buffers are dumped */
+	while (!skb_queue_empty(&mse->txq)) {
+		struct sk_buff *txb = skb_dequeue(&mse->txq);
+
+		netif_dbg(mse, ifdown, ndev,
+			  "%s: freeing txb %p\n", __func__, txb);
+
+		dev_kfree_skb(txb);
+	}
+
+	free_irq(ndev->irq, mse);
+
+	return 0;

Maybe a netif_carrier_off() in there, to be symmetric with open?

+/* ethtool support */
+
+static void mse102x_get_drvinfo(struct net_device *ndev,
+				struct ethtool_drvinfo *di)
+{
+	strscpy(di->driver, DRV_NAME, sizeof(di->driver));
+	strscpy(di->version, "1.00", sizeof(di->version));
+	strscpy(di->bus_info, dev_name(ndev->dev.parent), sizeof(di->bus_info));
+}

Version is pretty pointless. We suggest you don't use it. The ethtool
core will then fill it with the kernel version, 

+static int mse102x_probe_spi(struct spi_device *spi)
+{

...

+	netif_carrier_off(mse->ndev);
+	ndev->if_port = IF_PORT_10BASET;

That is not correct. Maybe you should add a IF_PORT_HOMEPLUG ?

+	ndev->netdev_ops = &mse102x_netdev_ops;
+	ndev->ethtool_ops = &mse102x_ethtool_ops;
+
+	mse102x_init_mac(mse, dev->of_node);
+
+	ret = register_netdev(ndev);
+	if (ret) {
+		dev_err(dev, "failed to register network device: %d\n", ret);
+		return ret;
+	}
+
+	mse102x_init_device_debugfs(mses);
+
+	return 0;
+}

+static const struct of_device_id mse102x_match_table[] = {
+	{ .compatible = "vertexcom,mse1021" },
+	{ .compatible = "vertexcom,mse1022" },

Is there an ID register you can read to determine what device you
actually have? If so, i suggest you verify the correct compatible is
used.

	Andrew