LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Andrew Lunn <andrew@lunn.ch>
To: Stefan Wahren <stefan.wahren@i2se.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Michael Heimpold <michael.heimpold@in-tech.com>,
	jimmy.shen@vertexcom.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
Subject: Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support
Date: Wed, 15 Sep 2021 23:17:05 +0200	[thread overview]
Message-ID: <YUJi0cVawjyiteEx@lunn.ch> (raw)
In-Reply-To: <20210914151717.12232-4-stefan.wahren@i2se.com>

> +#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

  parent reply	other threads:[~2021-09-15 21:17 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 15:17 [PATCH RFC 0/3] add Vertexcom MSE102x support Stefan Wahren
2021-09-14 15:17 ` [PATCH RFC 1/3] dt-bindings: add vendor Vertexcom Stefan Wahren
2021-09-21 20:55   ` Rob Herring
2021-09-14 15:17 ` [PATCH RFC 2/3] dt-bindings: net: add Vertexcom MSE102x support Stefan Wahren
2021-09-21 21:09   ` Rob Herring
2021-09-14 15:17 ` [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support Stefan Wahren
2021-09-15  2:55   ` Jakub Kicinski
2021-09-15 21:17   ` Andrew Lunn [this message]
2021-09-16 11:12     ` Stefan Wahren
2021-09-16 12:35       ` Andrew Lunn
2021-09-21 21:07         ` Rob Herring
2021-09-16 11:26     ` Michael Heimpold
2021-09-16 12:46       ` Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YUJi0cVawjyiteEx@lunn.ch \
    --to=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=devicetree@vger.kernel.org \
    --cc=jimmy.shen@vertexcom.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=michael.heimpold@in-tech.com \
    --cc=netdev@vger.kernel.org \
    --cc=robh+dt@kernel.org \
    --cc=stefan.wahren@i2se.com \
    --subject='Re: [PATCH RFC 3/3] net: vertexcom: Add MSE102x SPI support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).