[PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Rasmus Andersen]

Hi.

(Does anybody know who the maintainer for this code is?)

The patch below moves drivers/scsi/g_NCR5380.c from using isa_{read,
write, memcpy} to ioremap and the equivalent non-isa functions. It
also fixes an 'unused variable' warning and exchanges check_region for
request_region (and fixes a possible scenario where the checked-for
region was unequal in size to the later requested one).

(An indication of how often this code path is used can be found in
the fact that the previous define of NCR5380_write had its payload
and address mixed up, probably making for wierd results should
the code ever be executed.)

Since I feel a bit wierd after following the #defines around in the
code I would appreciate somebody else eyeballing this.


diff -aur linux-242-ac3-clean/drivers/scsi/g_NCR5380.c linux-242-ac3/drivers/scsi/g_NCR5380.c
--- linux-242-ac3-clean/drivers/scsi/g_NCR5380.c	Sat Feb 24 21:20:21 2001
+++ linux-242-ac3/drivers/scsi/g_NCR5380.c	Sun Feb 25 14:07:57 2001
@@ -144,6 +144,10 @@
     [1] __initdata = {{0,},};
 #endif
 
+#ifdef CONFIG_SCSI_G_NCR5380_MEM
+static void *isa_remap_ptr;
+#endif
+
 #define NO_OVERRIDES (sizeof(overrides) / sizeof(struct override))
 
 /*
@@ -264,7 +268,7 @@
 
 int __init generic_NCR5380_detect(Scsi_Host_Template * tpnt){
     static int current_override = 0;
-    int count, i;
+    int count;
     u_int *ports;
     u_int ncr_53c400a_ports[] = {0x280, 0x290, 0x300, 0x310, 0x330,
     		     		0x340, 0x348, 0x350, 0};
@@ -344,6 +348,7 @@
 
 #ifdef CONFIG_SCSI_G_NCR5380_PORT
 	if (ports) {
+	    int i;
 	    /* wakeup sequence for the NCR53C400A and DTC3181E*/
 
 	    /* Disable the adapter and look for a free io port */
@@ -361,8 +366,8 @@
 	        }
 	    else
 	        for(i=0; ports[i]; i++) {
-	            if ((!check_region(ports[i], 16)) && (inb(ports[i]) == 0xff))
-	                break;
+		    if ((inb(ports[i]) == 0xff) && (request_region(ports[i], NCR5380_region_size, "ncr5380")))
+		        break;
 		}
 	    if (ports[i]) {
 	        outb(0x59, 0x779);
@@ -373,33 +378,25 @@
 	        outb(0x80 | i, 0x379);          /* set io port to be used */
         	outb(0xc0, ports[i] + 9);
 	        if (inb(ports[i] + 9) != 0x80)
-	            continue;
+		    goto err_release;
 	        else
 	            overrides[current_override].NCR5380_map_name=ports[i];
 	    } else
-	        continue;
+	        goto err_release;
 	}
-
-	request_region(overrides[current_override].NCR5380_map_name,
-					NCR5380_region_size, "ncr5380");
 #else
-	if(check_mem_region(overrides[current_override].NCR5380_map_name,
-		NCR5380_region_size))
+	if(!request_mem_region(overrides[current_override].NCR5380_map_name,
+		NCR5380_region_size,  "ncr5380"))
 		continue;
-	request_mem_region(overrides[current_override].NCR5380_map_name,
-					NCR5380_region_size, "ncr5380");
+
+	isa_remap_ptr = ioremap(overrides[current_override].NCR5380_map_name +
+				NCR53C400_mem_base, NCR53C400_register_offset);
+	if (!isa_remap_ptr)
+	    goto err_release_mem;
 #endif
 	instance = scsi_register (tpnt, sizeof(struct NCR5380_hostdata));
 	if(instance == NULL)
-	{
-#ifdef CONFIG_SCSI_G_NCR5380_PORT
-		release_region(overrides[current_override].NCR5380_map_name,
-	                                        NCR5380_region_size);
-#else
-		release_mem_region(overrides[current_override].NCR5380_map_name,
-	                                  	NCR5380_region_size);
-#endif
-	}
+	    goto err_release;
 	
 	instance->NCR5380_instance_name = overrides[current_override].NCR5380_map_name;
 
@@ -434,6 +431,19 @@
 
 	++current_override;
 	++count;
