Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1351755 > unrolled thread

[PATCH 0/3] net: macb: Fix coding style issues

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] net: macb: Fix coding style issues Moritz Fischer <moritz.fischer@ettus.com> - 2016-03-07 17:20 +0100
    [PATCH 3/3] net: macb: Cleanup checkpatch checks Moritz Fischer <moritz.fischer@ettus.com> - 2016-03-07 17:20 +0100
      Re: [PATCH 3/3] net: macb: Cleanup checkpatch checks Moritz Fischer <moritz.fischer@ettus.com> - 2016-03-07 17:30 +0100
    [PATCH 1/3] net: macb: Fix coding style error message Moritz Fischer <moritz.fischer@ettus.com> - 2016-03-07 17:20 +0100
    [PATCH 3/3] net: macb: Address checkpatch 'check' suggestions Moritz Fischer <moritz.fischer@ettus.com> - 2016-03-07 17:30 +0100
    Re: [PATCH 0/3] net: macb: Fix coding style issues Nicolas Ferre <nicolas.ferre@atmel.com> - 2016-03-07 18:20 +0100
      Re: [PATCH 0/3] net: macb: Fix coding style issues Michal Simek <michal.simek@xilinx.com> - 2016-03-09 17:40 +0100
        Re: [PATCH 0/3] net: macb: Fix coding style issues Moritz Fischer <moritz.fischer@ettus.com> - 2016-03-09 18:20 +0100
        Re: [PATCH 0/3] net: macb: Fix coding style issues Michal Simek <michal.simek@xilinx.com> - 2016-03-09 18:30 +0100
          Re: [PATCH 0/3] net: macb: Fix coding style issues David Miller <davem@davemloft.net> - 2016-03-09 21:30 +0100
        Re: [PATCH 0/3] net: macb: Fix coding style issues David Miller <davem@davemloft.net> - 2016-03-09 18:30 +0100
    Re: [PATCH 0/3] net: macb: Fix coding style issues Alexander Stein <alexander.stein@systec-electronic.com> - 2016-03-08 08:10 +0100

#1351755 — [PATCH 0/3] net: macb: Fix coding style issues

Hi Nicolas,

this series deals with most of the checkpatch warnings
generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
that were suggested by checkpatch, that I can address in a follow up
commit if needed.
Let me know if you want me to split the fixes differently or squash
them into one commit.

Cheers,

Moritz


Moritz Fischer (3):
  net: macb: Fix coding style error message
  net: macb: Fix more coding style issues
  net: macb: Address checkpatch 'check' suggestions

 drivers/net/ethernet/cadence/macb.c | 157 ++++++++++++++++--------------------
 1 file changed, 71 insertions(+), 86 deletions(-)

-- 
2.4.3

[toc] | [next] | [standalone]

#1351756 — [PATCH 3/3] net: macb: Cleanup checkpatch checks

This commit deals with a bunch of check suggestions
that without changing behavior make checkpatch hapy.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 drivers/net/ethernet/cadence/macb.c | 46 +++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index fba4239..75c19a2 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -184,7 +184,7 @@ static void macb_get_hwaddr(struct macb *bp)
 
 	pdata = dev_get_platdata(&bp->pdev->dev);
 
-	/* Check all 4 address register for vaild address */
+	/* Check all 4 address register for valid address */
 	for (i = 0; i < 4; i++) {
 		bottom = macb_or_gem_readl(bp, SA1B + i * 8);
 		top = macb_or_gem_readl(bp, SA1T + i * 8);
@@ -292,7 +292,7 @@ static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
 	ferr = DIV_ROUND_UP(ferr, rate / 100000);
 	if (ferr > 5)
 		netdev_warn(dev, "unable to generate target frequency: %ld Hz\n",
-				rate);
+			    rate);
 
 	if (clk_set_rate(clk, rate_rounded))
 		netdev_err(dev, "adjusting tx_clk failed.\n");
@@ -426,7 +426,7 @@ static int macb_mii_init(struct macb *bp)
 	macb_writel(bp, NCR, MACB_BIT(MPE));
 
 	bp->mii_bus = mdiobus_alloc();
