[KJ][PATCH]dev->mem_start default value (~0) test

[KJ][PATCH]dev->mem_start default value (~0) test

[Ravid Baruch Naali]

[-- Attachment #1: Type: text/plain, Size: 170 bytes --]

The attached patch covers all of the drivers/net directory adding the
dev->mem_start check for ~0

-- 
Ravid Baruch Naali
ravid@gmail.com
+972 4 6732729
+972 52 5830021


[-- Attachment #2: Type: text/plain, Size: 424 bytes --]

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

[KJ][PATCH]dev->mem_start default value (~0) test

[Ravid Baruch Naali]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

Trying to resend my patch hopefully the patch will be attached inline.
The attached patch covers all of the drivers/net directory adding the
dev->mem_start check for ~0

-- 
Ravid Baruch Naali
ravid@gmail.com
+972 4 6732729
+972 52 5830021



[-- Attachment #2: Type: text/plain, Size: 424 bytes --]

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

[KJ][PATCH]dev->mem_start default value (~0) test (final go)

[Ravid Baruch Naali]

The patch is now inserted as a text file (using Evolution), I hope
that's acceptable.
This patch applies to all the directory: drivers/net in which I've
checked and changed every check for the dev->mem_start = 0 to also
check for the default value hich is ~0.


diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/3c503.c ./drivers/net/3c503.c
--- ../vanilla/linux-2.6.22.1/drivers/net/3c503.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/3c503.c	2007-07-24 10:48:55.000000000 +0300
@@ -290,7 +290,8 @@ el2_probe1(struct net_device *dev, int i
 	}
 #endif  /* EL2MEMTEST */
 
-	if (dev->mem_start)
+	/*dev->mem_start can also be ~0 which is the default*/
+	if (dev->mem_start && dev->mem_start != ~0)
 		dev->mem_end = dev->mem_start + EL2_MEMSIZE;
 
 	if (wordlength) {	/* No Tx pages to skip over to get to Rx */
@@ -347,7 +348,8 @@ el2_probe1(struct net_device *dev, int i
     if (retval)
 	goto out1;
 
-    if (dev->mem_start)
+    /*dev->mem_start can also be ~0 which is the default*/
+    if (dev->mem_start && dev->mem_start != ~0)
 	printk("%s: %s - %dkB RAM, 8kB shared mem window at %#6lx-%#6lx.\n",
 		dev->name, ei_status.name, (wordlength+1)<<3,
 		dev->mem_start, dev->mem_end-1);
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/3c505.c ./drivers/net/3c505.c
--- ../vanilla/linux-2.6.22.1/drivers/net/3c505.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/3c505.c	2007-07-24 11:03:35.000000000 +0300
@@ -1526,7 +1526,8 @@ static int __init elplus_setup(struct ne
 
 	/* find a DMA channel */
 	if (!dev->dma) {
-		if (dev->mem_start) {
+		/*dev->mem_start can also be ~0 which is the default*/
+		if (dev->mem_start && dev->mem_start != ~0) {
 			dev->dma = dev->mem_start & 7;
 		}
 		else {
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/3c59x.c ./drivers/net/3c59x.c
--- ../vanilla/linux-2.6.22.1/drivers/net/3c59x.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/3c59x.c	2007-07-24 11:03:07.000000000 +0300
@@ -1040,7 +1040,8 @@ static int __devinit vortex_probe1(struc
 	option = global_options;
 
 	/* The lower four bits are the media type. */
-	if (dev->mem_start) {
+	/*dev->mem_start can also be ~0 which is the default*/
+	if (dev->mem_start && dev->mem_start != ~0) {
 		/*
 		 * The 'options' param is passed in as the third arg to the
 		 * LILO 'ether=' argument for non-modular use
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/ac3200.c ./drivers/net/ac3200.c
--- ../vanilla/linux-2.6.22.1/drivers/net/ac3200.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/ac3200.c	2007-07-24 11:01:35.000000000 +0300
@@ -204,7 +204,8 @@ static int __init ac_probe1(int ioaddr, 
 	dev->base_addr = ioaddr;
 
 #ifdef notyet
-	if (dev->mem_start)	{		/* Override the value from the board. */
+	/*dev->mem_start can also be ~0 which is the default*/
+	if (dev->mem_start && dev->mem_start != ~0) {	/* Override the value
from the board. */
 		for (i = 0; i < 7; i++)
 			if (addrmap[i] = dev->mem_start)
 				break;
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/depca.c ./drivers/net/depca.c
--- ../vanilla/linux-2.6.22.1/drivers/net/depca.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/depca.c	2007-07-24 11:10:56.000000000 +0300
@@ -604,8 +604,8 @@ static int __init depca_hw_init (struct 
 
 	lp = (struct depca_private *) dev->priv;
 	mem_start = lp->mem_start;
-
-	if (!mem_start || lp->adapter < DEPCA || lp->adapter >=unknown)
+	/*mem_start can also be ~0 which is the default*/
+	if (!mem_start || mem_start = ~0 || lp->adapter < DEPCA ||
lp->adapter >=unknown)
 		return -ENXIO;
 
 	printk ("%s: %s at 0x%04lx",
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/e2100.c ./drivers/net/e2100.c
--- ../vanilla/linux-2.6.22.1/drivers/net/e2100.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/e2100.c	2007-07-24 11:13:30.000000000 +0300
@@ -243,7 +243,8 @@ static int __init e21_probe1(struct net_
 	/* Never map in the E21 shared memory unless you are actively using
it.
 	   Also, the shared memory has effective only one setting -- spread
all
 	   over the 128K region! */
-	if (dev->mem_start = 0)
+	/*mem_start default is ~0 so an extra check was added*/   
+	if (dev->mem_start = 0 || dev->mem_start = ~0 )
 		dev->mem_start = 0xd0000;
 
 	ei_status.mem = ioremap(dev->mem_start, 2*1024);
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/eepro100.c ./drivers/net/eepro100.c
--- ../vanilla/linux-2.6.22.1/drivers/net/eepro100.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/eepro100.c	2007-07-24 11:16:00.000000000 +0300
@@ -637,8 +637,9 @@ static int __devinit speedo_found1(struc
 
 	SET_MODULE_OWNER(dev);
 	SET_NETDEV_DEV(dev, &pdev->dev);
-
-	if (dev->mem_start > 0)
+	/* Since mem_start is unsigned long an extra check of the default
+	 * value (~0) should be added*/
+	if (dev->mem_start > 0 && dev->mem_start != ~0)
 		option = dev->mem_start;
 	else if (card_idx >= 0  &&  options[card_idx] >= 0)
 		option = options[card_idx];
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/epic100.c ./drivers/net/epic100.c
--- ../vanilla/linux-2.6.22.1/drivers/net/epic100.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/epic100.c	2007-07-24 11:16:58.000000000 +0300
@@ -384,8 +384,8 @@ static int __devinit epic_init_one (stru
 		goto err_out_unmap_tx;
 	ep->rx_ring = (struct epic_rx_desc *)ring_space;
 	ep->rx_ring_dma = ring_dma;
-
-	if (dev->mem_start) {
+	/*dev->mem_start can also be ~0 which is the default*/
+        if (dev->mem_start && dev->mem_start != ~0) {
 		option = dev->mem_start;
 		duplex = (dev->mem_start & 16) ? 1 : 0;
 	} else if (card_idx >= 0  &&  card_idx < MAX_UNITS) {
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/es3210.c ./drivers/net/es3210.c
--- ../vanilla/linux-2.6.22.1/drivers/net/es3210.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/es3210.c	2007-07-24 11:18:34.000000000 +0300
@@ -244,8 +244,8 @@ static int __init es_probe1(struct net_d
 		retval = -EAGAIN;
 		goto out;
 	}
-
-	if (dev->mem_start = 0) {
+	/*dev->mem_start can also be ~0 which is the default*/
+        if (dev->mem_start = 0 || dev->mem_start = ~0) {
 		unsigned char mem_enabled = inb(ioaddr + ES_CFG2) & 0xc0;
 		unsigned char mem_bits = inb(ioaddr + ES_CFG3) & 0x07;
 
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/fealnx.c ./drivers/net/fealnx.c
--- ../vanilla/linux-2.6.22.1/drivers/net/fealnx.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/fealnx.c	2007-07-24 11:24:47.000000000 +0300
@@ -618,7 +618,8 @@ static int __devinit fealnx_init_one(str
 	}
 	np->mii.phy_id = np->phys[0];
 
-	if (dev->mem_start)
+	/*dev->mem_start can also be ~0 which is the default*/
+        if (dev->mem_start && dev->mem_start != ~0)
 		option = dev->mem_start;
 
 	/* The lower four bits are the media type. */
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/hamachi.c ./drivers/net/hamachi.c
--- ../vanilla/linux-2.6.22.1/drivers/net/hamachi.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/hamachi.c	2007-07-24 11:25:25.000000000 +0300
@@ -655,7 +655,8 @@ static int __devinit hamachi_init_one (s
 
 	/* Check for options being passed in */
 	option = card_idx < MAX_UNITS ? options[card_idx] : 0;
-	if (dev->mem_start)
+	/*dev->mem_start can also be ~0 which is the default*/
+        if (dev->mem_start && dev->mem_start != ~0)
 		option = dev->mem_start;
 
 	/* If the bus size is misidentified, do the following. */
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/ibmlana.c ./drivers/net/ibmlana.c
--- ../vanilla/linux-2.6.22.1/drivers/net/ibmlana.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/ibmlana.c	2007-07-24 11:28:52.000000000 +0300
@@ -926,7 +926,8 @@ static int ibmlana_probe(struct net_devi
 		/* were we looking for something different ? */
 		if (dev->irq && dev->irq != irq)
 			continue;
-		if (dev->mem_start && dev->mem_start != base)
+		/* Adding an extra check on mem_start's default value (~0)*/
+		if (dev->mem_start && dev->mem_start != ~0 && dev->mem_start != base)
 			continue;
 		/* found something that matches */
 		break;
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/lne390.c ./drivers/net/lne390.c
--- ../vanilla/linux-2.6.22.1/drivers/net/lne390.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/lne390.c	2007-07-24 11:31:49.000000000 +0300
@@ -223,8 +223,9 @@ static int __init lne390_probe1(struct n
 		printk (" unable to get IRQ %d.\n", dev->irq);
 		return ret;
 	}
-
-	if (dev->mem_start = 0) {
+	/* dev->mem_start default value is ~0 therefore additional check was
+	 * added*/
+	if (dev->mem_start = 0 && dev->mem_start != ~0) {
 		unsigned char mem_reg = inb(ioaddr + LNE390_CFG2) & 0x07;
 
 		if (revision)	/* LNE390B */
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/natsemi.c ./drivers/net/natsemi.c
--- ../vanilla/linux-2.6.22.1/drivers/net/natsemi.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/natsemi.c	2007-07-24 11:35:48.000000000 +0300
@@ -906,7 +906,8 @@ static int __devinit natsemi_probe1 (str
 	}
 
 	option = find_cnt < MAX_UNITS ? options[find_cnt] : 0;
-	if (dev->mem_start)
+	/*Adding a check for dev->mem_start's default value ~0*/
+	if (dev->mem_start && dev->mem_start != ~0)
 		option = dev->mem_start;
 
 	/* The lower four bits are the media type. */
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/ni52.c ./drivers/net/ni52.c
--- ../vanilla/linux-2.6.22.1/drivers/net/ni52.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/ni52.c	2007-07-24 11:37:57.000000000 +0300
@@ -460,7 +460,8 @@ static int __init ni52_probe1(struct net
 		goto out;
 	}
 #else
-	if(dev->mem_start != 0) /* no auto-mem-probe */
+	/* Adding a check for dev->mem_start'd default value ~0*/
+	if(dev->mem_start != 0 && dev->mem_start != ~0) /* no auto-mem-probe
*/
 	{
 		size = 0x4000; /* check for 16K mem */
 		if(!check586(dev,(char *) dev->mem_start,size)) {
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/starfire.c ./drivers/net/starfire.c
--- ../vanilla/linux-2.6.22.1/drivers/net/starfire.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/starfire.c	2007-07-24 11:41:21.000000000 +0300
@@ -806,7 +806,8 @@ static int __devinit starfire_init_one(s
 	drv_flags = netdrv_tbl[chip_idx].drv_flags;
 
 	option = card_idx < MAX_UNITS ? options[card_idx] : 0;
-	if (dev->mem_start)
+	/* Adding a check for dev->mem_start's defult value ~0 */
+	if (dev->mem_start && dev->mem_start != ~0)
 		option = dev->mem_start;
 
 	/* The lower four bits are the media type. */
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/tlan.c ./drivers/net/tlan.c
--- ../vanilla/linux-2.6.22.1/drivers/net/tlan.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/tlan.c	2007-07-24 11:42:54.000000000 +0300
@@ -616,7 +616,8 @@ static int __devinit TLan_probe1(struct 
 	}
 
 	/* Kernel parameters */
-	if (dev->mem_start) {
+	/* Adding a check for dev->mem_start's default value ~0 */
+	if (dev->mem_start && dev->mem_start != ~0) {
 		priv->aui    = dev->mem_start & 0x01;
 		priv->duplex = ((dev->mem_start & 0x06) = 0x06) ? 0 :
(dev->mem_start & 0x06) >> 1;
 		priv->speed  = ((dev->mem_start & 0x18) = 0x18) ? 0 :
(dev->mem_start & 0x18) >> 3;
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/tulip/winbond-840.c ./drivers/net/tulip/winbond-840.c
--- ../vanilla/linux-2.6.22.1/drivers/net/tulip/winbond-840.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/tulip/winbond-840.c	2007-07-24 11:56:41.000000000
+0300
@@ -401,8 +401,8 @@ static int __devinit w840_probe1 (struct
 	np->base_addr = ioaddr;
 
 	pci_set_drvdata(pdev, dev);
-
-	if (dev->mem_start)
+	/*Adding a check for dev->mem_start's defualt value ~0 */
+	if (dev->mem_start && dev->mem_start != ~0)
 		option = dev->mem_start;
 
 	/* The lower four bits are the media type. */
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/tulip/xircom_tulip_cb.c ./drivers/net/tulip/xircom_tulip_cb.c
--- ../vanilla/linux-2.6.22.1/drivers/net/tulip/xircom_tulip_cb.c
2007-07-10 21:56:30.000000000 +0300
+++ ./drivers/net/tulip/xircom_tulip_cb.c	2007-07-24 11:57:29.000000000
+0300
@@ -588,7 +588,8 @@ static int __devinit xircom_init_one(str
 		if (mtu[board_idx] > 0)
 			dev->mtu = mtu[board_idx];
 	}
-	if (dev->mem_start)
+	/*Adding a check for dev->mem_start's defualt value ~0 */
+	if (dev->mem_start && dev->mem_start != ~0)
 		tp->default_port = dev->mem_start;
 	if (tp->default_port) {
 		if (media_cap[tp->default_port] & MediaAlwaysFD)
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/wd.c ./drivers/net/wd.c
--- ../vanilla/linux-2.6.22.1/drivers/net/wd.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/wd.c	2007-07-24 11:44:42.000000000 +0300
@@ -239,7 +239,9 @@ static int __init wd_probe1(struct net_d
 	/* Allow a compile-time override.	 */
 	dev->mem_start = WD_SHMEM;
 #else
-	if (dev->mem_start = 0) {
+	/* dev->mem_start's default value is ~0 therefore an extra check was
+	 * added */ 
+	if (dev->mem_start = 0 || dev->mem_start = ~0) {
 		/* Sanity and old 8003 check */
 		int reg0 = inb(ioaddr);
 		if (reg0 = 0xff || reg0 = 0) {
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/wireless/arlan-main.c ./drivers/net/wireless/arlan-main.c
--- ../vanilla/linux-2.6.22.1/drivers/net/wireless/arlan-main.c
2007-07-10 21:56:30.000000000 +0300
+++ ./drivers/net/wireless/arlan-main.c	2007-07-24 10:13:19.000000000
+0300
@@ -1798,7 +1798,7 @@ struct net_device * __init arlan_probe(i
 		sprintf(dev->name, "eth%d", unit);
 		netdev_boot_setup_check(dev);
 		
-		if (dev->mem_start) {
+		if (dev->mem_start && dev->mem_start != ~0 ) {
 			if (arlan_probe_here(dev, dev->mem_start) = 0)
 				goto found;
 			goto not_found;
diff -uprN -X
Documentation/dontdiff ../vanilla/linux-2.6.22.1/drivers/net/yellowfin.c ./drivers/net/yellowfin.c
--- ../vanilla/linux-2.6.22.1/drivers/net/yellowfin.c	2007-07-10
21:56:30.000000000 +0300
+++ ./drivers/net/yellowfin.c	2007-07-24 11:46:06.000000000 +0300
@@ -448,8 +448,8 @@ static int __devinit yellowfin_init_one(
 		goto err_out_unmap_rx;
 	np->tx_status = (struct tx_status_words *)ring_space;
 	np->tx_status_dma = ring_dma;
-
-	if (dev->mem_start)
+	/* Adding a check for dev->mem_start's default value ~0 */
+	if (dev->mem_start && dev->mem_start != ~0)
 		option = dev->mem_start;
 
 	/* The lower four bits are the media type. */
 

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
&quot;subscribe kernel-janitors&quot; in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ][PATCH]dev->mem_start default value (~0) test (final go)

[Håkon Løvdal]

On 24/07/07, Ravid Baruch Naali <ravid@codefidence.com> wrote:
>
> -       if (dev->mem_start)
> +       /*dev->mem_start can also be ~0 which is the default*/
> +       if (dev->mem_start && dev->mem_start != ~0)
>                 dev->mem_end = dev->mem_start + EL2_MEMSIZE;
>

In my oppinion there are three ways to write code:

1. Write unreadable code

        if (answer = 42)

2. Write unreadable code and try to comment it readable

        if (answer = 42)    // life, the universe and everything

3. Write readable code
        #define LIFE_THE_UNIVERSE_AND_EVERYTHING 42
        ...
        if (answer = LIFE_THE_UNIVERSE_AND_EVERYTHING)


I think your patch would be better by not hardcoding ~0 with
a corresponding comment.

BR Håkon Løvdal

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
&quot;subscribe kernel-janitors&quot; in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ][PATCH]dev->mem_start default value (~0) test (final go)

[WANG Cong]

On Tue, Jul 24, 2007 at 03:03:14PM +0200, H??kon L??vdal wrote:
>On 24/07/07, Ravid Baruch Naali <ravid@codefidence.com> wrote:
>>
>>-       if (dev->mem_start)
>>+       /*dev->mem_start can also be ~0 which is the default*/
>>+       if (dev->mem_start && dev->mem_start != ~0)
>>                dev->mem_end = dev->mem_start + EL2_MEMSIZE;
>>
>
>In my oppinion there are three ways to write code:
>
>1. Write unreadable code
>
>       if (answer = 42)
>
>2. Write unreadable code and try to comment it readable
>
>       if (answer = 42)    // life, the universe and everything
>
>3. Write readable code
>       #define LIFE_THE_UNIVERSE_AND_EVERYTHING 42
>       ...
>       if (answer = LIFE_THE_UNIVERSE_AND_EVERYTHING)
>

Are you also a fan of The Hitchhiker¡¯s Guide to the Galaxy?

It's really a good science fiction.

>
>I think your patch would be better by not hardcoding ~0 with
>a corresponding comment.
>

I recommend the book named "The Practice of Programming" for you.
It tells you how to write readable and nice code.

-- 
To do great work, you have to have a pure mind. You can think only about the
mathematics. Everything else is human weakness. Accepting prizes is showing
weakness.

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
&quot;subscribe kernel-janitors&quot; in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ][PATCH]dev->mem_start default value (~0) test (final go)

[Cripps]

>>3. Write readable code
>>       #define LIFE_THE_UNIVERSE_AND_EVERYTHING 42
>>       ...
>>       if (answer = LIFE_THE_UNIVERSE_AND_EVERYTHING)
>
>I recommend the book named "The Practice of Programming" for you.
>It tells you how to write readable and nice code.

    I am in agreement, way number 3 is definitely the best way to write
code; I should probably start making my code
easier to read.
    In addition, Ravid, that's a fairly hefty patchfile. Personally, I would
split the existing file into individual patches
for each file being patched, that way it's easy to look over the patchfile
and go "okay, this all looks right to me,
lets move on." Splitting up large patchfiles makes processing patches
easier, and faster, for kernel maintainers.

-acripps
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ][PATCH]dev->mem_start default value (~0) test (final go)

[Ravid Baruch Naali]

Thanks for the comment,

But this 3 ways is one of the first lecture you get from you programming
teacher.

Where and what ever I developed adding an extra define for constant like
0, ~0, 0xffff etc.. always received the opposite comment "what's the
extra confusion for?"

So it's a matter of opinion but ask your self if you would define 0 any
where.

As for any arbitrary constant I totally agree with you.

I would love to here from any code maintainer what's the linux kernel
convention for the constant I mentioned above.

Håkon Løvdal wrote:
> On 24/07/07, Ravid Baruch Naali <ravid@codefidence.com> wrote:
>>
>> -       if (dev->mem_start)
>> +       /*dev->mem_start can also be ~0 which is the default*/
>> +       if (dev->mem_start && dev->mem_start != ~0)
>>                 dev->mem_end = dev->mem_start + EL2_MEMSIZE;
>>
> 
> In my oppinion there are three ways to write code:
> 
> 1. Write unreadable code
> 
>        if (answer = 42)
> 
> 2. Write unreadable code and try to comment it readable
> 
>        if (answer = 42)    // life, the universe and everything
> 
> 3. Write readable code
>        #define LIFE_THE_UNIVERSE_AND_EVERYTHING 42
>        ...
>        if (answer = LIFE_THE_UNIVERSE_AND_EVERYTHING)
> 
> 
> I think your patch would be better by not hardcoding ~0 with
> a corresponding comment.
> 
> BR Håkon Løvdal

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
&quot;subscribe kernel-janitors&quot; in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ][PATCH]dev->mem_start default value (~0) test (final go)

[Ravid Baruch Naali]

Your comment was on my mind before submitting the patch:

It's always hard to know what's best: splitting the long patch, which
repeat it self, or combined it to one. It took me a while to decide but
finally I came to the conclusion that I would expect my colleague, to
combine.
Looking at other patches in the archive I could not tell what's the rule
of thumb.

I also followed the FAQ answer "Try splitting them up per subsystem
(drivers/net/ ..." (all my patch refers to drivers/net)
So at the end of the day I'm left confuse at what is best.

Cripps wrote:
>>> 3. Write readable code
>>>       #define LIFE_THE_UNIVERSE_AND_EVERYTHING 42
>>>       ...
>>>       if (answer = LIFE_THE_UNIVERSE_AND_EVERYTHING)
>>
>> I recommend the book named "The Practice of Programming" for you.
>> It tells you how to write readable and nice code.
> 
>    I am in agreement, way number 3 is definitely the best way to write
> code; I should probably start making my code
> easier to read.
>    In addition, Ravid, that's a fairly hefty patchfile. Personally, I would
> split the existing file into individual patches
> for each file being patched, that way it's easy to look over the patchfile
> and go "okay, this all looks right to me,
> lets move on." Splitting up large patchfiles makes processing patches
> easier, and faster, for kernel maintainers.
> 
> -acripps
> 

_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
"subscribe kernel-janitors" in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors

Re: [KJ][PATCH]dev->mem_start default value (~0) test (final go)

[Cripps]

Ravid,
Duly noted. I was told by the friendly folks here on KJ to ensure that each
patch was for one problem, and one problem only,
and I just split up each patch by file (since there were multiple changes in
each file). A good compromise might be to patch
only X number of files at a time in the same subsystem. Since my patches
cover different subdirectories in the drivers/net
subsystem, I would definitely break up by directory, but since yours all
appear to be in the same directory, making the patches
shorter, but not *too* short might also be a good idea.

-acripps

On 7/24/07, Ravid Baruch Naali <ravid@codefidence.com> wrote:
>
> Your comment was on my mind before submitting the patch:
>
> It's always hard to know what's best: splitting the long patch, which
> repeat it self, or combined it to one. It took me a while to decide but
> finally I came to the conclusion that I would expect my colleague, to
> combine.
> Looking at other patches in the archive I could not tell what's the rule
> of thumb.
>
> I also followed the FAQ answer "Try splitting them up per subsystem
> (drivers/net/ ..." (all my patch refers to drivers/net)
> So at the end of the day I'm left confuse at what is best.
>
> Cripps wrote:
> >>> 3. Write readable code
> >>>       #define LIFE_THE_UNIVERSE_AND_EVERYTHING 42
> >>>       ...
> >>>       if (answer = LIFE_THE_UNIVERSE_AND_EVERYTHING)
> >>
> >> I recommend the book named "The Practice of Programming" for you.
> >> It tells you how to write readable and nice code.
> >
> >    I am in agreement, way number 3 is definitely the best way to write
> > code; I should probably start making my code
> > easier to read.
> >    In addition, Ravid, that's a fairly hefty patchfile. Personally, I
> would
> > split the existing file into individual patches
> > for each file being patched, that way it's easy to look over the
> patchfile
> > and go "okay, this all looks right to me,
> > lets move on." Splitting up large patchfiles makes processing patches
> > easier, and faster, for kernel maintainers.
> >
> > -acripps
> >
>
>
_______________________________________________
REMINDER: this mailing list moved to vger.kernel.org and current one will be discontinued soon.
To resubscribe, send email to majordomo@vger.kernel.org with
&quot;subscribe kernel-janitors&quot; in message body and follow instructions.

Kernel-janitors mailing list
Kernel-janitors@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/kernel-janitors