Message ID | 75b669c0a947effe74b291093abfa8c71f83736a.1392220536.git.michal.simek@xilinx.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2014-02-12 at 16:55 +0100, Michal Simek wrote: > From: Srikanth Thokala <srikanth.thokala@xilinx.com> trivia: > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > + netdev_err(lp->ndev, > + "axienet_device_reset DMA reset timeout!\n"); could you please align multi-line arguments to the appropriate open parenthesis? netdev_err(lp->ndev, "axienet_device_reset DMA reset timeout!\n"); or maybe: netdev_err(lp->ndev, "%s: "DMA reset timeout!\n", __func__); > @@ -484,8 +484,8 @@ static void axienet_device_reset(struct net_device *ndev) > } > > if (axienet_dma_bd_init(ndev)) { > - dev_err(&ndev->dev, "axienet_device_reset descriptor " > - "allocation failed\n"); > + netdev_err(ndev, > + "axienet_device_reset descriptor allocation failed\n"); etc, et al. > diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c [] > @@ -161,19 +161,19 @@ int axienet_mdio_setup(struct axienet_local *lp, struct device_node *np) > > np1 = of_find_node_by_name(NULL, "cpu"); > if (!np1) { > - printk(KERN_WARNING "%s(): Could not find CPU device node.", > - __func__); > - printk(KERN_WARNING "Setting MDIO clock divisor to " > - "default %d\n", DEFAULT_CLOCK_DIVISOR); > + netdev_warn(lp->ndev, "Could not find CPU device node."); missing trailing "\n" to terminate message. > + netdev_warn(lp->ndev, > + "Could not find clock ethernet controller property."); here too. (and alignment) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joe, On 02/13/2014 01:31 AM, Joe Perches wrote: > On Wed, 2014-02-12 at 16:55 +0100, Michal Simek wrote: >> From: Srikanth Thokala <srikanth.thokala@xilinx.com> > > trivia: > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c > >> + netdev_err(lp->ndev, >> + "axienet_device_reset DMA reset timeout!\n"); > > could you please align multi-line arguments to the > appropriate open parenthesis? > > netdev_err(lp->ndev, > "axienet_device_reset DMA reset timeout!\n"); > > or maybe: > > netdev_err(lp->ndev, "%s: "DMA reset timeout!\n", > __func__); ok. > >> @@ -484,8 +484,8 @@ static void axienet_device_reset(struct net_device *ndev) >> } >> >> if (axienet_dma_bd_init(ndev)) { >> - dev_err(&ndev->dev, "axienet_device_reset descriptor " >> - "allocation failed\n"); >> + netdev_err(ndev, >> + "axienet_device_reset descriptor allocation failed\n"); > > etc, et al. ok. > >> diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c > [] >> @@ -161,19 +161,19 @@ int axienet_mdio_setup(struct axienet_local *lp, struct device_node *np) >> >> np1 = of_find_node_by_name(NULL, "cpu"); >> if (!np1) { >> - printk(KERN_WARNING "%s(): Could not find CPU device node.", >> - __func__); >> - printk(KERN_WARNING "Setting MDIO clock divisor to " >> - "default %d\n", DEFAULT_CLOCK_DIVISOR); >> + netdev_warn(lp->ndev, "Could not find CPU device node."); > > missing trailing "\n" to terminate message. ok. > >> + netdev_warn(lp->ndev, >> + "Could not find clock ethernet controller property."); > > here too. (and alignment) This is problematic. I would like to keep 80 char limits and keeping this align just break it. That's why I was using tab alignment. Probably the solution is just to shorten message. Thanks for your comments, Michal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2014-02-13 at 08:19 +0100, Michal Simek wrote: > On 02/13/2014 01:31 AM, Joe Perches wrote: > > On Wed, 2014-02-12 at 16:55 +0100, Michal Simek wrote: Hi again Michal. > >> + netdev_warn(lp->ndev, > >> + "Could not find clock ethernet controller property."); > > > > here too. (and alignment) > > This is problematic. I would like to keep 80 char limits and keeping > this align just break it. That's why I was using tab alignment. > Probably the solution is just to shorten message. (overly long, tiresomely trivial stuff below) Your choice. I'm not an 80 column zealot but please don't shorten the message just to fit 80 columns if it impacts intelligibility. Generally, I'd write this something like: netdev_warn(lp->ndev, "Could not find clock ethernet controller property\n"); (without the period) which is 83 columns. checkpatch makes exceptions for 80 column line length maximums for format strings. I've no real issue if you indent it back one. fyi: this is 77 columns netdev_warn(lp->ndev, "No clock ethernet controller property found\n"); About the message itself. You dropped the "axienet_mdio_setup" function name. I believe the dmesg output will look something like: xilinx_temac 0000:01:00.0 (unregistered net_device): Could not find clock ethernet controller property. xilinx_temac 0000:01:00.0 (unregistered net_device): Setting MDIO clock divisor to default 29 Because these 2 messages are effectively linked, my preference would be to emit them on a single line, Something like: xilinx_temac 0000:01:00.0 (unregistered net_device): of_get_property("clock-frequency") not found - setting MDIO clock divisor to default 29 or netdev_warn(lp->ndev, "of_get_property(\"clock-frequency\") not found - setting MDIO clock divisor to default %u\n", DEFAULT_CLOCK_DIVISOR); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/13/2014 04:51 PM, Joe Perches wrote: > On Thu, 2014-02-13 at 08:19 +0100, Michal Simek wrote: >> On 02/13/2014 01:31 AM, Joe Perches wrote: >>> On Wed, 2014-02-12 at 16:55 +0100, Michal Simek wrote: > > Hi again Michal. > >>>> + netdev_warn(lp->ndev, >>>> + "Could not find clock ethernet controller property."); >>> >>> here too. (and alignment) >> >> This is problematic. I would like to keep 80 char limits and keeping >> this align just break it. That's why I was using tab alignment. >> Probably the solution is just to shorten message. > > (overly long, tiresomely trivial stuff below) > > Your choice. I'm not an 80 column zealot but > please don't shorten the message just to fit > 80 columns if it impacts intelligibility. I am trying to keep 80 chars and follow subsystem standards. > Generally, I'd write this something like: > > netdev_warn(lp->ndev, > "Could not find clock ethernet controller property\n"); > > (without the period) which is 83 columns. ok. > checkpatch makes exceptions for 80 column line > length maximums for format strings. yes but testing systems reports it because that 80 chars is still default value. > > I've no real issue if you indent it back one. > > fyi: this is 77 columns > > netdev_warn(lp->ndev, > "No clock ethernet controller property found\n"); > > About the message itself. > > You dropped the "axienet_mdio_setup" function name. > > I believe the dmesg output will look something like: > > xilinx_temac 0000:01:00.0 (unregistered net_device): Could not find clock ethernet controller property. > xilinx_temac 0000:01:00.0 (unregistered net_device): Setting MDIO clock divisor to default 29 > > Because these 2 messages are effectively linked, > my preference would be to emit them on a single line, > > Something like: > > xilinx_temac 0000:01:00.0 (unregistered net_device): of_get_property("clock-frequency") not found - setting MDIO clock divisor to default 29 > > or > > netdev_warn(lp->ndev, > "of_get_property(\"clock-frequency\") not found - setting MDIO clock divisor to default %u\n", > DEFAULT_CLOCK_DIVISOR); > But then you are breaking 80 char limits a lot. Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 9cc9e59..6059a0f 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -444,8 +444,8 @@ static void __axienet_device_reset(struct axienet_local *lp, while (axienet_dma_in32(lp, offset) & XAXIDMA_CR_RESET_MASK) { udelay(1); if (--timeout == 0) { - dev_err(dev, "axienet_device_reset DMA " - "reset timeout!\n"); + netdev_err(lp->ndev, + "axienet_device_reset DMA reset timeout!\n"); break; } } @@ -484,8 +484,8 @@ static void axienet_device_reset(struct net_device *ndev) } if (axienet_dma_bd_init(ndev)) { - dev_err(&ndev->dev, "axienet_device_reset descriptor " - "allocation failed\n"); + netdev_err(ndev, + "axienet_device_reset descriptor allocation failed\n"); } axienet_status = axienet_ior(lp, XAE_RCW1_OFFSET); @@ -560,8 +560,8 @@ static void axienet_adjust_link(struct net_device *ndev) lp->last_link = link_state; phy_print_status(phy); } else { - dev_err(&ndev->dev, "Error setting Axi Ethernet " - "mac speed\n"); + netdev_err(ndev, + "Error setting Axi Ethernet mac speed\n"); } } } @@ -1238,8 +1238,8 @@ axienet_ethtools_set_pauseparam(struct net_device *ndev, struct axienet_local *lp = netdev_priv(ndev); if (netif_running(ndev)) { - printk(KERN_ERR "%s: Please stop netif before applying " - "configruation\n", ndev->name); + netdev_err(ndev, + "Please stop netif before applying configuration\n"); return -EFAULT; } @@ -1295,8 +1295,8 @@ static int axienet_ethtools_set_coalesce(struct net_device *ndev, struct axienet_local *lp = netdev_priv(ndev); if (netif_running(ndev)) { - printk(KERN_ERR "%s: Please stop netif before applying " - "configruation\n", ndev->name); + netdev_err(ndev, + "Please stop netif before applying configuration\n"); return -EFAULT; } diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c index 64b4639..ef0a20c 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_mdio.c @@ -161,19 +161,19 @@ int axienet_mdio_setup(struct axienet_local *lp, struct device_node *np) np1 = of_find_node_by_name(NULL, "cpu"); if (!np1) { - printk(KERN_WARNING "%s(): Could not find CPU device node.", - __func__); - printk(KERN_WARNING "Setting MDIO clock divisor to " - "default %d\n", DEFAULT_CLOCK_DIVISOR); + netdev_warn(lp->ndev, "Could not find CPU device node."); + netdev_warn(lp->ndev, + "Setting MDIO clock divisor to default %d\n", + DEFAULT_CLOCK_DIVISOR); clk_div = DEFAULT_CLOCK_DIVISOR; goto issue; } property_p = (u32 *) of_get_property(np1, "clock-frequency", NULL); if (!property_p) { - printk(KERN_WARNING "%s(): Could not find CPU property: " - "clock-frequency.", __func__); - printk(KERN_WARNING "Setting MDIO clock divisor to " - "default %d\n", DEFAULT_CLOCK_DIVISOR); + netdev_warn(lp->ndev, + "Could not find clock ethernet controller property."); + netdev_warn(lp->ndev, "Setting MDIO clock divisor to default %d\n", + DEFAULT_CLOCK_DIVISOR); clk_div = DEFAULT_CLOCK_DIVISOR; of_node_put(np1); goto issue; @@ -187,8 +187,9 @@ int axienet_mdio_setup(struct axienet_local *lp, struct device_node *np) if (host_clock % (MAX_MDIO_FREQ * 2)) clk_div++; - printk(KERN_DEBUG "%s(): Setting MDIO clock divisor to %u based " - "on %u Hz host clock.\n", __func__, clk_div, host_clock); + netdev_dbg(lp->ndev, + "Setting MDIO clock divisor to %u based on %u Hz host clock.\n", + clk_div, host_clock); of_node_put(np1); issue: