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

Groups > linux.kernel > #1246790 > unrolled thread

[PATCH] staging: octeon: fixed few coding style warnings

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: octeon: fixed few coding style warnings Sakshi Bansal <sakshi.april5@gmail.com> - 2015-10-14 16:10 +0200
    Re: [PATCH] staging: octeon: fixed few coding style warnings David Daney <ddaney@caviumnetworks.com> - 2015-10-14 18:10 +0200
      Re: [PATCH] staging: octeon: fixed few coding style warnings Arnd Bergmann <arnd@arndb.de> - 2015-10-14 23:00 +0200
        Re: [PATCH] staging: octeon: fixed few coding style warnings David Daney <ddaney@caviumnetworks.com> - 2015-10-14 23:10 +0200
          Re: [PATCH] staging: octeon: fixed few coding style warnings Arnd Bergmann <arnd@arndb.de> - 2015-10-14 23:40 +0200
      Re: [PATCH] staging: octeon: fixed few coding style warnings Dan Carpenter <dan.carpenter@oracle.com> - 2015-10-15 15:50 +0200
        Re: [PATCH] staging: octeon: fixed few coding style warnings Dan Carpenter <dan.carpenter@oracle.com> - 2015-10-15 20:20 +0200
    Re: [PATCH] staging: octeon: fixed few coding style warnings Aaro Koskinen <aaro.koskinen@iki.fi> - 2015-10-14 21:40 +0200

#1246790 — [PATCH] staging: octeon: fixed few coding style warnings

Fixed allignment issues and line over 80 characters

Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com>
---
 drivers/staging/octeon/ethernet-defines.h |  2 +-
 drivers/staging/octeon/ethernet-mdio.c    |  2 +-
 drivers/staging/octeon/ethernet-mem.c     |  4 ++--
 drivers/staging/octeon/ethernet-rgmii.c   |  7 +++----
 drivers/staging/octeon/ethernet-rx.c      | 27 +++++++++++++--------------
 drivers/staging/octeon/ethernet-rx.h      |  2 +-
 drivers/staging/octeon/ethernet-spi.c     |  2 --
 drivers/staging/octeon/ethernet-tx.c      | 26 ++++++++++++--------------
 drivers/staging/octeon/ethernet-xaui.c    |  2 +-
 drivers/staging/octeon/ethernet.c         |  2 +-
 10 files changed, 35 insertions(+), 41 deletions(-)

diff --git a/drivers/staging/octeon/ethernet-defines.h b/drivers/staging/octeon/ethernet-defines.h
index 7164005..c86d905 100644
--- a/drivers/staging/octeon/ethernet-defines.h
+++ b/drivers/staging/octeon/ethernet-defines.h
@@ -40,7 +40,7 @@
 #define FAU_TOTAL_TX_TO_CLEAN (CVMX_FAU_REG_END - sizeof(u32))
 #define FAU_NUM_PACKET_BUFFERS_TO_FREE (FAU_TOTAL_TX_TO_CLEAN - sizeof(u32))
 
-#define TOTAL_NUMBER_OF_PORTS       (CVMX_PIP_NUM_INPUT_PORTS+1)
+#define TOTAL_NUMBER_OF_PORTS       (CVMX_PIP_NUM_INPUT_PORTS + 1)
 
 
 #endif /* __ETHERNET_DEFINES_H__ */
diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
index fd9b3d8..590a6cb 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
 	priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
 				      PHY_INTERFACE_MODE_GMII);
 
-	if (priv->phydev == NULL)
+	if (!priv->phydev)
 		return -ENODEV;
 
 	priv->last_link = 0;