-	if (bp->mii_bus == NULL) {
+	if (!bp->mii_bus) {
 		err = -ENOMEM;
 		goto err_out;
 	}
@@ -435,7 +435,7 @@ static int macb_mii_init(struct macb *bp)
 	bp->mii_bus->read = &macb_mdio_read;
 	bp->mii_bus->write = &macb_mdio_write;
 	snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
-		bp->pdev->name, bp->pdev->id);
+		 bp->pdev->name, bp->pdev->id);
 	bp->mii_bus->priv = bp;
 	bp->mii_bus->parent = &bp->dev->dev;
 	pdata = dev_get_platdata(&bp->pdev->dev);
@@ -656,7 +656,7 @@ static void macb_tx_interrupt(struct macb_queue *queue)
 		queue_writel(queue, ISR, MACB_BIT(TCOMP));
 
 	netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
-		(unsigned long)status);
+		    (unsigned long)status);
 
 	head = queue->tx_head;
 	for (tail = queue->tx_tail; tail != head; tail++) {
@@ -725,10 +725,10 @@ static void gem_rx_refill(struct macb *bp)
 
 		bp->rx_prepared_head++;
 
-		if (bp->rx_skbuff[entry] == NULL) {
+		if (!bp->rx_skbuff[entry]) {
 			/* allocate sk_buff for this free entry in ring */
 			skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
-			if (unlikely(skb == NULL)) {
+			if (unlikely(!skb)) {
 				netdev_err(bp->dev,
 					   "Unable to allocate sk_buff\n");
 				break;
@@ -762,7 +762,7 @@ static void gem_rx_refill(struct macb *bp)
 	wmb();
 
 	netdev_vdbg(bp->dev, "rx ring: prepared head %d, tail %d\n",
-		   bp->rx_prepared_head, bp->rx_tail);
+		    bp->rx_prepared_head, bp->rx_tail);
 }
 
 /* Mark DMA descriptors from begin up to and not including end as unused */
@@ -876,8 +876,8 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	len = desc->ctrl & bp->rx_frm_len_mask;
 
 	netdev_vdbg(bp->dev, "macb_rx_frame frags %u - %u (len %u)\n",
-		macb_rx_ring_wrap(first_frag),
-		macb_rx_ring_wrap(last_frag), len);
+		    macb_rx_ring_wrap(first_frag),
+		    macb_rx_ring_wrap(last_frag), len);
 
 	/* The ethernet header starts NET_IP_ALIGN bytes into the
 	 * first buffer. Since the header is 14 bytes, this makes the
@@ -916,7 +916,8 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 			frag_len = len - offset;
 		}
 		skb_copy_to_linear_data_offset(skb, offset,
-				macb_rx_buffer(bp, frag), frag_len);
+					       macb_rx_buffer(bp, frag),
+					       frag_len);
 		offset += bp->rx_buffer_size;
 		desc = macb_rx_desc(bp, frag);
 		desc->addr &= ~MACB_BIT(RX_USED);
@@ -934,7 +935,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	bp->stats.rx_packets++;
 	bp->stats.rx_bytes += skb->len;
 	netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
-		   skb->len, skb->csum);
+		    skb->len, skb->csum);
 	netif_receive_skb(skb);
 
 	return 0;
@@ -998,7 +999,7 @@ static int macb_poll(struct napi_struct *napi, int budget)
 	work_done = 0;
 
 	netdev_vdbg(bp->dev, "poll: status = %08lx, budget = %d\n",
-		   (unsigned long)status, budget);
+		    (unsigned long)status, budget);
 
 	work_done = bp->macbgem_ops.mog_rx(bp, budget);
 	if (work_done < budget) {
@@ -1213,7 +1214,7 @@ static unsigned int macb_tx_map(struct macb *bp,
 	}
 
 	/* Should never happen */
-	if (unlikely(tx_skb == NULL)) {
+	if (unlikely(!tx_skb)) {
 		netdev_err(bp->dev, "BUG! empty skb!\n");
 		return 0;
 	}
@@ -1283,16 +1284,16 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 #if defined(DEBUG) && defined(VERBOSE_DEBUG)
 	netdev_vdbg(bp->dev,
-		   "start_xmit: queue %hu len %u head %p data %p tail %p end %p\n",
-		   queue_index, skb->len, skb->head, skb->data,
-		   skb_tail_pointer(skb), skb_end_pointer(skb));
+		    "start_xmit: queue %hu len %u head %p data %p tail %p end %p\n",
+		    queue_index, skb->len, skb->head, skb->data,
+		    skb_tail_pointer(skb), skb_end_pointer(skb));
 	print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_OFFSET, 16, 1,
 		       skb->data, 16, true);
 #endif
 
 	/* Count how many TX buffer descriptors are needed to send this
 	 * socket buffer: skb fragments of jumbo frames may need to be
-	 * splitted into many buffer descriptors.
+	 * split into many buffer descriptors.
 	 */
 	count = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
 	nr_frags = skb_shinfo(skb)->nr_frags;
@@ -1343,8 +1344,8 @@ static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
 
 		if (bp->rx_buffer_size % RX_BUFFER_MULTIPLE) {
 			netdev_dbg(bp->dev,
-				    "RX buffer must be multiple of %d bytes, expanding\n",
-				    RX_BUFFER_MULTIPLE);
+				   "RX buffer must be multiple of %d bytes, expanding\n",
+				   RX_BUFFER_MULTIPLE);
 			bp->rx_buffer_size =
 				roundup(bp->rx_buffer_size, RX_BUFFER_MULTIPLE);
 		}
