[PATCH] ep93xx_eth.c: general cleanup
H Hartley Sweeten
1) Use pr_fmt() to prefix the module name and __func__ to the error
messages.
2) <linux/io.h> instead of <asm/io.h>
3) <mach/hardware.h> instead of <mach/ep93xx-regs.h> and <mach/platform.h>
4) Remove the DRV_MODULE_NAME and DRV_MODULE_VERSION defines and just use
the raw string data in ep93xx_get_drvinfo().
5) Move the ep93xx_mdio_read (and ep93xx_mdio_write) function to eliminate
the function prototype.
6) Change all the printk(<level> messages to pr_<level> and remove the
__func__ argument.
7) Use %pM to print the MAC address at the end of the probe.
8) Use dev->dev_addr not data->dev_addr for the MAC argument because a
random address could be used if the platform does not supply one.
9) Remove unnecessary noise in ep93xx_eth_init_module().
The message at the end of the probe is left as a printk since it displays
cleaner without the function name that would be displayed with pr_info().
Signed-off-by: H Hartley Sweeten <hswe...@visionengravers.com>
Cc: Lennert Buytenhek <ker...@wantstofly.org>
Cc: David S. Miller <da...@davemloft.net>
---
diff --git a/drivers/net/arm/ep93xx_eth.c b/drivers/net/arm/ep93xx_eth.c
index b25467a..8955850 100644
--- a/drivers/net/arm/ep93xx_eth.c
+++ b/drivers/net/arm/ep93xx_eth.c
@@ -9,6 +9,8 @@
* (at your option) any later version.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
+
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/kernel.h>
@@ -20,12 +22,9 @@
#include <linux/moduleparam.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
-#include <mach/ep93xx-regs.h>
-#include <mach/platform.h>
-#include <asm/io.h>
+#include <linux/io.h>
-#define DRV_MODULE_NAME "ep93xx-eth"
-#define DRV_MODULE_VERSION "0.1"
+#include <mach/hardware.h>
#define RX_QUEUE_ENTRIES 64
#define TX_QUEUE_ENTRIES 8
@@ -185,7 +184,47 @@ struct ep93xx_priv
#define wrw(ep, off, val) __raw_writew((val), (ep)->base_addr + (off))
#define wrl(ep, off, val) __raw_writel((val), (ep)->base_addr + (off))
-static int ep93xx_mdio_read(struct net_device *dev, int phy_id, int reg);
+static int ep93xx_mdio_read(struct net_device *dev, int phy_id, int reg)
+{
+ struct ep93xx_priv *ep = netdev_priv(dev);
+ int data;
+ int i;
+
+ wrl(ep, REG_MIICMD, REG_MIICMD_READ | (phy_id << 5) | reg);
+
+ for (i = 0; i < 10; i++) {
+ if ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY) == 0)
+ break;
+ msleep(1);
+ }
+
+ if (i == 10) {
+ pr_info("mdio read timed out\n");
+ data = 0xffff;
+ } else {
+ data = rdl(ep, REG_MIIDATA);
+ }
+
+ return data;
+}
+
+static void ep93xx_mdio_write(struct net_device *dev, int phy_id, int reg, int data)
+{
+ struct ep93xx_priv *ep = netdev_priv(dev);
+ int i;
+
+ wrl(ep, REG_MIIDATA, data);
+ wrl(ep, REG_MIICMD, REG_MIICMD_WRITE | (phy_id << 5) | reg);
+
+ for (i = 0; i < 10; i++) {
+ if ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY) == 0)
+ break;
+ msleep(1);
+ }
+
+ if (i == 10)
+ pr_info("mdio write timed out\n");
+}
static struct net_device_stats *ep93xx_get_stats(struct net_device *dev)
{
@@ -217,14 +256,11 @@ static int ep93xx_rx(struct net_device *dev, int processed, int budget)
rstat->rstat1 = 0;
if (!(rstat0 & RSTAT0_EOF))
- printk(KERN_CRIT "ep93xx_rx: not end-of-frame "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_crit("not end-of-frame %.8x %.8x\n", rstat0, rstat1);
if (!(rstat0 & RSTAT0_EOB))
- printk(KERN_CRIT "ep93xx_rx: not end-of-buffer "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_crit("not end-of-buffer %.8x %.8x\n", rstat0, rstat1);
if ((rstat1 & RSTAT1_BUFFER_INDEX) >> 16 != entry)
- printk(KERN_CRIT "ep93xx_rx: entry mismatch "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_crit("entry mismatch %.8x %.8x\n", rstat0, rstat1);
if (!(rstat0 & RSTAT0_RWE)) {
ep->stats.rx_errors++;
@@ -241,8 +277,7 @@ static int ep93xx_rx(struct net_device *dev, int processed, int budget)
length = rstat1 & RSTAT1_FRAME_LENGTH;
if (length > MAX_PKT_SIZE) {
- printk(KERN_NOTICE "ep93xx_rx: invalid length "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_notice("invalid length %.8x %.8x\n", rstat0, rstat1);
goto err;
}
@@ -371,11 +406,9 @@ static void ep93xx_tx_complete(struct net_device *dev)
tstat->tstat0 = 0;
if (tstat0 & TSTAT0_FA)
- printk(KERN_CRIT "ep93xx_tx_complete: frame aborted "
- " %.8x\n", tstat0);
+ pr_crit("frame aborted %.8x\n", tstat0);
if ((tstat0 & TSTAT0_BUFFER_INDEX) != entry)
- printk(KERN_CRIT "ep93xx_tx_complete: entry mismatch "
- " %.8x\n", tstat0);
+ pr_crit("entry mismatch %.8x\n", tstat0);
if (tstat0 & TSTAT0_TXWE) {
int length = ep->descs->tdesc[entry].tdesc1 & 0xfff;
@@ -536,7 +569,7 @@ static int ep93xx_start_hw(struct net_device *dev)
}
if (i == 10) {
- printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
+ pr_crit("hw failed to reset\n");
return 1;
}
@@ -581,7 +614,7 @@ static int ep93xx_start_hw(struct net_device *dev)
}
if (i == 10) {
- printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to start\n");
+ pr_crit("hw failed to start\n");
return 1;
}
@@ -617,7 +650,7 @@ static void ep93xx_stop_hw(struct net_device *dev)
}
if (i == 10)
- printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
+ pr_crit("hw failed to reset\n");
}
static int ep93xx_open(struct net_device *dev)
@@ -681,52 +714,10 @@ static int ep93xx_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
return generic_mii_ioctl(&ep->mii, data, cmd, NULL);
}
-static int ep93xx_mdio_read(struct net_device *dev, int phy_id, int reg)
-{
- struct ep93xx_priv *ep = netdev_priv(dev);
- int data;
- int i;
-
- wrl(ep, REG_MIICMD, REG_MIICMD_READ | (phy_id << 5) | reg);
-
- for (i = 0; i < 10; i++) {
- if ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY) == 0)
- break;
- msleep(1);
- }
-
- if (i == 10) {
- printk(KERN_INFO DRV_MODULE_NAME ": mdio read timed out\n");
- data = 0xffff;
- } else {
- data = rdl(ep, REG_MIIDATA);
- }
-
- return data;
-}
-
-static void ep93xx_mdio_write(struct net_device *dev, int phy_id, int reg, int data)
-{
- struct ep93xx_priv *ep = netdev_priv(dev);
- int i;
-
- wrl(ep, REG_MIIDATA, data);
- wrl(ep, REG_MIICMD, REG_MIICMD_WRITE | (phy_id << 5) | reg);
-
- for (i = 0; i < 10; i++) {
- if ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY) == 0)
- break;
- msleep(1);
- }
-
- if (i == 10)
- printk(KERN_INFO DRV_MODULE_NAME ": mdio write timed out\n");
-}
-
static void ep93xx_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
{
- strcpy(info->driver, DRV_MODULE_NAME);
- strcpy(info->version, DRV_MODULE_VERSION);
+ strcpy(info->driver, "ep93xx-eth");
+ strcpy(info->version, "0.1");
}
static int ep93xx_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
@@ -877,11 +868,8 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
goto err_out;
}
- printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, "
- "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
- ep->irq, data->dev_addr[0], data->dev_addr[1],
- data->dev_addr[2], data->dev_addr[3],
- data->dev_addr[4], data->dev_addr[5]);
+ printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, %pM\n",
+ dev->name, ep->irq, dev->dev_addr);
return 0;
@@ -902,7 +890,6 @@ static struct platform_driver ep93xx_eth_driver = {
static int __init ep93xx_eth_init_module(void)
{
- printk(KERN_INFO DRV_MODULE_NAME " version " DRV_MODULE_VERSION " loading\n");
return platform_driver_register(&ep93xx_eth_driver);
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Joe Perches
> +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
I think KBUILD_MODNAME ":%s: " fmt, __func__
or its equivalent direct use is more common.
It's possibly useful to standardize.
> -#define DRV_MODULE_NAME "ep93xx-eth"
> -#define DRV_MODULE_VERSION "0.1"
> +#include <mach/hardware.h>
I think these sorts of defines are common and useful.
> static int __init ep93xx_eth_init_module(void)
> {
> - printk(KERN_INFO DRV_MODULE_NAME " version " DRV_MODULE_VERSION " loading\n");
Why remove this printk completely?
H Hartley Sweeten
1) Use pr_fmt() to prefix the module name and __func__ to the error
messages.
2) <linux/io.h> instead of <asm/io.h>
3) <mach/hardware.h> instead of <mach/ep93xx-regs.h> and <mach/platform.h>
4) Remove the DRV_MODULE_NAME and DRV_MODULE_VERSION defines and just use
the raw string data in ep93xx_get_drvinfo().
5) Move the ep93xx_mdio_read (and ep93xx_mdio_write) function to eliminate
the function prototype.
6) Change all the printk(<level> messages to pr_<level> and remove the
__func__ argument.
7) Use platform_get_{resource/irq} to get the platform resources and add
an error check.
8) Use resource_size() for request_mem_region() and ioremap().
9) Use %pM to print the MAC address at the end of the probe.
10) Use dev->dev_addr not data->dev_addr for the MAC argument because a
random address could be used if the platform does not supply one.
11) Remove unnecessary noise in ep93xx_eth_init_module().
The message at the end of the probe is left as a printk since it displays
cleaner without the function name that would be displayed with pr_info().
Signed-off-by: H Hartley Sweeten <hswe...@visionengravers.com>
Cc: Lennert Buytenhek <ker...@wantstofly.org>
Cc: David S. Miller <da...@davemloft.net>
---
V2 - Missed the resource check and resource_size()
diff --git a/drivers/net/arm/ep93xx_eth.c b/drivers/net/arm/ep93xx_eth.c
index b25467a..c949f9a 100644
--- a/drivers/net/arm/ep93xx_eth.c
+++ b/drivers/net/arm/ep93xx_eth.c
@@ -9,6 +9,8 @@
* (at your option) any later version.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
+
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/kernel.h>
@@ -20,12 +22,9 @@
#include <linux/moduleparam.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
-#include <mach/ep93xx-regs.h>
-#include <mach/platform.h>
-#include <asm/io.h>
+#include <linux/io.h>
-#define DRV_MODULE_NAME "ep93xx-eth"
-#define DRV_MODULE_VERSION "0.1"
+#include <mach/hardware.h>
@@ -825,12 +816,19 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
struct ep93xx_eth_data *data;
struct net_device *dev;
struct ep93xx_priv *ep;
+ struct resource *mem;
+ struct resource *irq;
int err;
if (pdev == NULL)
return -ENODEV;
data = pdev->dev.platform_data;
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ irq = platform_get_irq(pdev, 0);
+ if (!mem || irq < 0)
+ return -ENXIO;
+
dev = ep93xx_dev_alloc(data);
if (dev == NULL) {
err = -ENOMEM;
@@ -842,23 +840,21 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dev);
- ep->res = request_mem_region(pdev->resource[0].start,
- pdev->resource[0].end - pdev->resource[0].start + 1,
- dev_name(&pdev->dev));
+ ep->res = request_mem_region(mem->start, resource_size(mem),
+ dev_name(&pdev->dev));
if (ep->res == NULL) {
dev_err(&pdev->dev, "Could not reserve memory region\n");
err = -ENOMEM;
goto err_out;
}
- ep->base_addr = ioremap(pdev->resource[0].start,
- pdev->resource[0].end - pdev->resource[0].start);
+ ep->base_addr = ioremap(mem->start, resource_size(mem));
if (ep->base_addr == NULL) {
dev_err(&pdev->dev, "Failed to ioremap ethernet registers\n");
err = -EIO;
goto err_out;
}
- ep->irq = pdev->resource[1].start;
+ ep->irq = irq->start;
ep->mii.phy_id = data->phy_id;
ep->mii.phy_id_mask = 0x1f;
@@ -877,11 +873,8 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
goto err_out;
}
- printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, "
- "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
- ep->irq, data->dev_addr[0], data->dev_addr[1],
- data->dev_addr[2], data->dev_addr[3],
- data->dev_addr[4], data->dev_addr[5]);
+ printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, %pM\n",
+ dev->name, ep->irq, dev->dev_addr);
return 0;
@@ -902,7 +895,6 @@ static struct platform_driver ep93xx_eth_driver = {
static int __init ep93xx_eth_init_module(void)
{
- printk(KERN_INFO DRV_MODULE_NAME " version " DRV_MODULE_VERSION " loading\n");
return platform_driver_register(&ep93xx_eth_driver);
H Hartley Sweeten
Added netdev to the Cc: list, and removed Nick M. (unintended)
> On Wed, 2009-12-16 at 12:09 -0500, H Hartley Sweeten wrote:
>> +#define pr_fmt(fmt) KBUILD_MODNAME ": %s: " fmt, __func__
>
> I think KBUILD_MODNAME ":%s: " fmt, __func__
> or its equivalent direct use is more common.
> It's possibly useful to standardize.
The biggest user of pr_fmt that I could find is in drivers/s390 and that
uses a local KMSG_COMPONENT define instead of KBUILD_MODNAME.
>> -#define DRV_MODULE_NAME "ep93xx-eth"
>> -#define DRV_MODULE_VERSION "0.1"
>> +#include <mach/hardware.h>
>
> I think these sorts of defines are common and useful.
and only use them in one place.
>> static int __init ep93xx_eth_init_module(void)
>> {
>> - printk(KERN_INFO DRV_MODULE_NAME " version " DRV_MODULE_VERSION " loading\n");
>
> Why remove this printk completely?
ep93xx-eth version 0.1 loading
eth0: ep93xx on-chip ethernet, IRQ 39, 00:21:f5:00:00:17
If the drive loads correctly you will get the second line. If it error's for
some reason you will get one of the dev_err messages.
But, if the defines go back in I can also add this message back.
Regards,
Hartley
Lennert Buytenhek
> >> General cleanup of the ep93xx_eth driver.
> >
> > Apart from keeping DRV_MODULE_NAME and DRV_MODULE_VERSION, I have
> > no strong opinion about this one way or the other. So I guess that
> > means ACK.
>
> I will add back the DRV_MODULE_NAME and DRV_MODULE_VERSION and repost.
>
> What about the message in ep93xx_eth_init_module()? Would you prefer
> that one to stay?
Most drivers I'm familiar with print a version message when they are
first instantiated -- perhaps that makes more sense.
H Hartley Sweeten
> On Wed, Dec 16, 2009 at 12:29:38PM -0500, H Hartley Sweeten wrote:
>
>> General cleanup of the ep93xx_eth driver.
>
> no strong opinion about this one way or the other. So I guess that
> means ACK.
I will add back the DRV_MODULE_NAME and DRV_MODULE_VERSION and repost.
What about the message in ep93xx_eth_init_module()? Would you prefer
that one to stay?
Regards,
Hartley
H Hartley Sweeten
1) Use pr_fmt() to prefix the module name and __func__ to the error
messages.
2) <linux/io.h> instead of <asm/io.h>
3) <mach/hardware.h> instead of <mach/ep93xx-regs.h> and <mach/platform.h>
4) Move the ep93xx_mdio_read (and ep93xx_mdio_write) function to eliminate
the function prototype.
5) Change all the printk(<level> messages to pr_<level> and remove the
__func__ argument.
6) Use platform_get_{resource/irq} to get the platform resources and add
an error check.
7) Use resource_size() for request_mem_region() and ioremap().
8) Use %pM to print the MAC address at the end of the probe.
9) Use dev->dev_addr not data->dev_addr for the MAC argument because a
random address could be used if the platform does not supply one.
The message at the end of the probe is left as a printk since it displays
cleaner without the function name that would be displayed with pr_info().
Signed-off-by: H Hartley Sweeten <hswe...@visionengravers.com>
Cc: Lennert Buytenhek <ker...@wantstofly.org>
Cc: David S. Miller <da...@davemloft.net>
Cc: Joe Perches <j...@perches.com>
---
V2 - Missed the resource check and resource_size()
V3 - Change pr_fmt() as suggested by Joe Perches
Don't remove DRV_MODULE_NAME and DRV_MODULE_VERSION
Don't remove the message in ep93xx_eth_init_module()
platform_get_irq returns an int not a resource *
diff --git a/drivers/net/arm/ep93xx_eth.c b/drivers/net/arm/ep93xx_eth.c
index b25467a..bf72d57 100644
--- a/drivers/net/arm/ep93xx_eth.c
+++ b/drivers/net/arm/ep93xx_eth.c
@@ -9,6 +9,8 @@
* (at your option) any later version.
*/
+#define pr_fmt(fmt) KBUILD_MODNAME ":%s: " fmt, __func__
+
#include <linux/dma-mapping.h>
#include <linux/module.h>
#include <linux/kernel.h>
@@ -20,9 +22,9 @@
#include <linux/moduleparam.h>
#include <linux/platform_device.h>
#include <linux/delay.h>
-#include <mach/ep93xx-regs.h>
-#include <mach/platform.h>
-#include <asm/io.h>
+#include <linux/io.h>
+
+#include <mach/hardware.h>
#define DRV_MODULE_NAME "ep93xx-eth"
#define DRV_MODULE_VERSION "0.1"
@@ -185,7 +187,47 @@ struct ep93xx_priv
@@ -217,14 +259,11 @@ static int ep93xx_rx(struct net_device *dev, int processed, int budget)
rstat->rstat1 = 0;
if (!(rstat0 & RSTAT0_EOF))
- printk(KERN_CRIT "ep93xx_rx: not end-of-frame "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_crit("not end-of-frame %.8x %.8x\n", rstat0, rstat1);
if (!(rstat0 & RSTAT0_EOB))
- printk(KERN_CRIT "ep93xx_rx: not end-of-buffer "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_crit("not end-of-buffer %.8x %.8x\n", rstat0, rstat1);
if ((rstat1 & RSTAT1_BUFFER_INDEX) >> 16 != entry)
- printk(KERN_CRIT "ep93xx_rx: entry mismatch "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_crit("entry mismatch %.8x %.8x\n", rstat0, rstat1);
if (!(rstat0 & RSTAT0_RWE)) {
ep->stats.rx_errors++;
@@ -241,8 +280,7 @@ static int ep93xx_rx(struct net_device *dev, int processed, int budget)
length = rstat1 & RSTAT1_FRAME_LENGTH;
if (length > MAX_PKT_SIZE) {
- printk(KERN_NOTICE "ep93xx_rx: invalid length "
- " %.8x %.8x\n", rstat0, rstat1);
+ pr_notice("invalid length %.8x %.8x\n", rstat0, rstat1);
goto err;
}
@@ -371,11 +409,9 @@ static void ep93xx_tx_complete(struct net_device *dev)
tstat->tstat0 = 0;
if (tstat0 & TSTAT0_FA)
- printk(KERN_CRIT "ep93xx_tx_complete: frame aborted "
- " %.8x\n", tstat0);
+ pr_crit("frame aborted %.8x\n", tstat0);
if ((tstat0 & TSTAT0_BUFFER_INDEX) != entry)
- printk(KERN_CRIT "ep93xx_tx_complete: entry mismatch "
- " %.8x\n", tstat0);
+ pr_crit("entry mismatch %.8x\n", tstat0);
if (tstat0 & TSTAT0_TXWE) {
int length = ep->descs->tdesc[entry].tdesc1 & 0xfff;
@@ -536,7 +572,7 @@ static int ep93xx_start_hw(struct net_device *dev)
}
if (i == 10) {
- printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
+ pr_crit("hw failed to reset\n");
return 1;
}
@@ -581,7 +617,7 @@ static int ep93xx_start_hw(struct net_device *dev)
}
if (i == 10) {
- printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to start\n");
+ pr_crit("hw failed to start\n");
return 1;
}
@@ -617,7 +653,7 @@ static void ep93xx_stop_hw(struct net_device *dev)
}
if (i == 10)
- printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
+ pr_crit("hw failed to reset\n");
}
static int ep93xx_open(struct net_device *dev)
@@ -681,48 +717,6 @@ static int ep93xx_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
strcpy(info->driver, DRV_MODULE_NAME);
@@ -825,12 +819,19 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
struct ep93xx_eth_data *data;
struct net_device *dev;
struct ep93xx_priv *ep;
+ struct resource *mem;
+ int irq;
int err;
if (pdev == NULL)
return -ENODEV;
data = pdev->dev.platform_data;
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ irq = platform_get_irq(pdev, 0);
+ if (!mem || irq < 0)
+ return -ENXIO;
+
dev = ep93xx_dev_alloc(data);
if (dev == NULL) {
err = -ENOMEM;
@@ -842,23 +843,21 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, dev);
- ep->res = request_mem_region(pdev->resource[0].start,
- pdev->resource[0].end - pdev->resource[0].start + 1,
- dev_name(&pdev->dev));
+ ep->res = request_mem_region(mem->start, resource_size(mem),
+ dev_name(&pdev->dev));
if (ep->res == NULL) {
dev_err(&pdev->dev, "Could not reserve memory region\n");
err = -ENOMEM;
goto err_out;
}
- ep->base_addr = ioremap(pdev->resource[0].start,
- pdev->resource[0].end - pdev->resource[0].start);
+ ep->base_addr = ioremap(mem->start, resource_size(mem));
if (ep->base_addr == NULL) {
dev_err(&pdev->dev, "Failed to ioremap ethernet registers\n");
err = -EIO;
goto err_out;
}
- ep->irq = pdev->resource[1].start;
+ ep->irq = irq;
ep->mii.phy_id = data->phy_id;
ep->mii.phy_id_mask = 0x1f;
@@ -877,11 +876,8 @@ static int ep93xx_eth_probe(struct platform_device *pdev)
goto err_out;
}
- printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, "
- "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
- ep->irq, data->dev_addr[0], data->dev_addr[1],
- data->dev_addr[2], data->dev_addr[3],
- data->dev_addr[4], data->dev_addr[5]);
+ printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, %pM\n",
+ dev->name, ep->irq, dev->dev_addr);
return 0;
Lennert Buytenhek
> V3 - Change pr_fmt() as suggested by Joe Perches
> Don't remove DRV_MODULE_NAME and DRV_MODULE_VERSION
> Don't remove the message in ep93xx_eth_init_module()
That's not what I meant, but alright, as you prefer.
H Hartley Sweeten
> On Wed, Dec 16, 2009 at 01:18:13PM -0500, H Hartley Sweeten wrote:
>
>> V3 - Change pr_fmt() as suggested by Joe Perches
>> Don't remove DRV_MODULE_NAME and DRV_MODULE_VERSION
>> Don't remove the message in ep93xx_eth_init_module()
>
> That's not what I meant, but alright, as you prefer.
Guess my parser is off line today.... ;-)
What did you mean?
Regards,
Hartley
Lennert Buytenhek
> General cleanup of the ep93xx_eth driver.
Apart from keeping DRV_MODULE_NAME and DRV_MODULE_VERSION, I have
no strong opinion about this one way or the other. So I guess that
means ACK.
David Miller
Date: Wed, 16 Dec 2009 13:34:55 -0500
> Wednesday, December 16, 2009 11:32 AM, Lennert Buytenhek wrote:
>> On Wed, Dec 16, 2009 at 01:18:13PM -0500, H Hartley Sweeten wrote:
>>
>>> V3 - Change pr_fmt() as suggested by Joe Perches
>>> Don't remove DRV_MODULE_NAME and DRV_MODULE_VERSION
>>> Don't remove the message in ep93xx_eth_init_module()
>>
>> That's not what I meant, but alright, as you prefer.
>
> Guess my parser is off line today.... ;-)
>
> What did you mean?
This patch also doesn't apply to current sources, so if you
could also please respin this once you've resolved the feedback
that would be great.
Thanks.
H Hartley Sweeten
> From: "H Hartley Sweeten" <hart...@visionengravers.com>
> Date: Wed, 16 Dec 2009 13:34:55 -0500
>
>> Wednesday, December 16, 2009 11:32 AM, Lennert Buytenhek wrote:
>>> On Wed, Dec 16, 2009 at 01:18:13PM -0500, H Hartley Sweeten wrote:
>>>
>>>> V3 - Change pr_fmt() as suggested by Joe Perches
>>>> Don't remove DRV_MODULE_NAME and DRV_MODULE_VERSION
>>>> Don't remove the message in ep93xx_eth_init_module()
>>>
>>> That's not what I meant, but alright, as you prefer.
>>
>> Guess my parser is off line today.... ;-)
>>
>> What did you mean?
>
> This patch also doesn't apply to current sources, so if you
> could also please respin this once you've resolved the feedback
> that would be great.
Lennert,
Since I need to respin this patch what did you mean by this comment?
On Wednesday, December 16, 2009 11:09 AM, Lennert Buytenhek wrote:
> On Wed, Dec 16, 2009 at 01:06:10PM -0500, H Hartley Sweeten wrote:
>
>>>> General cleanup of the ep93xx_eth driver.
>>>
>>> Apart from keeping DRV_MODULE_NAME and DRV_MODULE_VERSION, I have
>>> no strong opinion about this one way or the other. So I guess that
>>> means ACK.
>>
>> I will add back the DRV_MODULE_NAME and DRV_MODULE_VERSION and repost.
>>
>> What about the message in ep93xx_eth_init_module()? Would you prefer
>> that one to stay?
>
> Most drivers I'm familiar with print a version message when they are
> first instantiated -- perhaps that makes more sense.
I will wait for your reply before updating the patch.
Thanks,
Hartley
Lennert Buytenhek
> >> Wednesday, December 16, 2009 11:32 AM, Lennert Buytenhek wrote:
> >>> On Wed, Dec 16, 2009 at 01:18:13PM -0500, H Hartley Sweeten wrote:
> >>>
> >>>> V3 - Change pr_fmt() as suggested by Joe Perches
> >>>> Don't remove DRV_MODULE_NAME and DRV_MODULE_VERSION
> >>>> Don't remove the message in ep93xx_eth_init_module()
> >>>
> >>> That's not what I meant, but alright, as you prefer.
> >>
> >> Guess my parser is off line today.... ;-)
> >>
> >> What did you mean?
> >
> > This patch also doesn't apply to current sources, so if you
> > could also please respin this once you've resolved the feedback
> > that would be great.
>
> Lennert,
Hi Hartley,
> Since I need to respin this patch what did you mean by this comment?
>
> On Wednesday, December 16, 2009 11:09 AM, Lennert Buytenhek wrote:
> > On Wed, Dec 16, 2009 at 01:06:10PM -0500, H Hartley Sweeten wrote:
> >
> >>>> General cleanup of the ep93xx_eth driver.
> >>>
> >>> Apart from keeping DRV_MODULE_NAME and DRV_MODULE_VERSION, I have
> >>> no strong opinion about this one way or the other. So I guess that
> >>> means ACK.
> >>
> >> I will add back the DRV_MODULE_NAME and DRV_MODULE_VERSION and repost.
> >>
> >> What about the message in ep93xx_eth_init_module()? Would you prefer
> >> that one to stay?
> >
> > Most drivers I'm familiar with print a version message when they are
> > first instantiated -- perhaps that makes more sense.
>
> I will wait for your reply before updating the patch.
What I meant was, most drivers seem to print a message at probe
time, i.e. when they are attached to an actual device for the first
time.
Grepping for 'printed_version' in drivers/net/* actually turns up
two variants:
- Print the version message on the first probe. (e.g. 3c59x)
- Print the version message at module load time if we were built as
a module, while if we were built into the kernel, only print a
version message on the first probe. (e.g. 8139too)
At least in the case of ep93xx_eth, you can't even have it enabled if
you're not building a kernel specifically for the ep93xx ARM SoC, and
if you are building for the ep93xx, as far as I know we don't support
any boards that don't have the ethernet brought out and so you're very
likely to have ep93xx_eth enabled then, so whether you do it
unconditionally at driver init time or at probe time will make no
effective difference for the majority of cases.
I.e. I can't really say I care much either way.
cheers,
Lennert