diff --git a/drivers/staging/octeon/ethernet-mem.c b/drivers/staging/octeon/ethernet-mem.c
index 5a5cdb3..d6172e4 100644
--- a/drivers/staging/octeon/ethernet-mem.c
+++ b/drivers/staging/octeon/ethernet-mem.c
@@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int pool, int size, int elements)
 	while (freed) {
 		struct sk_buff *skb = dev_alloc_skb(size + 256);
 
-		if (unlikely(skb == NULL))
+		if (unlikely(!skb))
 			break;
 		skb_reserve(skb, 256 - (((unsigned long)skb->data) & 0x7f));
 		*(struct sk_buff **)(skb->data - sizeof(void *)) = skb;
@@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int pool, int size, int elements)
 		 * just before the block.
 		 */
 		memory = kmalloc(size + 256, GFP_ATOMIC);
-		if (unlikely(memory == NULL)) {
+		if (unlikely(!memory)) {
 			pr_warn("Unable to allocate %u bytes for FPA pool %d\n",
 				elements * size, pool);
 			break;
diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
index 51dcb61..3d7513c 100644
--- a/drivers/staging/octeon/ethernet-rgmii.c
+++ b/drivers/staging/octeon/ethernet-rgmii.c
@@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 	struct octeon_ethernet *priv = netdev_priv(dev);
 	unsigned long flags = 0;
 	cvmx_helper_link_info_t link_info;
-	int use_global_register_lock = (priv->phydev == NULL);
+	int use_global_register_lock = (!priv->phydev);
 
 	BUG_ON(in_interrupt());
 	if (use_global_register_lock) {
@@ -124,7 +124,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 	 */
 	cvm_oct_set_hw_preamble(priv, true);
 
-	if (priv->phydev == NULL) {
+	if (!priv->phydev) {
 		link_info = cvmx_helper_link_autoconf(priv->port);
 		priv->link_info = link_info.u64;
 	}
@@ -134,7 +134,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
 	else
 		mutex_unlock(&priv->phydev->bus->mdio_lock);
 
-	if (priv->phydev == NULL) {
+	if (!priv->phydev) {
 		/* Tell core. */
 		if (link_info.s.link_up) {
 			if (!netif_carrier_ok(dev))
@@ -285,7 +285,6 @@ void cvm_oct_rgmii_uninit(struct net_device *dev)
 	    || (priv->imode == CVMX_HELPER_INTERFACE_MODE_RGMII)) {
 
 		if (!octeon_is_simulation()) {
-
 			union cvmx_gmxx_rxx_int_en gmx_rx_int_en;
 			int interface = INTERFACE(priv->port);
 			int index = INDEX(priv->port);
diff --git a/drivers/staging/octeon/ethernet-rx.c b/drivers/staging/octeon/ethernet-rx.c
index 6aed3cf..170cb12 100644
--- a/drivers/staging/octeon/ethernet-rx.c
+++ b/drivers/staging/octeon/ethernet-rx.c
@@ -103,7 +103,6 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 		gmxx_rxx_frm_ctl.u64 =
 		    cvmx_read_csr(CVMX_GMXX_RXX_FRM_CTL(index, interface));
 		if (gmxx_rxx_frm_ctl.s.pre_chk == 0) {
-
 			u8 *ptr =
 			    cvmx_phys_to_ptr(work->packet_ptr.s.addr);
 			int i = 0;
@@ -117,15 +116,15 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 
 			if (*ptr == 0xd5) {
 				/*
-				  printk_ratelimited("Port %d received 0xd5 preamble\n",
-					  port);
+				 * printk_ratelimited("Port %d received 0xd5 preamble\n"
+				 *		      port);
 				 */
 				work->packet_ptr.s.addr += i + 1;
 				work->word1.len -= i + 5;
 			} else if ((*ptr & 0xf) == 0xd) {
 				/*
-				  printk_ratelimited("Port %d received 0x?d preamble\n",
-					  port);
+				 * printk_ratelimited("Port %d received 0x?d preamble\n",
+				 *		      port);
 				 */
 				work->packet_ptr.s.addr += i;
 				work->word1.len -= i + 4;
@@ -139,7 +138,7 @@ static inline int cvm_oct_check_rcv_error(cvmx_wqe_t *work)
 				printk_ratelimited("Port %d unknown preamble, packet dropped\n",
 						   port);
 				/*
-				   cvmx_helper_dump_packet(work);
+				 * cvmx_helper_dump_packet(work);
 				 */
 				cvm_oct_free_work(work);
 				return 1;
@@ -184,7 +183,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 	if (OCTEON_IS_MODEL(OCTEON_CN68XX)) {
 		old_group_mask = cvmx_read_csr(CVMX_SSO_PPX_GRP_MSK(coreid));
 		cvmx_write_csr(CVMX_SSO_PPX_GRP_MSK(coreid),
-				1ull << pow_receive_group);
+			       1ull << pow_receive_group);
 		cvmx_read_csr(CVMX_SSO_PPX_GRP_MSK(coreid)); /* Flush */
 	} else {
 		old_group_mask = cvmx_read_csr(CVMX_POW_PP_GRP_MSKX(coreid));
@@ -211,7 +210,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 
 		prefetch(work);
 		did_work_request = 0;
-		if (work == NULL) {
+		if (!work) {
 			if (OCTEON_IS_MODEL(OCTEON_CN68XX)) {
 				cvmx_write_csr(CVMX_SSO_WQ_IQ_DIS,
 					       1ull << pow_receive_group);
@@ -380,9 +379,9 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 			} else {
 				/* Drop any packet received for a device that isn't up */
 				/*
-				  printk_ratelimited("%s: Device not up, packet dropped\n",
-					   dev->name);
-				*/
+				 * printk_ratelimited("%s: Device not up, packet dropped\n",
+				 *                    dev->name);
+				 */
 #ifdef CONFIG_64BIT
 				atomic64_add(1,
 					     (atomic64_t *)&priv->stats.rx_dropped);
@@ -398,7 +397,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 			 * doesn't exist.
 			 */
 			printk_ratelimited("Port %d not controlled by Linux, packet dropped\n",
-				   port);
+					   port);
 			dev_kfree_skb_irq(skb);
 		}
 		/*
@@ -433,7 +432,7 @@ static int cvm_oct_napi_poll(struct napi_struct *napi, int budget)
 	}
 	cvm_oct_rx_refill_pool(0);
 
-	if (rx_count < budget && napi != NULL) {
+	if (rx_count < budget && napi) {
 		/* No more work */
 		napi_complete(napi);
 		enable_irq(OCTEON_IRQ_WORKQ0 + pow_receive_group);
@@ -466,7 +465,7 @@ void cvm_oct_rx_initialize(void)
 		}
 	}
 
-	if (NULL == dev_for_napi)
+	if (!dev_for_napi)
 		panic("No net_devices were allocated.");
 
 	netif_napi_add(dev_for_napi, &cvm_oct_napi, cvm_oct_napi_poll,
diff --git a/drivers/staging/octeon/ethernet-rx.h b/drivers/staging/octeon/ethernet-rx.h
index a5973fd..315a63d 100644
--- a/drivers/staging/octeon/ethernet-rx.h
+++ b/drivers/staging/octeon/ethernet-rx.h
@@ -30,7 +30,7 @@ static inline void cvm_oct_rx_refill_pool(int fill_threshold)
 						 number_to_free);
 		if (num_freed != number_to_free) {
 			cvmx_fau_atomic_add32(FAU_NUM_PACKET_BUFFERS_TO_FREE,
-					number_to_free - num_freed);
+					      number_to_free - num_freed);
 		}
 	}
 }
diff --git a/drivers/staging/octeon/ethernet-spi.c b/drivers/staging/octeon/ethernet-spi.c
index 2ae1944..063dcd0 100644
--- a/drivers/staging/octeon/ethernet-spi.c
+++ b/drivers/staging/octeon/ethernet-spi.c
@@ -167,9 +167,7 @@ static void cvm_oct_spi_poll(struct net_device *dev)
 	int interface;
 
 	for (interface = 0; interface < 2; interface++) {
-
 		if ((priv->port == interface * 16) && need_retrain[interface]) {
-
 			if (cvmx_spi_restart_interface
 			    (interface, CVMX_SPI_MODE_DUPLEX, 10) == 0) {
 				need_retrain[interface] = 0;
diff --git a/drivers/staging/octeon/ethernet-tx.c b/drivers/staging/octeon/ethernet-tx.c
index 3a5d8f5..afbe537 100644
--- a/drivers/staging/octeon/ethernet-tx.c
+++ b/drivers/staging/octeon/ethernet-tx.c
@@ -95,11 +95,10 @@ static void cvm_oct_free_tx_skbs(struct net_device *dev)
 	for (qos = 0; qos < queues_per_port; qos++) {
 		if (skb_queue_len(&priv->tx_free_list[qos]) == 0)
 			continue;
-		skb_to_free = cvmx_fau_fetch_and_add32(priv->fau+qos*4,
+		skb_to_free = cvmx_fau_fetch_and_add32(priv->fau + qos * 4,
 						       MAX_SKB_TO_FREE);
 		skb_to_free = cvm_oct_adjust_skb_to_free(skb_to_free,
-							 priv->fau+qos*4);
-
+							 priv->fau + qos * 4);
 
 		total_freed += skb_to_free;
 		if (skb_to_free > 0) {
@@ -318,37 +317,37 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely
 	    ((skb_end_pointer(skb) - fpa_head) < CVMX_FPA_PACKET_POOL_SIZE)) {
 		/*
-		   printk("TX buffer isn't large enough for the FPA\n");
+		 * printk("TX buffer isn't large enough for the FPA\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_shared(skb))) {
 		/*
-		   printk("TX buffer sharing data with someone else\n");
+		 * printk("TX buffer sharing data with someone else\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_cloned(skb))) {
 		/*
-		   printk("TX buffer has been cloned\n");
+		 * printk("TX buffer has been cloned\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_header_cloned(skb))) {
 		/*
-		   printk("TX buffer header has been cloned\n");
+		 * printk("TX buffer header has been cloned\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb->destructor)) {
 		/*
-		   printk("TX buffer has a destructor\n");
+		 * printk("TX buffer has a destructor\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
 	if (unlikely(skb_shinfo(skb)->nr_frags)) {
 		/*
-		   printk("TX buffer has fragments\n");
+		 * printk("TX buffer has fragments\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
@@ -356,7 +355,7 @@ int cvm_oct_xmit(struct sk_buff *skb, struct net_device *dev)
 	    (skb->truesize !=
 	     sizeof(*skb) + skb_end_offset(skb))) {
 		/*
-		   printk("TX buffer truesize has been changed\n");
+		 * printk("TX buffer truesize has been changed\n");
 		 */
 		goto dont_put_skbuff_in_hw;
 	}
@@ -430,7 +429,7 @@ dont_put_skbuff_in_hw:
 
 	if (pko_command.s.dontfree) {
 		queue_type = QUEUE_CORE;
-		pko_command.s.reg0 = priv->fau+qos*4;
+		pko_command.s.reg0 = priv->fau + qos * 4;
 	} else {
 		queue_type = QUEUE_HW;
 	}
@@ -443,7 +442,6 @@ dont_put_skbuff_in_hw:
 	/* Drop this packet if we have too many already queued to the HW */
 	if (unlikely(skb_queue_len(&priv->tx_free_list[qos]) >=
 		     MAX_OUT_QUEUE_DEPTH)) {
-
 		if (dev->tx_queue_len != 0) {
 			/* Drop the lock when notifying the core.  */
 			spin_unlock_irqrestore(&priv->tx_free_list[qos].lock,
@@ -549,7 +547,7 @@ int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
 	/* Get a work queue entry */
 	cvmx_wqe_t *work = cvmx_fpa_alloc(CVMX_FPA_WQE_POOL);
 
-	if (unlikely(work == NULL)) {
+	if (unlikely(!work)) {
 		printk_ratelimited("%s: Failed to allocate a work queue entry\n",
 				   dev->name);
 		priv->stats.tx_dropped++;
@@ -559,7 +557,7 @@ int cvm_oct_xmit_pow(struct sk_buff *skb, struct net_device *dev)
 
 	/* Get a packet buffer */
 	packet_buffer = cvmx_fpa_alloc(CVMX_FPA_PACKET_POOL);
-	if (unlikely(packet_buffer == NULL)) {
+	if (unlikely(!packet_buffer)) {
 		printk_ratelimited("%s: Failed to allocate a packet buffer\n",
 				   dev->name);
 		cvmx_fpa_free(work, CVMX_FPA_WQE_POOL, 1);
diff --git a/drivers/staging/octeon/ethernet-xaui.c b/drivers/staging/octeon/ethernet-xaui.c
index 4b47bcf..978407e 100644
--- a/drivers/staging/octeon/ethernet-xaui.c
+++ b/drivers/staging/octeon/ethernet-xaui.c
@@ -35,7 +35,7 @@ int cvm_oct_xaui_init(struct net_device *dev)
 	struct octeon_ethernet *priv = netdev_priv(dev);
 
 	cvm_oct_common_init(dev);
-	if (!octeon_is_simulation() && priv->phydev == NULL)
+	if (!octeon_is_simulation() && !priv->phydev)
 		priv->poll = cvm_oct_link_poll;
 
 	return 0;
diff --git a/drivers/staging/octeon/ethernet.c b/drivers/staging/octeon/ethernet.c
index c9183e0..6e81e79 100644
--- a/drivers/staging/octeon/ethernet.c
+++ b/drivers/staging/octeon/ethernet.c
@@ -674,7 +674,7 @@ static int cvm_oct_probe(struct platform_device *pdev)
 	}
 
 	cvm_oct_poll_queue = create_singlethread_workqueue("octeon-ethernet");
-	if (cvm_oct_poll_queue == NULL) {
+	if (!cvm_oct_poll_queue) {
 		pr_err("octeon-ethernet: Cannot create workqueue");
 		return -ENOMEM;
 	}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1246956

On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
> Fixed allignment issues and line over 80 characters

Use spell checking on 'allignment'

But that is not the main problem with the patch...


You are changing things other than white space and comment formatting, 
can you tell us on which platforms the patch was tested to verify that 
you didn't break anything?

>
> Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com>


NAK.

[...]
> diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
> index fd9b3d8..590a6cb 100644
> --- a/drivers/staging/octeon/ethernet-mdio.c
> +++ b/drivers/staging/octeon/ethernet-mdio.c
> @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
>   	priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
>   				      PHY_INTERFACE_MODE_GMII);
>
> -	if (priv->phydev == NULL)
> +	if (!priv->phydev)


Not a coding style change.  There is no WARNING generated for this case.



>   		return -ENODEV;
>
>   	priv->last_link = 0;
> diff --git a/drivers/staging/octeon/ethernet-mem.c b/drivers/staging/octeon/ethernet-mem.c
> index 5a5cdb3..d6172e4 100644
> --- a/drivers/staging/octeon/ethernet-mem.c
> +++ b/drivers/staging/octeon/ethernet-mem.c
> @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int pool, int size, int elements)
>   	while (freed) {
>   		struct sk_buff *skb = dev_alloc_skb(size + 256);
>
> -		if (unlikely(skb == NULL))
> +		if (unlikely(!skb))

Same

>   			break;
>   		skb_reserve(skb, 256 - (((unsigned long)skb->data) & 0x7f));
>   		*(struct sk_buff **)(skb->data - sizeof(void *)) = skb;
> @@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int pool, int size, int elements)
>   		 * just before the block.
>   		 */
>   		memory = kmalloc(size + 256, GFP_ATOMIC);
> -		if (unlikely(memory == NULL)) {
> +		if (unlikely(!memory)) {

Same

>   			pr_warn("Unable to allocate %u bytes for FPA pool %d\n",
>   				elements * size, pool);
>   			break;
> diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c
> index 51dcb61..3d7513c 100644
> --- a/drivers/staging/octeon/ethernet-rgmii.c
> +++ b/drivers/staging/octeon/ethernet-rgmii.c
> @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
>   	struct octeon_ethernet *priv = netdev_priv(dev);
>   	unsigned long flags = 0;
>   	cvmx_helper_link_info_t link_info;
> -	int use_global_register_lock = (priv->phydev == NULL);
> +	int use_global_register_lock = (!priv->phydev);

Same.

I could go on, but I think you see the pattern here.

Your changelog says you are fixing warnings, but none of these are 
warning fixes.

In fact it is perfectly acceptable to compare a pointer to NULL.  It is 
a common idiom in the kernel.  The original author of the code thought 
it was more clear this way, and you are causing code churn for no reason.

Try to run this command on the kernel sources:
$ git grep -e '== NULL' | wc -l
21488

I would suggest that you convince people that the other 21,000 cases of 
comparison to NULL need changing before you do it to this driver.

David Daney

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247147

On Wednesday 14 October 2015 08:47:44 David Daney wrote:
> On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
> > Fixed allignment issues and line over 80 characters
> 
> Use spell checking on 'allignment'
> 
> But that is not the main problem with the patch...
> 
> 
> You are changing things other than white space and comment formatting, 
> can you tell us on which platforms the patch was tested to verify that 
> you didn't break anything?

In general a good advice, but for trivial whitespace or comment
changes, this is normally not necessary. Compile-testing a patch
as you say is normally expected, if only to avoid embarrassing
complaints if it does break later.

For drivers that are not enabled in the x86 allmodconfig, it sure
helps to say something like "Compile-tested using MIPS cross toolchain
from https://www.kernel.org/pub/tools/crosstool/". Even better would
be to send a fix to decouple the driver from asm/octeon/*.h
to make it build on all architectures, but that is much more work
than I'd expect for a trivial patch.

> > Signed-off-by: Sakshi Bansal <sakshi.april5@gmail.com>
> 
> 
> NAK.

Actually all the changes in the patch look reasonable for a staging driver,
but the submission form needs to be improved.

I'm guessing this was a first-time patch submission (presumably for
an outreachy application), which means it's pretty much expected to
get a few things wrong and correct them in a follow-up.

> > @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
> >       struct octeon_ethernet *priv = netdev_priv(dev);
> >       unsigned long flags = 0;
> >       cvmx_helper_link_info_t link_info;
> > -     int use_global_register_lock = (priv->phydev == NULL);
> > +     int use_global_register_lock = (!priv->phydev);
> 
> Same.
> 
> I could go on, but I think you see the pattern here.
> 
> Your changelog says you are fixing warnings, but none of these are 
> warning fixes.
> 
> In fact it is perfectly acceptable to compare a pointer to NULL.  It is 
> a common idiom in the kernel.  The original author of the code thought 
> it was more clear this way, and you are causing code churn for no reason.

The style she changed it to is the preferred style however. We don't
normally send or take patches to change this in regular code, but
for drivers/staging/ this is the kind of patch you should expect to
see.

To improve the patch, I would suggest to split it up into smaller chunks
in more logical units. "fixed few coding style warnings" is not a good
one-line description for a patch, as it already hints that the patch
is doing multiple unrelated changes, and is not unique in the sense
that another patch could have the exact same description and do something
completely different.

Instead, this can be done as a series of three patches:

[PATCH 1/3] staging: octeon: fix commenting style
[PATCH 2/3] staging: octeon: avoid redundant "== NULL" comparison
[PATCH 3/3] staging: octeon: whitespace fixes

All three of these are still trivial of course, but this is already
made clear by the subject line. In the detailed description, there
should be an explanation for each patch why the new version is
considered an improvement over the existing code. If you cannot come
up with an explanation other than 'checkpatch said this', it's better
to drop the patch, but with a good description most arguments about
the merits of a change can be avoided.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247171

On 10/14/2015 01:58 PM, Arnd Bergmann wrote:
> On Wednesday 14 October 2015 08:47:44 David Daney wrote:
>> On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
>>> Fixed allignment issues and line over 80 characters
>>
>> Use spell checking on 'allignment'
>>
>> But that is not the main problem with the patch...
>>
>>
>> You are changing things other than white space and comment formatting,
>> can you tell us on which platforms the patch was tested to verify that
>> you didn't break anything?
>
> In general a good advice, but for trivial whitespace or comment
> changes, this is normally not necessary. Compile-testing a patch
> as you say is normally expected, if only to avoid embarrassing
> complaints if it does break later.
>
> For drivers that are not enabled in the x86 allmodconfig, it sure
> helps to say something like "Compile-tested using MIPS cross toolchain
> from https://www.kernel.org/pub/tools/crosstool/". Even better would
> be to send a fix to decouple the driver from asm/octeon/*.h
> to make it build on all architectures, but that is much more work
> than I'd expect for a trivial patch.

There is no point in such a patch.  We know, a priori, that there will 
*never* be a non-MIPS system with this hardware.

David Daney


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247206

On Wednesday 14 October 2015 14:04:00 David Daney wrote:
> On 10/14/2015 01:58 PM, Arnd Bergmann wrote:
> > On Wednesday 14 October 2015 08:47:44 David Daney wrote:
> >> On 10/14/2015 07:06 AM, Sakshi Bansal wrote:
> >>> Fixed allignment issues and line over 80 characters
> >>
> >> Use spell checking on 'allignment'
> >>
> >> But that is not the main problem with the patch...
> >>
> >>
> >> You are changing things other than white space and comment formatting,
> >> can you tell us on which platforms the patch was tested to verify that
> >> you didn't break anything?
> >
> > In general a good advice, but for trivial whitespace or comment
> > changes, this is normally not necessary. Compile-testing a patch
> > as you say is normally expected, if only to avoid embarrassing
> > complaints if it does break later.
> >
> > For drivers that are not enabled in the x86 allmodconfig, it sure
> > helps to say something like "Compile-tested using MIPS cross toolchain
> > from https://www.kernel.org/pub/tools/crosstool/". Even better would
> > be to send a fix to decouple the driver from asm/octeon/*.h
> > to make it build on all architectures, but that is much more work
> > than I'd expect for a trivial patch.
> 
> There is no point in such a patch.  We know, a priori, that there will 
> *never* be a non-MIPS system with this hardware.

We generally like to have all drivers compilable using COMPILE_TEST
across all architectures, as that enables automated build regression
tests from services like coverity or the 0-day build tester.

It also makes it much easier for normal developers to test changes
when they can verify them using a single toolchain and allmodconfig.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247807

On Wed, Oct 14, 2015 at 09:09:04AM -0700, David Daney wrote:
> and make sure you don't break the driver.

I'm not seeing how this patch breaks anything?  What am I missing?

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1248058

Oh.  I use a script to check these...  It doesn't break anything except
for that commented out code which should be deleted anyway.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1247112

Hi,

On Wed, Oct 14, 2015 at 07:36:37PM +0530, Sakshi Bansal wrote:
> Fixed allignment issues and line over 80 characters

You are also doing many other things which are not mention in the
commit log.

>  				/*
> -				  printk_ratelimited("Port %d received 0xd5 preamble\n",
> -					  port);
> +				 * printk_ratelimited("Port %d received 0xd5 preamble\n"
> +				 *		      port);
>  				 */

This is not really an improvement. Either the dead code should be deleted
altogether or converted to debug printk()s. (Probably the idea has been
that those printk()s can be "easily" enabled for debugging by removing
comment start/end marks.)

You also removed a comma there, breaking the syntax...

A.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web