@@ -1367,7 +1368,7 @@ static void gem_free_rx_buffers(struct macb *bp)
 	for (i = 0; i < RX_RING_SIZE; i++) {
 		skb = bp->rx_skbuff[i];
 
-		if (skb == NULL)
+		if (!skb)
 			continue;
 
 		desc = &bp->rx_ring[i];
@@ -1775,7 +1776,8 @@ static void macb_sethashtable(struct net_device *dev)
 	unsigned int bitnr;
 	struct macb *bp = netdev_priv(dev);
 
-	mc_filter[0] = mc_filter[1] = 0;
+	mc_filter[0] = 0;
+	mc_filter[1] = 0;
 
 	netdev_for_each_mc_addr(ha, dev) {
 		bitnr = hash_get_index(ha->addr);
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1351777 — Re: [PATCH 3/3] net: macb: Cleanup checkpatch checks

Derp, it's monday morning. Ignore the second [3/3] patch... Sorry for the noise.

Moritz

[toc] | [prev] | [next] | [standalone]

#1351758 — [PATCH 1/3] net: macb: Fix coding style error message

checkpatch.pl gave the following error:

ERROR: space required before the open parenthesis '('
+	for(; p < end; p++, offset += 4)

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 drivers/net/ethernet/cadence/macb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 50c9410..4370f37 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -496,7 +496,7 @@ static void macb_update_stats(struct macb *bp)
 
 	WARN_ON((unsigned long)(end - p - 1) != (MACB_TPF - MACB_PFR) / 4);
 
-	for(; p < end; p++, offset += 4)
+	for (; p < end; p++, offset += 4)
 		*p += bp->macb_reg_readl(bp, offset);
 }
 
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1351760 — [PATCH 3/3] net: macb: Address checkpatch 'check' suggestions

This commit deals with a bunch of checkpatch suggestions
that without changing behavior make checkpatch happier.

Signed-off-by: Moritz Fischer <moritz.fischer@ettus.com>
---
 drivers/net/ethernet/cadence/macb.c | 46 +++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index fba4239..75c19a2 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -184,7 +184,7 @@ static void macb_get_hwaddr(struct macb *bp)
 
 	pdata = dev_get_platdata(&bp->pdev->dev);
 
-	/* Check all 4 address register for vaild address */
+	/* Check all 4 address register for valid address */
 	for (i = 0; i < 4; i++) {
 		bottom = macb_or_gem_readl(bp, SA1B + i * 8);
 		top = macb_or_gem_readl(bp, SA1T + i * 8);
@@ -292,7 +292,7 @@ static void macb_set_tx_clk(struct clk *clk, int speed, struct net_device *dev)
 	ferr = DIV_ROUND_UP(ferr, rate / 100000);
 	if (ferr > 5)
 		netdev_warn(dev, "unable to generate target frequency: %ld Hz\n",
-				rate);
+			    rate);
 
 	if (clk_set_rate(clk, rate_rounded))
 		netdev_err(dev, "adjusting tx_clk failed.\n");
@@ -426,7 +426,7 @@ static int macb_mii_init(struct macb *bp)
 	macb_writel(bp, NCR, MACB_BIT(MPE));
 
 	bp->mii_bus = mdiobus_alloc();
-	if (bp->mii_bus == NULL) {
+	if (!bp->mii_bus) {
 		err = -ENOMEM;
 		goto err_out;
 	}
@@ -435,7 +435,7 @@ static int macb_mii_init(struct macb *bp)
 	bp->mii_bus->read = &macb_mdio_read;
 	bp->mii_bus->write = &macb_mdio_write;
 	snprintf(bp->mii_bus->id, MII_BUS_ID_SIZE, "%s-%x",
-		bp->pdev->name, bp->pdev->id);
+		 bp->pdev->name, bp->pdev->id);
 	bp->mii_bus->priv = bp;
 	bp->mii_bus->parent = &bp->dev->dev;
 	pdata = dev_get_platdata(&bp->pdev->dev);
@@ -656,7 +656,7 @@ static void macb_tx_interrupt(struct macb_queue *queue)
 		queue_writel(queue, ISR, MACB_BIT(TCOMP));
 
 	netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
-		(unsigned long)status);
+		    (unsigned long)status);
 
 	head = queue->tx_head;
 	for (tail = queue->tx_tail; tail != head; tail++) {
@@ -725,10 +725,10 @@ static void gem_rx_refill(struct macb *bp)
 
 		bp->rx_prepared_head++;
 
-		if (bp->rx_skbuff[entry] == NULL) {
+		if (!bp->rx_skbuff[entry]) {
 			/* allocate sk_buff for this free entry in ring */
 			skb = netdev_alloc_skb(bp->dev, bp->rx_buffer_size);
-			if (unlikely(skb == NULL)) {
+			if (unlikely(!skb)) {
 				netdev_err(bp->dev,
 					   "Unable to allocate sk_buff\n");
 				break;
@@ -762,7 +762,7 @@ static void gem_rx_refill(struct macb *bp)
 	wmb();
 
 	netdev_vdbg(bp->dev, "rx ring: prepared head %d, tail %d\n",
-		   bp->rx_prepared_head, bp->rx_tail);
+		    bp->rx_prepared_head, bp->rx_tail);
 }
 
 /* Mark DMA descriptors from begin up to and not including end as unused */
@@ -876,8 +876,8 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	len = desc->ctrl & bp->rx_frm_len_mask;
 
 	netdev_vdbg(bp->dev, "macb_rx_frame frags %u - %u (len %u)\n",
-		macb_rx_ring_wrap(first_frag),
-		macb_rx_ring_wrap(last_frag), len);
+		    macb_rx_ring_wrap(first_frag),
+		    macb_rx_ring_wrap(last_frag), len);
 
 	/* The ethernet header starts NET_IP_ALIGN bytes into the
 	 * first buffer. Since the header is 14 bytes, this makes the
@@ -916,7 +916,8 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 			frag_len = len - offset;
 		}
 		skb_copy_to_linear_data_offset(skb, offset,
-				macb_rx_buffer(bp, frag), frag_len);
+					       macb_rx_buffer(bp, frag),
+					       frag_len);
 		offset += bp->rx_buffer_size;
 		desc = macb_rx_desc(bp, frag);
 		desc->addr &= ~MACB_BIT(RX_USED);
@@ -934,7 +935,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	bp->stats.rx_packets++;
 	bp->stats.rx_bytes += skb->len;
 	netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
-		   skb->len, skb->csum);
+		    skb->len, skb->csum);
 	netif_receive_skb(skb);
 
 	return 0;
@@ -998,7 +999,7 @@ static int macb_poll(struct napi_struct *napi, int budget)
 	work_done = 0;
 
 	netdev_vdbg(bp->dev, "poll: status = %08lx, budget = %d\n",
-		   (unsigned long)status, budget);
+		    (unsigned long)status, budget);
 
 	work_done = bp->macbgem_ops.mog_rx(bp, budget);
 	if (work_done < budget) {
@@ -1213,7 +1214,7 @@ static unsigned int macb_tx_map(struct macb *bp,
 	}
 
 	/* Should never happen */
-	if (unlikely(tx_skb == NULL)) {
+	if (unlikely(!tx_skb)) {
 		netdev_err(bp->dev, "BUG! empty skb!\n");
 		return 0;
 	}
@@ -1283,16 +1284,16 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 #if defined(DEBUG) && defined(VERBOSE_DEBUG)
 	netdev_vdbg(bp->dev,
-		   "start_xmit: queue %hu len %u head %p data %p tail %p end %p\n",
-		   queue_index, skb->len, skb->head, skb->data,
-		   skb_tail_pointer(skb), skb_end_pointer(skb));
+		    "start_xmit: queue %hu len %u head %p data %p tail %p end %p\n",
+		    queue_index, skb->len, skb->head, skb->data,
+		    skb_tail_pointer(skb), skb_end_pointer(skb));
 	print_hex_dump(KERN_DEBUG, "data: ", DUMP_PREFIX_OFFSET, 16, 1,
 		       skb->data, 16, true);
 #endif
 
 	/* Count how many TX buffer descriptors are needed to send this
 	 * socket buffer: skb fragments of jumbo frames may need to be
-	 * splitted into many buffer descriptors.
+	 * split into many buffer descriptors.
 	 */
 	count = DIV_ROUND_UP(skb_headlen(skb), bp->max_tx_length);
 	nr_frags = skb_shinfo(skb)->nr_frags;
@@ -1343,8 +1344,8 @@ static void macb_init_rx_buffer_size(struct macb *bp, size_t size)
 
 		if (bp->rx_buffer_size % RX_BUFFER_MULTIPLE) {
 			netdev_dbg(bp->dev,
-				    "RX buffer must be multiple of %d bytes, expanding\n",
-				    RX_BUFFER_MULTIPLE);
+				   "RX buffer must be multiple of %d bytes, expanding\n",
+				   RX_BUFFER_MULTIPLE);
 			bp->rx_buffer_size =
 				roundup(bp->rx_buffer_size, RX_BUFFER_MULTIPLE);
 		}
@@ -1367,7 +1368,7 @@ static void gem_free_rx_buffers(struct macb *bp)
 	for (i = 0; i < RX_RING_SIZE; i++) {
 		skb = bp->rx_skbuff[i];
 
-		if (skb == NULL)
+		if (!skb)
 			continue;
 
 		desc = &bp->rx_ring[i];
@@ -1775,7 +1776,8 @@ static void macb_sethashtable(struct net_device *dev)
 	unsigned int bitnr;
 	struct macb *bp = netdev_priv(dev);
 
-	mc_filter[0] = mc_filter[1] = 0;
+	mc_filter[0] = 0;
+	mc_filter[1] = 0;
 
 	netdev_for_each_mc_addr(ha, dev) {
 		bitnr = hash_get_index(ha->addr);
-- 
2.4.3

[toc] | [prev] | [next] | [standalone]

#1351815

Le 07/03/2016 17:17, Moritz Fischer a écrit :
> Hi Nicolas,
> 
> this series deals with most of the checkpatch warnings
> generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
> that were suggested by checkpatch, that I can address in a follow up
> commit if needed.
> Let me know if you want me to split the fixes differently or squash
> them into one commit.

Hi,

I'm not usually fond of this type of patches, but I must admit that this
series corrects some style issues.

So, I would like more feedback from Michal and Cyrille as these changes
may delay some of the not-merged-yet features or more important
work-in-progress on their side.

On the other hand, if we all think it's a calm period for this macb
driver, we may find interesting to merge some "cleanup and style"
enhancements.

Thanks, bye,


> Moritz Fischer (3):
>   net: macb: Fix coding style error message
>   net: macb: Fix more coding style issues
>   net: macb: Address checkpatch 'check' suggestions
> 
>  drivers/net/ethernet/cadence/macb.c | 157 ++++++++++++++++--------------------
>  1 file changed, 71 insertions(+), 86 deletions(-)
> 


-- 
Nicolas Ferre

[toc] | [prev] | [next] | [standalone]

#1354259

On 7.3.2016 18:13, Nicolas Ferre wrote:
> Le 07/03/2016 17:17, Moritz Fischer a écrit :
>> Hi Nicolas,
>>
>> this series deals with most of the checkpatch warnings
>> generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
>> that were suggested by checkpatch, that I can address in a follow up
>> commit if needed.
>> Let me know if you want me to split the fixes differently or squash
>> them into one commit.
> 
> Hi,
> 
> I'm not usually fond of this type of patches, but I must admit that this
> series corrects some style issues.
> 
> So, I would like more feedback from Michal and Cyrille as these changes
> may delay some of the not-merged-yet features or more important
> work-in-progress on their side.
> 
> On the other hand, if we all think it's a calm period for this macb
> driver, we may find interesting to merge some "cleanup and style"
> enhancements.

Not a problem with merging cleanups in general. We have several out of
tree patches but doesn't make sense to to wait.
I wasn't in cc for the series but I don't like this change to be the
part of cleanup series.

	mac = of_get_mac_address(np);
 	if (mac)
-		memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
+		ether_addr_copy(bp->dev->dev_addr, mac);
 	else

Also extending scope of variables is not the right way to go. Especially
when some automation tools are reporting that you should reduce scope of
use for them. Wolfram is checking it for example.

Thanks,
Michal

[toc] | [prev] | [next] | [standalone]

#1354295

Hi all,

thanks for the feedback.

On Wed, Mar 9, 2016 at 8:29 AM, Michal Simek <michal.simek@xilinx.com> wrote:
> On 7.3.2016 18:13, Nicolas Ferre wrote:

>> I'm not usually fond of this type of patches, but I must admit that this
>> series corrects some style issues.

While I was playing around with my fixed-link for macb RFC I was
flooded with warnings,
so I figured I'll fix it while I'm at it. Just makes it easier for
other people to work
on it afterwards I thought.

>
>         mac = of_get_mac_address(np);
>         if (mac)
> -               memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
> +               ether_addr_copy(bp->dev->dev_addr, mac);
>         else

So don't you like this in general or just would like to have it split
out in a separate patch?

> Also extending scope of variables is not the right way to go. Especially
> when some automation tools are reporting that you should reduce scope of
> use for them. Wolfram is checking it for example.

Alright, understood, I'll address this.

Cheers,

Moritz

[toc] | [prev] | [next] | [standalone]

#1354298

On 9.3.2016 18:22, David Miller wrote:
> From: Michal Simek <michal.simek@xilinx.com>
> Date: Wed, 9 Mar 2016 17:29:39 +0100
> 
>> On 7.3.2016 18:13, Nicolas Ferre wrote:
>>> Le 07/03/2016 17:17, Moritz Fischer a écrit :
>>>> Hi Nicolas,
>>>>
>>>> this series deals with most of the checkpatch warnings
>>>> generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
>>>> that were suggested by checkpatch, that I can address in a follow up
>>>> commit if needed.
>>>> Let me know if you want me to split the fixes differently or squash
>>>> them into one commit.
>>>
>>> Hi,
>>>
>>> I'm not usually fond of this type of patches, but I must admit that this
>>> series corrects some style issues.
>>>
>>> So, I would like more feedback from Michal and Cyrille as these changes
>>> may delay some of the not-merged-yet features or more important
>>> work-in-progress on their side.
>>>
>>> On the other hand, if we all think it's a calm period for this macb
>>> driver, we may find interesting to merge some "cleanup and style"
>>> enhancements.
>>
>> Not a problem with merging cleanups in general. We have several out of
>> tree patches but doesn't make sense to to wait.
>> I wasn't in cc for the series but I don't like this change to be the
>> part of cleanup series.
>>
>> 	mac = of_get_mac_address(np);
>>  	if (mac)
>> -		memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
>> +		ether_addr_copy(bp->dev->dev_addr, mac);
> 
> Why?  This is what we tell people to use.

I would expect this as separate patch not the part of one huge cleanup
patch which does just comment and space cleanups.

Thanks,
Michal

[toc] | [prev] | [next] | [standalone]

#1354421

From: Michal Simek <michal.simek@xilinx.com>
Date: Wed, 9 Mar 2016 18:29:01 +0100

> On 9.3.2016 18:22, David Miller wrote:
>> From: Michal Simek <michal.simek@xilinx.com>
>> Date: Wed, 9 Mar 2016 17:29:39 +0100
>> 
>>> On 7.3.2016 18:13, Nicolas Ferre wrote:
>>>> Le 07/03/2016 17:17, Moritz Fischer a écrit :
>>>>> Hi Nicolas,
>>>>>
>>>>> this series deals with most of the checkpatch warnings
>>>>> generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
>>>>> that were suggested by checkpatch, that I can address in a follow up
>>>>> commit if needed.
>>>>> Let me know if you want me to split the fixes differently or squash
>>>>> them into one commit.
>>>>
>>>> Hi,
>>>>
>>>> I'm not usually fond of this type of patches, but I must admit that this
>>>> series corrects some style issues.
>>>>
>>>> So, I would like more feedback from Michal and Cyrille as these changes
>>>> may delay some of the not-merged-yet features or more important
>>>> work-in-progress on their side.
>>>>
>>>> On the other hand, if we all think it's a calm period for this macb
>>>> driver, we may find interesting to merge some "cleanup and style"
>>>> enhancements.
>>>
>>> Not a problem with merging cleanups in general. We have several out of
>>> tree patches but doesn't make sense to to wait.
>>> I wasn't in cc for the series but I don't like this change to be the
>>> part of cleanup series.
>>>
>>> 	mac = of_get_mac_address(np);
>>>  	if (mac)
>>> -		memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
>>> +		ether_addr_copy(bp->dev->dev_addr, mac);
>> 
>> Why?  This is what we tell people to use.
> 
> I would expect this as separate patch not the part of one huge cleanup
> patch which does just comment and space cleanups.

That is true.

[toc] | [prev] | [next] | [standalone]

#1354307

From: Michal Simek <michal.simek@xilinx.com>
Date: Wed, 9 Mar 2016 17:29:39 +0100

> On 7.3.2016 18:13, Nicolas Ferre wrote:
>> Le 07/03/2016 17:17, Moritz Fischer a écrit :
>>> Hi Nicolas,
>>>
>>> this series deals with most of the checkpatch warnings
>>> generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
>>> that were suggested by checkpatch, that I can address in a follow up
>>> commit if needed.
>>> Let me know if you want me to split the fixes differently or squash
>>> them into one commit.
>> 
>> Hi,
>> 
>> I'm not usually fond of this type of patches, but I must admit that this
>> series corrects some style issues.
>> 
>> So, I would like more feedback from Michal and Cyrille as these changes
>> may delay some of the not-merged-yet features or more important
>> work-in-progress on their side.
>> 
>> On the other hand, if we all think it's a calm period for this macb
>> driver, we may find interesting to merge some "cleanup and style"
>> enhancements.
> 
> Not a problem with merging cleanups in general. We have several out of
> tree patches but doesn't make sense to to wait.
> I wasn't in cc for the series but I don't like this change to be the
> part of cleanup series.
> 
> 	mac = of_get_mac_address(np);
>  	if (mac)
> -		memcpy(bp->dev->dev_addr, mac, ETH_ALEN);
> +		ether_addr_copy(bp->dev->dev_addr, mac);

Why?  This is what we tell people to use.

[toc] | [prev] | [next] | [standalone]

#1352679

Hi,

On Monday 07 March 2016 08:17:36, Moritz Fischer wrote:
> this series deals with most of the checkpatch warnings
> generated for macb. There are two BUG_ON()'s that I didn't touch, yet,
> that were suggested by checkpatch, that I can address in a follow up
> commit if needed.

I think addressing those BUG_ON() warnings would be nice as they can affect a running system pretty bad.

Best regards,
Alexander

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web