+	continue;
+
+    err_release:
+#ifdef CONFIG_SCSI_G_NCR5380_PORT
+	release_region(overrides[current_override].NCR5380_map_name,
+		       NCR5380_region_size);
+#else
+	iounmap(isa_remap_ptr);
+    err_release_mem:
+	release_mem_region(overrides[current_override].NCR5380_map_name,
+			   NCR5380_region_size);
+#endif
+
     }
     return count;
 }
@@ -453,6 +463,7 @@
     release_region(instance->NCR5380_instance_name, NCR5380_region_size);
 #else
     release_mem_region(instance->NCR5380_instance_name, NCR5380_region_size);
+    iounmap(isa_remap_ptr);
 #endif    
 
     if (instance->irq != IRQ_NONE)
@@ -550,7 +561,7 @@
 	    dst[start+i] = NCR5380_read(C400_HOST_BUFFER);
 #else
 	/* implies CONFIG_SCSI_G_NCR5380_MEM */
-	isa_memcpy_fromio(dst+start,NCR53C400_host_buffer+NCR5380_map_name,128);
+	memcpy_fromio(dst+start, isa_remap_ptr+OFFSET_FROM_REMAPPING, 128);
 #endif
 	start+=128;
 	blocks--;
@@ -571,7 +582,7 @@
 	    dst[start+i] = NCR5380_read(C400_HOST_BUFFER);
 #else
 	/* implies CONFIG_SCSI_G_NCR5380_MEM */
-	isa_memcpy_fromio(dst+start,NCR53C400_host_buffer+NCR5380_map_name,128);
+	memcpy_fromio(dst+start, isa_remap_ptr+OFFSET_FROM_REMAPPING, 128);
 #endif
 	start+=128;
 	blocks--;
@@ -658,7 +669,7 @@
 	    NCR5380_write(C400_HOST_BUFFER, src[start+i]);
 #else
 	/* implies CONFIG_SCSI_G_NCR5380_MEM */
-	isa_memcpy_toio(NCR53C400_host_buffer+NCR5380_map_name,src+start,128);
+	memcpy_toio(isa_remap_ptr+OFFSET_FROM_REMAPPING, src+start, 128);
 #endif
 	start+=128;
 	blocks--;
@@ -678,7 +689,7 @@
 	    NCR5380_write(C400_HOST_BUFFER, src[start+i]);
 #else
 	/* implies CONFIG_SCSI_G_NCR5380_MEM */
-	isa_memcpy_toio(NCR53C400_host_buffer+NCR5380_map_name,src+start,128);
+	memcpy_toio(isa_remap_ptr+OFFSET_FROM_REMAPPING, src+start, 128);
 #endif
 	start+=128;
 	blocks--;
diff -aur linux-242-ac3-clean/drivers/scsi/g_NCR5380.h linux-242-ac3/drivers/scsi/g_NCR5380.h
--- linux-242-ac3-clean/drivers/scsi/g_NCR5380.h	Sun Feb 11 12:04:58 2001
+++ linux-242-ac3/drivers/scsi/g_NCR5380.h	Sun Feb 25 14:33:30 2001
@@ -133,11 +133,12 @@
 
 #define NCR53C400_host_buffer 0x3900
 
-#define NCR5380_region_size 0x3a00
+#define OFFSET_FROM_REMAPPING (NCR53C400_host_buffer - NCR53C400_mem_base)
 
+#define NCR5380_region_size 0x3a00
 
-#define NCR5380_read(reg) isa_readb(NCR5380_map_name + NCR53C400_mem_base + (reg))
-#define NCR5380_write(reg, value) isa_writeb(NCR5380_map_name + NCR53C400_mem_base + (reg), value)
+#define NCR5380_read(reg) readb(isa_remap_ptr+(reg))
+#define NCR5380_write(reg, value) writeb(value, isa_remap_ptr+(reg))
 
 #endif
 

-- 
Regards,
        Rasmus(rasmus@jaquet.dk)

With Microsoft products, failure is not an option - it's a standard component. 
  -- Anonymous

Re: [PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Alan Cox]

> (Does anybody know who the maintainer for this code is?)

There isnt one

> (An indication of how often this code path is used can be found in
> the fact that the previous define of NCR5380_write had its payload
> and address mixed up, probably making for wierd results should
> the code ever be executed.)

The driver works for me nicely. Im not convinced by the changes of direction
either. At least not without a detailed audit on the 2.2 code. Some of the
naming is very misleading in that driver

