Thread (6 messages) 6 messages, 4 authors, 2011-02-16

Re: [PATCH v2] sh: sh_eth: Add support ethtool

flat view

From: Ben Hutchings <hidden>
Date: 2011-01-11 15:57:06
Also in: linux-sh

On Tue, 2011-01-11 at 20:58 +0900, nobuhiro.iwamatsu.yj@renesas.com
wrote:
quoted hunk ↗ jump to hunk
From: Nobuhiro Iwamatsu <redacted>

This commit supports following functions.
 - get_drvinfo
 - get_settings
 - set_settings
 - nway_reset
 - get_msglevel
 - set_msglevel
 - get_link
 - get_strings
 - get_ethtool_stats
 - get_sset_count

About other function, the device does not support.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Signed-off-by: Nobuhiro Iwamatsu <redacted>
---
 v2: reverted one part of the checks of checkpatch.pl.
       foo *bar -> foo * bar.
	 changed function copying of net_device_stats from *for* to memcopy.
 
 drivers/net/sh_eth.c |  186 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 174 insertions(+), 12 deletions(-)
diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
index 819c175..0b2cb7d 100644
--- a/drivers/net/sh_eth.c
+++ b/drivers/net/sh_eth.c
[...]
quoted hunk ↗ jump to hunk
@@ -1063,6 +1074,154 @@ static int sh_eth_phy_start(struct net_device *ndev)
 	return 0;
 }
 
+static void sh_eth_get_drvinfo(struct net_device *ndev,
+			struct ethtool_drvinfo *info)
+{
+	strncpy(info->driver, "sh_eth", sizeof(info->driver) - 1);
+	strcpy(info->version, "N/A");
+	strcpy(info->fw_version, "N/A");
+	strlcpy(info->bus_info, dev_name(ndev->dev.parent),
+		sizeof(info->bus_info));
+}
This is redundant; the default implementation already does this.

[...]
quoted hunk ↗ jump to hunk
+static int sh_eth_set_settings(struct net_device *ndev,
+		struct ethtool_cmd *ecmd)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+	unsigned long flags;
+	int ret;
+	u32 ioaddr = ndev->base_addr;
+
+	spin_lock_irqsave(&mdp->lock, flags);
+
+	/* disable tx and rx */
+	sh_eth_linkdown(ioaddr);
+
+	ret = phy_ethtool_sset(mdp->phydev, ecmd);
+	if (ret)
+		goto error_exit;
+
+	if (ecmd->duplex == DUPLEX_FULL)
+		mdp->duplex = 1;
+	else
+		mdp->duplex = 0;
+
+	if (mdp->cd->set_duplex)
+		mdp->cd->set_duplex(ndev);
+
+error_exit:
+	mdelay(100);
Ugh, 100 ms holding a spinlock?!
+	/* enable tx and rx */
+	sh_eth_linkup(ioaddr);
How do you know the link is up?  Shouldn't this be left to the link
polling function?

[...]
quoted hunk ↗ jump to hunk
+static u32 sh_eth_get_msglevel(struct net_device *ndev)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+	return mdp->msg_enable;
+}
+
+static void sh_eth_set_msglevel(struct net_device *ndev, u32 value)
+{
+	struct sh_eth_private *mdp = netdev_priv(ndev);
+	mdp->msg_enable = value;
+}
This would be more useful if msg_enable was actually used anywhere in
the driver.

[...]
quoted hunk ↗ jump to hunk
@@ -1073,8 +1232,8 @@ static int sh_eth_open(struct net_device *ndev)
 
 	ret = request_irq(ndev->irq, sh_eth_interrupt,
 #if defined(CONFIG_CPU_SUBTYPE_SH7763) || \
-    defined(CONFIG_CPU_SUBTYPE_SH7764) || \
-    defined(CONFIG_CPU_SUBTYPE_SH7757)
+	defined(CONFIG_CPU_SUBTYPE_SH7764) || \
+	defined(CONFIG_CPU_SUBTYPE_SH7757)
 				IRQF_SHARED,
 #else
 				0,
@@ -1232,11 +1391,11 @@ static int sh_eth_close(struct net_device *ndev)
 	sh_eth_ring_free(ndev);
 
 	/* free DMA buffer */
-	ringsize = sizeof(struct sh_eth_rxdesc) * RX_RING_SIZE;
+	ringsize = sizeof(struct sh_eth_rxdesc) *RX_RING_SIZE;
 	dma_free_coherent(NULL, ringsize, mdp->rx_ring, mdp->rx_desc_dma);
 
 	/* free DMA buffer */
-	ringsize = sizeof(struct sh_eth_txdesc) * TX_RING_SIZE;
+	ringsize = sizeof(struct sh_eth_txdesc) *TX_RING_SIZE;
 	dma_free_coherent(NULL, ringsize, mdp->tx_ring, mdp->tx_desc_dma);
 
 	pm_runtime_put_sync(&mdp->pdev->dev);
Please do not include these space changes.
quoted hunk ↗ jump to hunk
@@ -1497,8 +1656,11 @@ static int sh_eth_drv_probe(struct platform_device *pdev)
 
 	/* set function */
 	ndev->netdev_ops = &sh_eth_netdev_ops;
+	SET_ETHTOOL_OPS(ndev, &sh_eth_ethtool_ops);
 	ndev->watchdog_timeo = TX_TIMEOUT;
 
+	/* debug message level */
+	mdp->msg_enable = (1 << 3) - 1;
If you're actually going to *use* msg_enable, its value should be
initialised in terms of the NETIF_MSG_* flags defined in
<linux/netdevice.h>.
quoted hunk ↗ jump to hunk
 	mdp->post_rx = POST_RX >> (devno << 1);
 	mdp->post_fw = POST_FW >> (devno << 1);
 
@@ -1572,7 +1734,7 @@ static int sh_eth_runtime_nop(struct device *dev)
 	return 0;
 }
 
-static struct dev_pm_ops sh_eth_dev_pm_ops = {
+static const struct dev_pm_ops sh_eth_dev_pm_ops = {
 	.runtime_suspend = sh_eth_runtime_nop,
 	.runtime_resume = sh_eth_runtime_nop,
 };
This is worthwhile but unrelated to ethtool!

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help