Re: [PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Rasmus Andersen]

On Sun, Feb 25, 2001 at 02:05:42PM +0000, Alan Cox wrote:
[...]
> > (An indication of how often this code path is used can be found in
> > the fact that the previous define of NCR5380_write had its payload
> > and address mixed up, probably making for wierd results should
> > the code ever be executed.)
> 
> The driver works for me nicely. Im not convinced by the changes of direction
> either. At least not without a detailed audit on the 2.2 code. Some of the
> naming is very misleading in that driver
> 

Looking at the define of NCR_5380_write

#define NCR5380_write(reg, value) isa_writeb(NCR5380_map_name + +NCR53C400_mem_base + (reg), value)

followed by an use of NCR5380_write

    NCR5380_write(C400_CONTROL_STATUS_REG, CSR_BASE | CSR_TRANS_DIR);

I doubt that it is not the intention to write CSR_BASE | CSR_TRANS_DIR
at the offset C400_CONTROL_STATUS_REG. But note that this argument
swap only is in the code produced by -DCONFIG_SCSI_G_NCR5380_MEM.
Perhaps you use CONFIG_SCSI_G_NCR5380_PORT? Otherwise I must admit
that I have been had...
-- 
Regards,
        Rasmus(rasmus@jaquet.dk)

Duct tape is like the force; it has a light side and a dark side, and
it holds the universe together.
  -- Anonymous

Re: [PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Alan Cox]

> Looking at the define of NCR_5380_write
> 
> #define NCR5380_write(reg, value) isa_writeb(NCR5380_map_name + +NCR53C400_mem_base + (reg), value)
> 
> followed by an use of NCR5380_write
> 
>     NCR5380_write(C400_CONTROL_STATUS_REG, CSR_BASE | CSR_TRANS_DIR);
> 
> I doubt that it is not the intention to write CSR_BASE | CSR_TRANS_DIR
> at the offset C400_CONTROL_STATUS_REG. But note that this argument
> swap only is in the code produced by -DCONFIG_SCSI_G_NCR5380_MEM.
> Perhaps you use CONFIG_SCSI_G_NCR5380_PORT? Otherwise I must admit
> that I have been had...

Im running PIO. However while I agree with that one Im dubious about the
inverts on the memcpy_*io bits

Alan

Re: [PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Rasmus Andersen]

> 
> Im running PIO. However while I agree with that one Im dubious about the
> inverts on the memcpy_*io bits
> 

I am sorry but have I inverted the arguments to the memcpy_*io calls?
Or are you referring to something other than the arguments here?
-- 
        Rasmus(rasmus@jaquet.dk)

"Science is like sex: sometimes something useful comes out, but that 
is not the reason we are doing it" -- Richard Feynman 

Re: [PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Alan Cox]

> I am sorry but have I inverted the arguments to the memcpy_*io calls?
> Or are you referring to something other than the arguments here?

You seem to have swapped the source/dest over in memcpy_toio cases and I need
to convince myself you did that correctly

Re: [PATCH] s/isa//g in drivers/scsi/g_NCR5380.c and some cleanup (242)

[Rasmus Andersen]

On Sun, Feb 25, 2001 at 02:46:15PM +0000, Alan Cox wrote:
> > I am sorry but have I inverted the arguments to the memcpy_*io calls?
> > Or are you referring to something other than the arguments here?
> 
> You seem to have swapped the source/dest over in memcpy_toio cases and I need
> to convince myself you did that correctly

Yes, that is neither obvious nor nice. My apologies, but I could not
find a better way.

Explanation: The memcpy_toio cases goes like this:

-       isa_memcpy_toio(NCR53C400_host_buffer+NCR5380_map_name,src+start,128);
+       memcpy_toio(isa_remap_ptr+OFFSET_FROM_REMAPPING, src+start, 128);

isa_remap_ptr is the ioremap from NCR5380_map_name + NCR53C400_mem_base.
I would like to memcpy from NCR53C400_host_buffer+NCR5380_map_name thus
needing to add the difference between NCR53C400_host_buffer and the
NCR53C400_mem_base (used in isa_remap_ptr). Thus, in the hope that
this can be done linearly, I add OFFSET_FROM_REMAPPING 
(NCR53C400_host_buffer - NCR53C400_mem_base). (BTW, this is also done
in the memcpy_fromio cases.)

I hope that the above is readable.
-- 
Regards,
        Rasmus(rasmus@jaquet.dk)