[PATCH 0/3] g_NCR5380: Modernization

[PATCH 0/3] g_NCR5380: Modernization

[Ondrej Zary]

This small patch series removes deprecated code from g_NCR5380 driver
and converts it from scsi_module.c to scsi_add_host().

Tested with:
HP C2502 (53C400A chip)
Canon FG2-5202 (53C400 chip, memory-mapped)
DTC-3181L (DTCT-436P chip, PnP)

P.S. I don't like the ifdef mess needed for g_NCR5380_mmio. Should I try to
merge the code so g_NCR5380 will support both PIO and MMIO cards (and
g_NCR5380_mmio could be removed)?

-- 
Ondrej Zary

[PATCH 1/3] g_NCR5380: Remove deprecated __setup

[Ondrej Zary]

Remove deprecated __setup for parsing command line parameters.
g_NCR5380.* parameters could be used instead.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |  135 ----------------------------------------------
 1 file changed, 135 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 516bd6c..7e50b44e 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -56,136 +56,6 @@ static struct override {
 
 #define NO_OVERRIDES ARRAY_SIZE(overrides)
 
-#ifndef MODULE
-
-/**
- *	internal_setup		-	handle lilo command string override
- *	@board:	BOARD_* identifier for the board
- *	@str: unused
- *	@ints: numeric parameters
- *
- * 	Do LILO command line initialization of the overrides array. Display
- *	errors when needed
- *
- *	Locks: none
- */
-
-static void __init internal_setup(int board, char *str, int *ints)
-{
-	static int commandline_current;
-	switch (board) {
-	case BOARD_NCR5380:
-		if (ints[0] != 2 && ints[0] != 3) {
-			printk(KERN_ERR "generic_NCR5380_setup : usage ncr5380=" STRVAL(NCR5380_map_name) ",irq,dma\n");
-			return;
-		}
-		break;
-	case BOARD_NCR53C400:
-		if (ints[0] != 2) {
-			printk(KERN_ERR "generic_NCR53C400_setup : usage ncr53c400=" STRVAL(NCR5380_map_name) ",irq\n");
-			return;
-		}
-		break;
-	case BOARD_NCR53C400A:
-		if (ints[0] != 2) {
-			printk(KERN_ERR "generic_NCR53C400A_setup : usage ncr53c400a=" STRVAL(NCR5380_map_name) ",irq\n");
-			return;
-		}
-		break;
-	case BOARD_DTC3181E:
-		if (ints[0] != 2) {
-			printk("generic_DTC3181E_setup : usage dtc3181e=" STRVAL(NCR5380_map_name) ",irq\n");
-			return;
-		}
-		break;
-	}
-
-	if (commandline_current < NO_OVERRIDES) {
-		overrides[commandline_current].NCR5380_map_name = (NCR5380_map_type) ints[1];
-		overrides[commandline_current].irq = ints[2];
-		if (ints[0] == 3)
-			overrides[commandline_current].dma = ints[3];
-		else
-			overrides[commandline_current].dma = DMA_NONE;
-		overrides[commandline_current].board = board;
-		++commandline_current;
-	}
-}
-
-
-/**
- * 	do_NCR53C80_setup		-	set up entry point
- *	@str: unused
- *
- *	Setup function invoked at boot to parse the ncr5380= command
- *	line.
- */
-
-static int __init do_NCR5380_setup(char *str)
-{
-	int ints[10];
-
-	get_options(str, ARRAY_SIZE(ints), ints);
-	internal_setup(BOARD_NCR5380, str, ints);
-	return 1;
-}
-
-/**
- * 	do_NCR53C400_setup		-	set up entry point
- *	@str: unused
- *	@ints: integer parameters from kernel setup code
- *
- *	Setup function invoked at boot to parse the ncr53c400= command
- *	line.
- */
-
-static int __init do_NCR53C400_setup(char *str)
-{
-	int ints[10];
-
-	get_options(str, ARRAY_SIZE(ints), ints);
-	internal_setup(BOARD_NCR53C400, str, ints);
-	return 1;
-}
-
-/**
- * 	do_NCR53C400A_setup	-	set up entry point
- *	@str: unused
- *	@ints: integer parameters from kernel setup code
- *
- *	Setup function invoked at boot to parse the ncr53c400a= command
- *	line.
- */
-
-static int __init do_NCR53C400A_setup(char *str)
-{
-	int ints[10];
-
-	get_options(str, ARRAY_SIZE(ints), ints);
-	internal_setup(BOARD_NCR53C400A, str, ints);
-	return 1;
-}
-
-/**
- * 	do_DTC3181E_setup	-	set up entry point
- *	@str: unused
- *	@ints: integer parameters from kernel setup code
- *
- *	Setup function invoked at boot to parse the dtc3181e= command
- *	line.
- */
-
-static int __init do_DTC3181E_setup(char *str)
-{
-	int ints[10];
-
-	get_options(str, ARRAY_SIZE(ints), ints);
-	internal_setup(BOARD_DTC3181E, str, ints);
-	return 1;
-}
-
-#endif
-
 #ifndef SCSI_G_NCR5380_MEM
 /*
  * Configure I/O address of 53C400A or DTC436 by writing magic numbers
@@ -741,8 +611,3 @@ static struct isapnp_device_id id_table[] = {
 
 MODULE_DEVICE_TABLE(isapnp, id_table);
 #endif
-
-__setup("ncr5380=", do_NCR5380_setup);
-__setup("ncr53c400=", do_NCR53C400_setup);
-__setup("ncr53c400a=", do_NCR53C400A_setup);
-__setup("dtc3181e=", do_DTC3181E_setup);
-- 
Ondrej Zary

Re: [PATCH 1/3] g_NCR5380: Remove deprecated __setup

[Christoph Hellwig]

On Sat, Sep 24, 2016 at 08:59:03PM +0200, Ondrej Zary wrote:
> Remove deprecated __setup for parsing command line parameters.
> g_NCR5380.* parameters could be used instead.

This assumes everyone has moved over to it.  But I suspect the remaining
hand full of users could adjust..

Re: [PATCH 1/3] g_NCR5380: Remove deprecated __setup

[Finn Thain]

On Sat, 24 Sep 2016, Ondrej Zary wrote:

> Remove deprecated __setup for parsing command line parameters. 
> g_NCR5380.* parameters could be used instead.

Removing useful features is deprecated too.

The best reason I can see for removing the __setup parameters is that the 
g_NCR5380 module parameters clash with those of the g_NCR5380_mmio module 
(when both are built-in). But only a custom kernel would configure them as 
built-in drivers, and it probably would not have both drivers enabled. So 
a clash seems unlikely.

Anyway, Christoph seems to be OK with this so I won't object. But if you 
assume that no-one has built-in g_NCR5380{,_mmio} drivers, or you believe 
that the benefit outweighs the inconvenience of the breakage caused, then 
the commit log should explain that.

-- 

> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/g_NCR5380.c |  135 ----------------------------------------------
>  1 file changed, 135 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 516bd6c..7e50b44e 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -56,136 +56,6 @@ static struct override {
>  
>  #define NO_OVERRIDES ARRAY_SIZE(overrides)
>  
> -#ifndef MODULE
> -
> -/**
> - *	internal_setup		-	handle lilo command string override
> - *	@board:	BOARD_* identifier for the board
> - *	@str: unused
> - *	@ints: numeric parameters
> - *
> - * 	Do LILO command line initialization of the overrides array. Display
> - *	errors when needed
> - *
> - *	Locks: none
> - */
> -
> -static void __init internal_setup(int board, char *str, int *ints)
> -{
> -	static int commandline_current;
> -	switch (board) {
> -	case BOARD_NCR5380:
> -		if (ints[0] != 2 && ints[0] != 3) {
> -			printk(KERN_ERR "generic_NCR5380_setup : usage ncr5380=" STRVAL(NCR5380_map_name) ",irq,dma\n");
> -			return;
> -		}
> -		break;
> -	case BOARD_NCR53C400:
> -		if (ints[0] != 2) {
> -			printk(KERN_ERR "generic_NCR53C400_setup : usage ncr53c400=" STRVAL(NCR5380_map_name) ",irq\n");
> -			return;
> -		}
> -		break;
> -	case BOARD_NCR53C400A:
> -		if (ints[0] != 2) {
> -			printk(KERN_ERR "generic_NCR53C400A_setup : usage ncr53c400a=" STRVAL(NCR5380_map_name) ",irq\n");
> -			return;
> -		}
> -		break;
> -	case BOARD_DTC3181E:
> -		if (ints[0] != 2) {
> -			printk("generic_DTC3181E_setup : usage dtc3181e=" STRVAL(NCR5380_map_name) ",irq\n");
> -			return;
> -		}
> -		break;
> -	}
> -
> -	if (commandline_current < NO_OVERRIDES) {
> -		overrides[commandline_current].NCR5380_map_name = (NCR5380_map_type) ints[1];
> -		overrides[commandline_current].irq = ints[2];
> -		if (ints[0] == 3)
> -			overrides[commandline_current].dma = ints[3];
> -		else
> -			overrides[commandline_current].dma = DMA_NONE;
> -		overrides[commandline_current].board = board;
> -		++commandline_current;
> -	}
> -}
> -
> -
> -/**
> - * 	do_NCR53C80_setup		-	set up entry point
> - *	@str: unused
> - *
> - *	Setup function invoked at boot to parse the ncr5380= command
> - *	line.
> - */
> -
> -static int __init do_NCR5380_setup(char *str)
> -{
> -	int ints[10];
> -
> -	get_options(str, ARRAY_SIZE(ints), ints);
> -	internal_setup(BOARD_NCR5380, str, ints);
> -	return 1;
> -}
> -
> -/**
> - * 	do_NCR53C400_setup		-	set up entry point
> - *	@str: unused
> - *	@ints: integer parameters from kernel setup code
> - *
> - *	Setup function invoked at boot to parse the ncr53c400= command
> - *	line.
> - */
> -
> -static int __init do_NCR53C400_setup(char *str)
> -{
> -	int ints[10];
> -
> -	get_options(str, ARRAY_SIZE(ints), ints);
> -	internal_setup(BOARD_NCR53C400, str, ints);
> -	return 1;
> -}
> -
> -/**
> - * 	do_NCR53C400A_setup	-	set up entry point
> - *	@str: unused
> - *	@ints: integer parameters from kernel setup code
> - *
> - *	Setup function invoked at boot to parse the ncr53c400a= command
> - *	line.
> - */
> -
> -static int __init do_NCR53C400A_setup(char *str)
> -{
> -	int ints[10];
> -
> -	get_options(str, ARRAY_SIZE(ints), ints);
> -	internal_setup(BOARD_NCR53C400A, str, ints);
> -	return 1;
> -}
> -
> -/**
> - * 	do_DTC3181E_setup	-	set up entry point
> - *	@str: unused
> - *	@ints: integer parameters from kernel setup code
> - *
> - *	Setup function invoked at boot to parse the dtc3181e= command
> - *	line.
> - */
> -
> -static int __init do_DTC3181E_setup(char *str)
> -{
> -	int ints[10];
> -
> -	get_options(str, ARRAY_SIZE(ints), ints);
> -	internal_setup(BOARD_DTC3181E, str, ints);
> -	return 1;
> -}
> -
> -#endif
> -
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
>   * Configure I/O address of 53C400A or DTC436 by writing magic numbers
> @@ -741,8 +611,3 @@ static struct isapnp_device_id id_table[] = {
>  
>  MODULE_DEVICE_TABLE(isapnp, id_table);
>  #endif
> -
> -__setup("ncr5380=", do_NCR5380_setup);
> -__setup("ncr53c400=", do_NCR53C400_setup);
> -__setup("ncr53c400a=", do_NCR53C400A_setup);
> -__setup("dtc3181e=", do_DTC3181E_setup);
> 

[PATCH 2/3] g_NCR5380: Reduce overrides[] from array to struct

[Ondrej Zary]

Remove compile-time card type definition GENERIC_NCR5380_OVERRIDE.
Then remove all code iterating the overrides[] array and reduce it to
struct card.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |  351 ++++++++++++++++++++++------------------------
 1 file changed, 167 insertions(+), 184 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 7e50b44e..5162de6 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -42,19 +42,12 @@ static int ncr_53c400a;
 static int dtc_3181e;
 static int hp_c2502;
 
-static struct override {
+static struct card {
 	NCR5380_map_type NCR5380_map_name;
 	int irq;
 	int dma;
 	int board;		/* Use NCR53c400, Ricoh, etc. extensions ? */
-} overrides
-#ifdef GENERIC_NCR5380_OVERRIDE
-[] __initdata = GENERIC_NCR5380_OVERRIDE;
-#else
-[1] __initdata = { { 0,},};
-#endif
-
-#define NO_OVERRIDES ARRAY_SIZE(overrides)
+} card;
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -85,16 +78,13 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
  *	@tpnt: the scsi template
  *
  *	Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- *	and DTC436(ISAPnP) controllers. If overrides have been set we use
- *	them.
+ *	and DTC436(ISAPnP) controllers.
  *
  *	Locks: none
  */
 
 static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 {
-	static int current_override;
-	int count;
 	unsigned int *ports;
 	u8 *magic = NULL;
 #ifndef SCSI_G_NCR5380_MEM
@@ -124,28 +114,25 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 #endif
 
 	if (ncr_irq)
-		overrides[0].irq = ncr_irq;
+		card.irq = ncr_irq;
 	if (ncr_dma)
-		overrides[0].dma = ncr_dma;
+		card.dma = ncr_dma;
 	if (ncr_addr)
-		overrides[0].NCR5380_map_name = (NCR5380_map_type) ncr_addr;
+		card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
 	if (ncr_5380)
-		overrides[0].board = BOARD_NCR5380;
+		card.board = BOARD_NCR5380;
 	else if (ncr_53c400)
-		overrides[0].board = BOARD_NCR53C400;
+		card.board = BOARD_NCR53C400;
 	else if (ncr_53c400a)
-		overrides[0].board = BOARD_NCR53C400A;
+		card.board = BOARD_NCR53C400A;
 	else if (dtc_3181e)
-		overrides[0].board = BOARD_DTC3181E;
+		card.board = BOARD_DTC3181E;
 	else if (hp_c2502)
-		overrides[0].board = BOARD_HP_C2502;
+		card.board = BOARD_HP_C2502;
 #ifndef SCSI_G_NCR5380_MEM
-	if (!current_override && isapnp_present()) {
+	if (isapnp_present()) {
 		struct pnp_dev *dev = NULL;
-		count = 0;
 		while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) {
-			if (count >= NO_OVERRIDES)
-				break;
 			if (pnp_device_attach(dev) < 0)
 				continue;
 			if (pnp_activate_dev(dev) < 0) {
@@ -159,202 +146,198 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 				continue;
 			}
 			if (pnp_irq_valid(dev, 0))
-				overrides[count].irq = pnp_irq(dev, 0);
+				card.irq = pnp_irq(dev, 0);
 			else
-				overrides[count].irq = NO_IRQ;
+				card.irq = NO_IRQ;
 			if (pnp_dma_valid(dev, 0))
-				overrides[count].dma = pnp_dma(dev, 0);
+				card.dma = pnp_dma(dev, 0);
 			else
-				overrides[count].dma = DMA_NONE;
-			overrides[count].NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0);
-			overrides[count].board = BOARD_DTC3181E;
-			count++;
+				card.dma = DMA_NONE;
+			card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0);
+			card.board = BOARD_DTC3181E;
+			break;
 		}
 	}
 #endif
 
-	for (count = 0; current_override < NO_OVERRIDES; ++current_override) {
-		if (!(overrides[current_override].NCR5380_map_name))
-			continue;
+	if (!(card.NCR5380_map_name))
+		return 0;
 
-		ports = NULL;
-		flags = 0;
-		switch (overrides[current_override].board) {
-		case BOARD_NCR5380:
-			flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
-			break;
-		case BOARD_NCR53C400A:
-			ports = ncr_53c400a_ports;
-			magic = ncr_53c400a_magic;
-			break;
-		case BOARD_HP_C2502:
-			ports = ncr_53c400a_ports;
-			magic = hp_c2502_magic;
-			break;
-		case BOARD_DTC3181E:
-			ports = dtc_3181e_ports;
-			magic = ncr_53c400a_magic;
-			break;
-		}
+	ports = NULL;
+	flags = 0;
+	switch (card.board) {
+	case BOARD_NCR5380:
+		flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
+		break;
+	case BOARD_NCR53C400A:
+		ports = ncr_53c400a_ports;
+		magic = ncr_53c400a_magic;
+		break;
+	case BOARD_HP_C2502:
+		ports = ncr_53c400a_ports;
+		magic = hp_c2502_magic;
+		break;
+	case BOARD_DTC3181E:
+		ports = dtc_3181e_ports;
+		magic = ncr_53c400a_magic;
+		break;
+	}
 
 #ifndef SCSI_G_NCR5380_MEM
-		if (ports && magic) {
-			/* wakeup sequence for the NCR53C400A and DTC3181E */
-
-			/* Disable the adapter and look for a free io port */
-			magic_configure(-1, 0, magic);
-
-			region_size = 16;
-
-			if (overrides[current_override].NCR5380_map_name != PORT_AUTO)
-				for (i = 0; ports[i]; i++) {
-					if (!request_region(ports[i], region_size, "ncr53c80"))
-						continue;
-					if (overrides[current_override].NCR5380_map_name == ports[i])
-						break;
-					release_region(ports[i], region_size);
-			} else
-				for (i = 0; ports[i]; i++) {
-					if (!request_region(ports[i], region_size, "ncr53c80"))
-						continue;
-					if (inb(ports[i]) == 0xff)
-						break;
-					release_region(ports[i], region_size);
-				}
-			if (ports[i]) {
-				/* At this point we have our region reserved */
-				magic_configure(i, 0, magic); /* no IRQ yet */
-				outb(0xc0, ports[i] + 9);
-				if (inb(ports[i] + 9) != 0x80)
+	if (ports && magic) {
+		/* wakeup sequence for the NCR53C400A and DTC3181E */
+
+		/* Disable the adapter and look for a free io port */
+		magic_configure(-1, 0, magic);
+
+		region_size = 16;
+
+		if (card.NCR5380_map_name != PORT_AUTO)
+			for (i = 0; ports[i]; i++) {
+				if (!request_region(ports[i], region_size, "ncr53c80"))
 					continue;
-				overrides[current_override].NCR5380_map_name = ports[i];
-				port_idx = i;
-			} else
-				continue;
-		}
-		else
-		{
-			/* Not a 53C400A style setup - just grab */
-			region_size = 8;
-			if (!request_region(overrides[current_override].NCR5380_map_name,
-			                    region_size, "ncr5380"))
-				continue;
-		}
+				if (card.NCR5380_map_name == ports[i])
+					break;
+				release_region(ports[i], region_size);
+		} else
+			for (i = 0; ports[i]; i++) {
+				if (!request_region(ports[i], region_size, "ncr53c80"))
+					continue;
+				if (inb(ports[i]) == 0xff)
+					break;
+				release_region(ports[i], region_size);
+			}
+		if (ports[i]) {
+			/* At this point we have our region reserved */
+			magic_configure(i, 0, magic); /* no IRQ yet */
+			outb(0xc0, ports[i] + 9);
+			if (inb(ports[i] + 9) != 0x80)
+				return 0;
+			card.NCR5380_map_name = ports[i];
+			port_idx = i;
+		} else
+			return 0;
+	}
+	else
+	{
+		/* Not a 53C400A style setup - just grab */
+		region_size = 8;
+		if (!request_region(card.NCR5380_map_name,
+		                    region_size, "ncr5380"))
+			return 0;
+	}
 #else
-		base = overrides[current_override].NCR5380_map_name;
-		iomem_size = NCR53C400_region_size;
-		if (!request_mem_region(base, iomem_size, "ncr5380"))
-			continue;
-		iomem = ioremap(base, iomem_size);
-		if (!iomem) {
-			release_mem_region(base, iomem_size);
-			continue;
-		}
+	base = card.NCR5380_map_name;
+	iomem_size = NCR53C400_region_size;
+	if (!request_mem_region(base, iomem_size, "ncr5380"))
+		return 0;
+	iomem = ioremap(base, iomem_size);
+	if (!iomem) {
+		release_mem_region(base, iomem_size);
+		return 0;
+	}
 #endif
-		instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
-		if (instance == NULL)
-			goto out_release;
-		hostdata = shost_priv(instance);
+	instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
+	if (instance == NULL)
+		goto out_release;
+	hostdata = shost_priv(instance);
 
 #ifndef SCSI_G_NCR5380_MEM
-		instance->io_port = overrides[current_override].NCR5380_map_name;
-		instance->n_io_port = region_size;
-		hostdata->io_width = 1; /* 8-bit PDMA by default */
-
-		/*
-		 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
-		 * the base address.
-		 */
-		switch (overrides[current_override].board) {
-		case BOARD_NCR53C400:
-			instance->io_port += 8;
-			hostdata->c400_ctl_status = 0;
-			hostdata->c400_blk_cnt = 1;
-			hostdata->c400_host_buf = 4;
-			break;
-		case BOARD_DTC3181E:
-			hostdata->io_width = 2;	/* 16-bit PDMA */
-			/* fall through */
-		case BOARD_NCR53C400A:
-		case BOARD_HP_C2502:
-			hostdata->c400_ctl_status = 9;
-			hostdata->c400_blk_cnt = 10;
-			hostdata->c400_host_buf = 8;
-			break;
-		}
+	instance->io_port = card.NCR5380_map_name;
+	instance->n_io_port = region_size;
+	hostdata->io_width = 1; /* 8-bit PDMA by default */
+
+	/*
+	 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
+	 * the base address.
+	 */
+	switch (card.board) {
+	case BOARD_NCR53C400:
+		instance->io_port += 8;
+		hostdata->c400_ctl_status = 0;
+		hostdata->c400_blk_cnt = 1;
+		hostdata->c400_host_buf = 4;
+		break;
+	case BOARD_DTC3181E:
+		hostdata->io_width = 2;	/* 16-bit PDMA */
+		/* fall through */
+	case BOARD_NCR53C400A:
+	case BOARD_HP_C2502:
+		hostdata->c400_ctl_status = 9;
+		hostdata->c400_blk_cnt = 10;
+		hostdata->c400_host_buf = 8;
+		break;
+	}
 #else
-		instance->base = overrides[current_override].NCR5380_map_name;
-		hostdata->iomem = iomem;
-		hostdata->iomem_size = iomem_size;
-		switch (overrides[current_override].board) {
-		case BOARD_NCR53C400:
-			hostdata->c400_ctl_status = 0x100;
-			hostdata->c400_blk_cnt = 0x101;
-			hostdata->c400_host_buf = 0x104;
-			break;
-		case BOARD_DTC3181E:
-		case BOARD_NCR53C400A:
-		case BOARD_HP_C2502:
-			pr_err(DRV_MODULE_NAME ": unknown register offsets\n");
-			goto out_unregister;
-		}
+	instance->base = card.NCR5380_map_name;
+	hostdata->iomem = iomem;
+	hostdata->iomem_size = iomem_size;
+	switch (card.board) {
+	case BOARD_NCR53C400:
+		hostdata->c400_ctl_status = 0x100;
+		hostdata->c400_blk_cnt = 0x101;
+		hostdata->c400_host_buf = 0x104;
+		break;
+	case BOARD_DTC3181E:
+	case BOARD_NCR53C400A:
+	case BOARD_HP_C2502:
+		pr_err(DRV_MODULE_NAME ": unknown register offsets\n");
+		goto out_unregister;
+	}
 #endif
 
-		if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
-			goto out_unregister;
+	if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
+		goto out_unregister;
 
-		switch (overrides[current_override].board) {
-		case BOARD_NCR53C400:
-		case BOARD_DTC3181E:
-		case BOARD_NCR53C400A:
-		case BOARD_HP_C2502:
-			NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
-		}
+	switch (card.board) {
+	case BOARD_NCR53C400:
+	case BOARD_DTC3181E:
+	case BOARD_NCR53C400A:
+	case BOARD_HP_C2502:
+		NCR5380_write(hostdata->c400_ctl_status, CSR_BASE);
+	}
 
-		NCR5380_maybe_reset_bus(instance);
+	NCR5380_maybe_reset_bus(instance);
 
-		if (overrides[current_override].irq != IRQ_AUTO)
-			instance->irq = overrides[current_override].irq;
-		else
-			instance->irq = NCR5380_probe_irq(instance, 0xffff);
+	if (card.irq != IRQ_AUTO)
+		instance->irq = card.irq;
+	else
+		instance->irq = NCR5380_probe_irq(instance, 0xffff);
 
-		/* Compatibility with documented NCR5380 kernel parameters */
-		if (instance->irq == 255)
-			instance->irq = NO_IRQ;
+	/* Compatibility with documented NCR5380 kernel parameters */
+	if (instance->irq == 255)
+		instance->irq = NO_IRQ;
 
-		if (instance->irq != NO_IRQ) {
+	if (instance->irq != NO_IRQ) {
 #ifndef SCSI_G_NCR5380_MEM
-			/* set IRQ for HP C2502 */
-			if (overrides[current_override].board == BOARD_HP_C2502)
-				magic_configure(port_idx, instance->irq, magic);
+		/* set IRQ for HP C2502 */
+		if (card.board == BOARD_HP_C2502)
+			magic_configure(port_idx, instance->irq, magic);
 #endif
-			if (request_irq(instance->irq, generic_NCR5380_intr,
-					0, "NCR5380", instance)) {
-				printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n", instance->host_no, instance->irq);
-				instance->irq = NO_IRQ;
-			}
-		}
-
-		if (instance->irq == NO_IRQ) {
-			printk(KERN_INFO "scsi%d : interrupts not enabled. for better interactive performance,\n", instance->host_no);
-			printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no);
+		if (request_irq(instance->irq, generic_NCR5380_intr,
+				0, "NCR5380", instance)) {
+			printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n", instance->host_no, instance->irq);
+			instance->irq = NO_IRQ;
 		}
+	}
 
-		++current_override;
-		++count;
+	if (instance->irq == NO_IRQ) {
+		printk(KERN_INFO "scsi%d : interrupts not enabled. for better interactive performance,\n", instance->host_no);
+		printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no);
 	}
-	return count;
+
+	return 1;
 
 out_unregister:
 	scsi_unregister(instance);
 out_release:
 #ifndef SCSI_G_NCR5380_MEM
-	release_region(overrides[current_override].NCR5380_map_name, region_size);
+	release_region(card.NCR5380_map_name, region_size);
 #else
 	iounmap(iomem);
 	release_mem_region(base, iomem_size);
 #endif
-	return count;
+	return 0;
 }
 
 /**
-- 
Ondrej Zary

Re: [PATCH 2/3] g_NCR5380: Reduce overrides[] from array to struct

[Christoph Hellwig]

On Sat, Sep 24, 2016 at 08:59:04PM +0200, Ondrej Zary wrote:
> Remove compile-time card type definition GENERIC_NCR5380_OVERRIDE.
> Then remove all code iterating the overrides[] array and reduce it to
> struct card.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

Looks great!

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] g_NCR5380: Reduce overrides[] from array to struct

[Finn Thain]

On Sat, 24 Sep 2016, Ondrej Zary wrote:

> Remove compile-time card type definition GENERIC_NCR5380_OVERRIDE. Then 
> remove all code iterating the overrides[] array and reduce it to struct 
> card.

There is a reference to the compile-time override in 
Documentation/scsi/g_NCR5380.txt which needs to be removed also.

The patch looks fine aside from that.

-- 

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Ondrej Zary]

Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
Use pnp_driver and isa_driver to manage cards.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/g_NCR5380.c |  310 +++++++++++++++++++++++++---------------------
 drivers/scsi/g_NCR5380.h |    8 --
 2 files changed, 169 insertions(+), 149 deletions(-)

diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 5162de6..6cf6b00 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -30,24 +30,25 @@
 #include "NCR5380.h"
 #include <linux/init.h>
 #include <linux/ioport.h>
-#include <linux/isapnp.h>
+#include <linux/isa.h>
+#include <linux/pnp.h>
 #include <linux/interrupt.h>
 
-static int ncr_irq;
-static int ncr_dma;
-static int ncr_addr;
-static int ncr_5380;
-static int ncr_53c400;
-static int ncr_53c400a;
-static int dtc_3181e;
-static int hp_c2502;
-
-static struct card {
-	NCR5380_map_type NCR5380_map_name;
-	int irq;
-	int dma;
-	int board;		/* Use NCR53c400, Ricoh, etc. extensions ? */
-} card;
+#define MAX_CARDS 8
+
+static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(irq, int, NULL, 0);
+MODULE_PARM_DESC(irq, "IRQ number(s)");
+
+static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(base, int, NULL, 0);
+MODULE_PARM_DESC(base, "base address(es)");
+
+static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+module_param_array(card, int, NULL, 0);
+MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC3181E, 4=HP C2502)");
+
+MODULE_LICENSE("GPL");
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 }
 #endif
 
-/**
- * 	generic_NCR5380_detect	-	look for NCR5380 controllers
- *	@tpnt: the scsi template
- *
- *	Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- *	and DTC436(ISAPnP) controllers.
- *
- *	Locks: none
- */
-
-static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
+static struct Scsi_Host *generic_NCR5380_hw_init(
+			struct scsi_host_template *tpnt, struct device *pdev,
+			int base, int irq, int board)
 {
 	unsigned int *ports;
 	u8 *magic = NULL;
@@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	struct Scsi_Host *instance;
 	struct NCR5380_hostdata *hostdata;
 #ifdef SCSI_G_NCR5380_MEM
-	unsigned long base;
 	void __iomem *iomem;
 	resource_size_t iomem_size;
 #endif
 
-	if (ncr_irq)
-		card.irq = ncr_irq;
-	if (ncr_dma)
-		card.dma = ncr_dma;
-	if (ncr_addr)
-		card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
-	if (ncr_5380)
-		card.board = BOARD_NCR5380;
-	else if (ncr_53c400)
-		card.board = BOARD_NCR53C400;
-	else if (ncr_53c400a)
-		card.board = BOARD_NCR53C400A;
-	else if (dtc_3181e)
-		card.board = BOARD_DTC3181E;
-	else if (hp_c2502)
-		card.board = BOARD_HP_C2502;
-#ifndef SCSI_G_NCR5380_MEM
-	if (isapnp_present()) {
-		struct pnp_dev *dev = NULL;
-		while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) {
-			if (pnp_device_attach(dev) < 0)
-				continue;
-			if (pnp_activate_dev(dev) < 0) {
-				printk(KERN_ERR "dtc436e probe: activate failed\n");
-				pnp_device_detach(dev);
-				continue;
-			}
-			if (!pnp_port_valid(dev, 0)) {
-				printk(KERN_ERR "dtc436e probe: no valid port\n");
-				pnp_device_detach(dev);
-				continue;
-			}
-			if (pnp_irq_valid(dev, 0))
-				card.irq = pnp_irq(dev, 0);
-			else
-				card.irq = NO_IRQ;
-			if (pnp_dma_valid(dev, 0))
-				card.dma = pnp_dma(dev, 0);
-			else
-				card.dma = DMA_NONE;
-			card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0);
-			card.board = BOARD_DTC3181E;
-			break;
-		}
-	}
-#endif
-
-	if (!(card.NCR5380_map_name))
-		return 0;
-
 	ports = NULL;
 	flags = 0;
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR5380:
 		flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
 		break;
@@ -191,17 +133,20 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 		magic_configure(-1, 0, magic);
 
 		region_size = 16;
-
-		if (card.NCR5380_map_name != PORT_AUTO)
+		if (base)
 			for (i = 0; ports[i]; i++) {
-				if (!request_region(ports[i], region_size, "ncr53c80"))
-					continue;
-				if (card.NCR5380_map_name == ports[i])
+				if (base == ports[i]) {	/* index found */
+					if (!request_region(ports[i],
+							    region_size,
+							    "ncr53c80"))
+						return NULL;
 					break;
-				release_region(ports[i], region_size);
-		} else
+				}
+			}
+		else
 			for (i = 0; ports[i]; i++) {
-				if (!request_region(ports[i], region_size, "ncr53c80"))
+				if (!request_region(ports[i], region_size,
+						    "ncr53c80"))
 					continue;
 				if (inb(ports[i]) == 0xff)
 					break;
@@ -212,38 +157,36 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 			magic_configure(i, 0, magic); /* no IRQ yet */
 			outb(0xc0, ports[i] + 9);
 			if (inb(ports[i] + 9) != 0x80)
-				return 0;
-			card.NCR5380_map_name = ports[i];
+				goto out_release;
+			base = ports[i];
 			port_idx = i;
 		} else
-			return 0;
+			return NULL;
 	}
 	else
 	{
-		/* Not a 53C400A style setup - just grab */
+		/* NCR5380 - no configuration, just grab */
 		region_size = 8;
-		if (!request_region(card.NCR5380_map_name,
-		                    region_size, "ncr5380"))
-			return 0;
+		if (!base || !request_region(base, region_size, "ncr5380"))
+			return NULL;
 	}
 #else
-	base = card.NCR5380_map_name;
 	iomem_size = NCR53C400_region_size;
 	if (!request_mem_region(base, iomem_size, "ncr5380"))
-		return 0;
+		return NULL;
 	iomem = ioremap(base, iomem_size);
 	if (!iomem) {
 		release_mem_region(base, iomem_size);
-		return 0;
+		return NULL;
 	}
 #endif
-	instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
+	instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata));
 	if (instance == NULL)
 		goto out_release;
 	hostdata = shost_priv(instance);
 
 #ifndef SCSI_G_NCR5380_MEM
-	instance->io_port = card.NCR5380_map_name;
+	instance->io_port = base;
 	instance->n_io_port = region_size;
 	hostdata->io_width = 1; /* 8-bit PDMA by default */
 
@@ -251,7 +194,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
 	 * the base address.
 	 */
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR53C400:
 		instance->io_port += 8;
 		hostdata->c400_ctl_status = 0;
@@ -269,10 +212,10 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 		break;
 	}
 #else
-	instance->base = card.NCR5380_map_name;
+	instance->base = base;
 	hostdata->iomem = iomem;
 	hostdata->iomem_size = iomem_size;
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR53C400:
 		hostdata->c400_ctl_status = 0x100;
 		hostdata->c400_blk_cnt = 0x101;
@@ -289,7 +232,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
 		goto out_unregister;
 
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR53C400:
 	case BOARD_DTC3181E:
 	case BOARD_NCR53C400A:
@@ -299,8 +242,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 
 	NCR5380_maybe_reset_bus(instance);
 
-	if (card.irq != IRQ_AUTO)
-		instance->irq = card.irq;
+	if (irq != IRQ_AUTO)
+		instance->irq = irq;
 	else
 		instance->irq = NCR5380_probe_irq(instance, 0xffff);
 
@@ -311,7 +254,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	if (instance->irq != NO_IRQ) {
 #ifndef SCSI_G_NCR5380_MEM
 		/* set IRQ for HP C2502 */
-		if (card.board == BOARD_HP_C2502)
+		if (board == BOARD_HP_C2502)
 			magic_configure(port_idx, instance->irq, magic);
 #endif
 		if (request_irq(instance->irq, generic_NCR5380_intr,
@@ -326,31 +269,28 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 		printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no);
 	}
 
-	return 1;
+	if (scsi_add_host(instance, pdev)) {
+		free_irq(irq, instance);
+		goto out_unregister;
+	}
+	scsi_scan_host(instance);
+	return instance;
 
 out_unregister:
-	scsi_unregister(instance);
+	scsi_host_put(instance);
 out_release:
 #ifndef SCSI_G_NCR5380_MEM
-	release_region(card.NCR5380_map_name, region_size);
+	release_region(base, region_size);
 #else
 	iounmap(iomem);
 	release_mem_region(base, iomem_size);
 #endif
-	return 0;
+	return NULL;
 }
 
-/**
- *	generic_NCR5380_release_resources	-	free resources
- *	@instance: host adapter to clean up 
- *
- *	Free the generic interface resources from this adapter.
- *
- *	Locks: none
- */
- 
-static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
+static void generic_NCR5380_release_resources(struct Scsi_Host *instance)
 {
+	scsi_remove_host(instance);
 	if (instance->irq != NO_IRQ)
 		free_irq(instance->irq, instance);
 	NCR5380_exit(instance);
@@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
 		release_mem_region(instance->base, hostdata->iomem_size);
 	}
 #endif
-	return 0;
+	scsi_host_put(instance);
 }
 
 /**
@@ -554,10 +494,9 @@ static int generic_NCR5380_dma_xfer_len(struct Scsi_Host *instance,
 #include "NCR5380.c"
 
 static struct scsi_host_template driver_template = {
+	.module			= THIS_MODULE,
 	.proc_name		= DRV_MODULE_NAME,
 	.name			= "Generic NCR5380/NCR53C400 SCSI",
-	.detect			= generic_NCR5380_detect,
-	.release		= generic_NCR5380_release_resources,
 	.info			= generic_NCR5380_info,
 	.queuecommand		= generic_NCR5380_queue_command,
 	.eh_abort_handler	= generic_NCR5380_abort,
@@ -571,26 +510,115 @@ static struct scsi_host_template driver_template = {
 	.max_sectors		= 128,
 };
 
-#include "scsi_module.c"
 
-module_param(ncr_irq, int, 0);
-module_param(ncr_dma, int, 0);
-module_param(ncr_addr, int, 0);
-module_param(ncr_5380, int, 0);
-module_param(ncr_53c400, int, 0);
-module_param(ncr_53c400a, int, 0);
-module_param(dtc_3181e, int, 0);
-module_param(hp_c2502, int, 0);
-MODULE_LICENSE("GPL");
+static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
+{
+	struct Scsi_Host *sh = NULL;
+
+	sh = generic_NCR5380_hw_init(&driver_template, pdev, base[ndev],
+				     irq[ndev], card[ndev]);
+	if (!sh && base[ndev])
+		printk(KERN_WARNING "Card not found at address 0x%03x\n",
+				base[ndev]);
+	if (!sh)
+		return 0;
 
-#if !defined(SCSI_G_NCR5380_MEM) && defined(MODULE)
-static struct isapnp_device_id id_table[] = {
-	{
-	 ISAPNP_ANY_ID, ISAPNP_ANY_ID,
-	 ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e),
-	 0},
-	{0}
+	dev_set_drvdata(pdev, sh);
+	return 1;
+}
+
+static int generic_NCR5380_isa_remove(struct device *pdev,
+				   unsigned int ndev)
+{
+	generic_NCR5380_release_resources(dev_get_drvdata(pdev));
+	dev_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct isa_driver generic_NCR5380_isa_driver = {
+	.match		= generic_NCR5380_isa_match,
+	.remove		= generic_NCR5380_isa_remove,
+	.driver		= {
+		.name	= DRV_MODULE_NAME
+	},
+};
+static int isa_registered;
+
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+static struct pnp_device_id generic_NCR5380_pnp_ids[] = {
+	{ .id = "DTC436e", .driver_data = BOARD_DTC3181E },
+	{ .id = "" }
 };
+MODULE_DEVICE_TABLE(pnp, generic_NCR5380_pnp_ids);
+
+static int generic_NCR5380_pnp_probe(struct pnp_dev *pdev,
+			       const struct pnp_device_id *id)
+{
+	struct Scsi_Host *sh;
+	int base, irq;
 
-MODULE_DEVICE_TABLE(isapnp, id_table);
+	if (pnp_activate_dev(pdev) < 0)
+		return -EBUSY;
+
+	base = pnp_port_start(pdev, 0);
+	irq = pnp_irq(pdev, 0);
+
+	sh = generic_NCR5380_hw_init(&driver_template, &pdev->dev, base, irq,
+				     id->driver_data);
+	if (!sh)
+		return -ENODEV;
+
+	pnp_set_drvdata(pdev, sh);
+	return 0;
+}
+
+static void generic_NCR5380_pnp_remove(struct pnp_dev *pdev)
+{
+	generic_NCR5380_release_resources(pnp_get_drvdata(pdev));
+	pnp_set_drvdata(pdev, NULL);
+}
+
+static struct pnp_driver generic_NCR5380_pnp_driver = {
+	.name		= DRV_MODULE_NAME,
+	.id_table	= generic_NCR5380_pnp_ids,
+	.probe		= generic_NCR5380_pnp_probe,
+	.remove		= generic_NCR5380_pnp_remove,
+};
+static int pnp_registered;
+#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
+
+static int __init generic_NCR5380_init(void)
+{
+	int ret = 0;
+
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+	ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
+	if (!ret)
+		pnp_registered = 1;
 #endif
+	ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
+	if (!ret)
+		isa_registered = 1;
+
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+	if (pnp_registered)
+		ret = 0;
+#endif
+	if (isa_registered)
+		ret = 0;
+
+	return ret;
+}
+
+static void __exit generic_NCR5380_exit(void)
+{
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+	if (pnp_registered)
+		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
+#endif
+	if (isa_registered)
+		isa_unregister_driver(&generic_NCR5380_isa_driver);
+}
+
+module_init(generic_NCR5380_init);
+module_exit(generic_NCR5380_exit);
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index 5951774..b175b92 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -14,15 +14,9 @@
 #ifndef GENERIC_NCR5380_H
 #define GENERIC_NCR5380_H
 
-#define __STRVAL(x) #x
-#define STRVAL(x) __STRVAL(x)
-
 #ifndef SCSI_G_NCR5380_MEM
 #define DRV_MODULE_NAME "g_NCR5380"
 
-#define NCR5380_map_type int
-#define NCR5380_map_name port
-
 #define NCR5380_read(reg) \
 	inb(instance->io_port + (reg))
 #define NCR5380_write(reg, value) \
@@ -38,8 +32,6 @@
 /* therefore SCSI_G_NCR5380_MEM */
 #define DRV_MODULE_NAME "g_NCR5380_mmio"
 
-#define NCR5380_map_type unsigned long
-#define NCR5380_map_name base
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
 #define NCR53C400_region_size 0x3a00
-- 
Ondrej Zary

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Christoph Hellwig]

> +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
> +{
> +	struct Scsi_Host *sh = NULL;
> +
> +	sh = generic_NCR5380_hw_init(&driver_template, pdev, base[ndev],
> +				     irq[ndev], card[ndev]);
> +	if (!sh && base[ndev])
> +		printk(KERN_WARNING "Card not found at address 0x%03x\n",
> +				base[ndev]);
> +	if (!sh)
> +		return 0;
>  
> +	dev_set_drvdata(pdev, sh);

Any reason not to move the dev_set_drvdata into generic_NCR5380_hw_init?
That would also allow to properly propagate the error down to the
caller, which would be useful for the PNP case.

Otherwise this look great to me.

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Finn Thain]

On Sat, 24 Sep 2016, Ondrej Zary wrote:

> Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
> Use pnp_driver and isa_driver to manage cards.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/g_NCR5380.c |  310 +++++++++++++++++++++++++---------------------
>  drivers/scsi/g_NCR5380.h |    8 --
>  2 files changed, 169 insertions(+), 149 deletions(-)
> 
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 5162de6..6cf6b00 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -30,24 +30,25 @@
>  #include "NCR5380.h"
>  #include <linux/init.h>
>  #include <linux/ioport.h>
> -#include <linux/isapnp.h>
> +#include <linux/isa.h>
> +#include <linux/pnp.h>
>  #include <linux/interrupt.h>
>  
> -static int ncr_irq;
> -static int ncr_dma;
> -static int ncr_addr;
> -static int ncr_5380;
> -static int ncr_53c400;
> -static int ncr_53c400a;
> -static int dtc_3181e;
> -static int hp_c2502;
> -
> -static struct card {
> -	NCR5380_map_type NCR5380_map_name;
> -	int irq;
> -	int dma;
> -	int board;		/* Use NCR53c400, Ricoh, etc. extensions ? */
> -} card;
> +#define MAX_CARDS 8
> +
> +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(irq, int, NULL, 0);
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
> +
> +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(base, int, NULL, 0);
> +MODULE_PARM_DESC(base, "base address(es)");
> +
> +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
> +module_param_array(card, int, NULL, 0);
> +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC3181E, 4=HP C2502)");
> +
> +MODULE_LICENSE("GPL");


If you are going to change all of the module parameters and break every 
user's setup, at least explain what changed and why in the commit log and 
update the documentation in the kernel tree.

If you really want to help those users (most of whom gain nothing from 
this patch) you could also update the documentation elsewhere:

http://www.sane-project.org/man/sane-scsi.5.html
http://www.xsane.org/rauch-domain/sane-umax/sane-umax-config-doc.html
http://www.meier-geinitz.de/sane/mustek-backend/

I'm not in favour of this patch, but Christoph liked it so I'll leave it 
at that.

>  
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
> @@ -73,17 +74,9 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  }
>  #endif
>  
> -/**
> - * 	generic_NCR5380_detect	-	look for NCR5380 controllers
> - *	@tpnt: the scsi template
> - *
> - *	Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
> - *	and DTC436(ISAPnP) controllers.
> - *
> - *	Locks: none
> - */
> -
> -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
> +static struct Scsi_Host *generic_NCR5380_hw_init(
> +			struct scsi_host_template *tpnt, struct device *pdev,
> +			int base, int irq, int board)

This allocates and returns a Scsi_Host so the name is misleading.

But the name isn't the only problem; the real problem has already been 
pointed out by Christoph: the caller needs the error code to be returned 
not the Scsi_Host pointer.


>  {
>  	unsigned int *ports;
>  	u8 *magic = NULL;
> @@ -108,64 +101,13 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	struct Scsi_Host *instance;
>  	struct NCR5380_hostdata *hostdata;
>  #ifdef SCSI_G_NCR5380_MEM
> -	unsigned long base;
>  	void __iomem *iomem;
>  	resource_size_t iomem_size;
>  #endif
>  
> -	if (ncr_irq)
> -		card.irq = ncr_irq;
> -	if (ncr_dma)
> -		card.dma = ncr_dma;
> -	if (ncr_addr)
> -		card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
> -	if (ncr_5380)
> -		card.board = BOARD_NCR5380;
> -	else if (ncr_53c400)
> -		card.board = BOARD_NCR53C400;
> -	else if (ncr_53c400a)
> -		card.board = BOARD_NCR53C400A;
> -	else if (dtc_3181e)
> -		card.board = BOARD_DTC3181E;
> -	else if (hp_c2502)
> -		card.board = BOARD_HP_C2502;
> -#ifndef SCSI_G_NCR5380_MEM
> -	if (isapnp_present()) {
> -		struct pnp_dev *dev = NULL;
> -		while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) {
> -			if (pnp_device_attach(dev) < 0)
> -				continue;
> -			if (pnp_activate_dev(dev) < 0) {
> -				printk(KERN_ERR "dtc436e probe: activate failed\n");
> -				pnp_device_detach(dev);
> -				continue;
> -			}
> -			if (!pnp_port_valid(dev, 0)) {
> -				printk(KERN_ERR "dtc436e probe: no valid port\n");
> -				pnp_device_detach(dev);
> -				continue;
> -			}
> -			if (pnp_irq_valid(dev, 0))
> -				card.irq = pnp_irq(dev, 0);
> -			else
> -				card.irq = NO_IRQ;
> -			if (pnp_dma_valid(dev, 0))
> -				card.dma = pnp_dma(dev, 0);
> -			else
> -				card.dma = DMA_NONE;
> -			card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0);
> -			card.board = BOARD_DTC3181E;
> -			break;
> -		}
> -	}
> -#endif
> -
> -	if (!(card.NCR5380_map_name))
> -		return 0;
> -
>  	ports = NULL;
>  	flags = 0;
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR5380:
>  		flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
>  		break;
> @@ -191,17 +133,20 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  		magic_configure(-1, 0, magic);
>  
>  		region_size = 16;
> -
> -		if (card.NCR5380_map_name != PORT_AUTO)
> +		if (base)
>  			for (i = 0; ports[i]; i++) {
> -				if (!request_region(ports[i], region_size, "ncr53c80"))
> -					continue;
> -				if (card.NCR5380_map_name == ports[i])
> +				if (base == ports[i]) {	/* index found */
> +					if (!request_region(ports[i],
> +							    region_size,
> +							    "ncr53c80"))
> +						return NULL;
>  					break;
> -				release_region(ports[i], region_size);
> -		} else
> +				}
> +			}
> +		else
>  			for (i = 0; ports[i]; i++) {
> -				if (!request_region(ports[i], region_size, "ncr53c80"))
> +				if (!request_region(ports[i], region_size,
> +						    "ncr53c80"))
>  					continue;
>  				if (inb(ports[i]) == 0xff)
>  					break;
> @@ -212,38 +157,36 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  			magic_configure(i, 0, magic); /* no IRQ yet */
>  			outb(0xc0, ports[i] + 9);
>  			if (inb(ports[i] + 9) != 0x80)
> -				return 0;
> -			card.NCR5380_map_name = ports[i];
> +				goto out_release;
> +			base = ports[i];
>  			port_idx = i;
>  		} else
> -			return 0;
> +			return NULL;
>  	}
>  	else
>  	{
> -		/* Not a 53C400A style setup - just grab */
> +		/* NCR5380 - no configuration, just grab */
>  		region_size = 8;
> -		if (!request_region(card.NCR5380_map_name,
> -		                    region_size, "ncr5380"))
> -			return 0;
> +		if (!base || !request_region(base, region_size, "ncr5380"))
> +			return NULL;
>  	}
>  #else
> -	base = card.NCR5380_map_name;
>  	iomem_size = NCR53C400_region_size;
>  	if (!request_mem_region(base, iomem_size, "ncr5380"))
> -		return 0;
> +		return NULL;
>  	iomem = ioremap(base, iomem_size);
>  	if (!iomem) {
>  		release_mem_region(base, iomem_size);
> -		return 0;
> +		return NULL;
>  	}
>  #endif
> -	instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
> +	instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata));
>  	if (instance == NULL)
>  		goto out_release;
>  	hostdata = shost_priv(instance);
>  
>  #ifndef SCSI_G_NCR5380_MEM
> -	instance->io_port = card.NCR5380_map_name;
> +	instance->io_port = base;
>  	instance->n_io_port = region_size;
>  	hostdata->io_width = 1; /* 8-bit PDMA by default */
>  
> @@ -251,7 +194,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
>  	 * the base address.
>  	 */
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR53C400:
>  		instance->io_port += 8;
>  		hostdata->c400_ctl_status = 0;
> @@ -269,10 +212,10 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  		break;
>  	}
>  #else
> -	instance->base = card.NCR5380_map_name;
> +	instance->base = base;
>  	hostdata->iomem = iomem;
>  	hostdata->iomem_size = iomem_size;
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR53C400:
>  		hostdata->c400_ctl_status = 0x100;
>  		hostdata->c400_blk_cnt = 0x101;
> @@ -289,7 +232,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
>  		goto out_unregister;
>  
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR53C400:
>  	case BOARD_DTC3181E:
>  	case BOARD_NCR53C400A:
> @@ -299,8 +242,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  
>  	NCR5380_maybe_reset_bus(instance);
>  
> -	if (card.irq != IRQ_AUTO)
> -		instance->irq = card.irq;
> +	if (irq != IRQ_AUTO)
> +		instance->irq = irq;
>  	else
>  		instance->irq = NCR5380_probe_irq(instance, 0xffff);
>  
> @@ -311,7 +254,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	if (instance->irq != NO_IRQ) {
>  #ifndef SCSI_G_NCR5380_MEM
>  		/* set IRQ for HP C2502 */
> -		if (card.board == BOARD_HP_C2502)
> +		if (board == BOARD_HP_C2502)
>  			magic_configure(port_idx, instance->irq, magic);
>  #endif
>  		if (request_irq(instance->irq, generic_NCR5380_intr,
> @@ -326,31 +269,28 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  		printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no);
>  	}
>  
> -	return 1;


Instead of this:

> +	if (scsi_add_host(instance, pdev)) {
> +		free_irq(irq, instance);
> +		goto out_unregister;
> +	}
> +	scsi_scan_host(instance);
> +	return instance;
>  

please use the following (note the missing NCR5380_exit() call):

+	if (scsi_add_host(instance, pdev))
+		goto out_free_irq;
+	scsi_scan_host(instance);
+	return(instance);
+
+out_free_irq:
+	if (instance->irq != NO_IRQ)
+		free_irq(instance->irq, instance);
+	NCR5380_exit(instance);

>  out_unregister:
> -	scsi_unregister(instance);
> +	scsi_host_put(instance);
>  out_release:
>  #ifndef SCSI_G_NCR5380_MEM
> -	release_region(card.NCR5380_map_name, region_size);
> +	release_region(base, region_size);
>  #else
>  	iounmap(iomem);
>  	release_mem_region(base, iomem_size);
>  #endif
> -	return 0;
> +	return NULL;
>  }
>  
> -/**
> - *	generic_NCR5380_release_resources	-	free resources
> - *	@instance: host adapter to clean up 
> - *
> - *	Free the generic interface resources from this adapter.
> - *
> - *	Locks: none
> - */
> - 
> -static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
> +static void generic_NCR5380_release_resources(struct Scsi_Host *instance)
>  {
> +	scsi_remove_host(instance);
>  	if (instance->irq != NO_IRQ)
>  		free_irq(instance->irq, instance);
>  	NCR5380_exit(instance);
> @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
>  		release_mem_region(instance->base, hostdata->iomem_size);
>  	}
>  #endif
> -	return 0;
> +	scsi_host_put(instance);

The sequence of operations here should be the same as the error path
above.


>  }
>  
>  /**
> @@ -554,10 +494,9 @@ static int generic_NCR5380_dma_xfer_len(struct Scsi_Host *instance,
>  #include "NCR5380.c"
>  
>  static struct scsi_host_template driver_template = {
> +	.module			= THIS_MODULE,
>  	.proc_name		= DRV_MODULE_NAME,
>  	.name			= "Generic NCR5380/NCR53C400 SCSI",
> -	.detect			= generic_NCR5380_detect,
> -	.release		= generic_NCR5380_release_resources,
>  	.info			= generic_NCR5380_info,
>  	.queuecommand		= generic_NCR5380_queue_command,
>  	.eh_abort_handler	= generic_NCR5380_abort,
> @@ -571,26 +510,115 @@ static struct scsi_host_template driver_template = {
>  	.max_sectors		= 128,
>  };
>  
> -#include "scsi_module.c"
>  
> -module_param(ncr_irq, int, 0);
> -module_param(ncr_dma, int, 0);
> -module_param(ncr_addr, int, 0);
> -module_param(ncr_5380, int, 0);
> -module_param(ncr_53c400, int, 0);
> -module_param(ncr_53c400a, int, 0);
> -module_param(dtc_3181e, int, 0);
> -module_param(hp_c2502, int, 0);
> -MODULE_LICENSE("GPL");
> +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
> +{
> +	struct Scsi_Host *sh = NULL;
> +
> +	sh = generic_NCR5380_hw_init(&driver_template, pdev, base[ndev],
> +				     irq[ndev], card[ndev]);
> +	if (!sh && base[ndev])
> +		printk(KERN_WARNING "Card not found at address 0x%03x\n",
> +				base[ndev]);
> +	if (!sh)
> +		return 0;

For clarity, please write that as,

if (!instance) {
	if (base[ndev])
		printk(...)
	return 0;
}


>  
> -#if !defined(SCSI_G_NCR5380_MEM) && defined(MODULE)
> -static struct isapnp_device_id id_table[] = {
> -	{
> -	 ISAPNP_ANY_ID, ISAPNP_ANY_ID,
> -	 ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e),
> -	 0},
> -	{0}
> +	dev_set_drvdata(pdev, sh);
> +	return 1;
> +}
> +
> +static int generic_NCR5380_isa_remove(struct device *pdev,
> +				   unsigned int ndev)
> +{
> +	generic_NCR5380_release_resources(dev_get_drvdata(pdev));
> +	dev_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct isa_driver generic_NCR5380_isa_driver = {
> +	.match		= generic_NCR5380_isa_match,
> +	.remove		= generic_NCR5380_isa_remove,
> +	.driver		= {
> +		.name	= DRV_MODULE_NAME
> +	},
> +};
> +static int isa_registered;
> +
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +static struct pnp_device_id generic_NCR5380_pnp_ids[] = {
> +	{ .id = "DTC436e", .driver_data = BOARD_DTC3181E },
> +	{ .id = "" }
>  };
> +MODULE_DEVICE_TABLE(pnp, generic_NCR5380_pnp_ids);
> +
> +static int generic_NCR5380_pnp_probe(struct pnp_dev *pdev,
> +			       const struct pnp_device_id *id)
> +{
> +	struct Scsi_Host *sh;
> +	int base, irq;
>  
> -MODULE_DEVICE_TABLE(isapnp, id_table);
> +	if (pnp_activate_dev(pdev) < 0)
> +		return -EBUSY;
> +
> +	base = pnp_port_start(pdev, 0);
> +	irq = pnp_irq(pdev, 0);
> +
> +	sh = generic_NCR5380_hw_init(&driver_template, &pdev->dev, base, irq,
> +				     id->driver_data);
> +	if (!sh)
> +		return -ENODEV;
> +
> +	pnp_set_drvdata(pdev, sh);
> +	return 0;
> +}
> +
> +static void generic_NCR5380_pnp_remove(struct pnp_dev *pdev)
> +{
> +	generic_NCR5380_release_resources(pnp_get_drvdata(pdev));
> +	pnp_set_drvdata(pdev, NULL);
> +}
> +
> +static struct pnp_driver generic_NCR5380_pnp_driver = {
> +	.name		= DRV_MODULE_NAME,
> +	.id_table	= generic_NCR5380_pnp_ids,
> +	.probe		= generic_NCR5380_pnp_probe,
> +	.remove		= generic_NCR5380_pnp_remove,
> +};
> +static int pnp_registered;
> +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> +
> +static int __init generic_NCR5380_init(void)
> +{
> +	int ret = 0;
> +
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +	ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> +	if (!ret)
> +		pnp_registered = 1;
>  #endif
> +	ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> +	if (!ret)
> +		isa_registered = 1;
> +
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +	if (pnp_registered)
> +		ret = 0;
> +#endif
> +	if (isa_registered)
> +		ret = 0;
> +
> +	return ret;
> +}
> +
> +static void __exit generic_NCR5380_exit(void)
> +{
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +	if (pnp_registered)
> +		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> +#endif
> +	if (isa_registered)
> +		isa_unregister_driver(&generic_NCR5380_isa_driver);
> +}
> +

I find that hard to follow. This should be equivalent and simpler:

static int __init generic_NCR5380_init(void)
{
	isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
	pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
	return pnp_ret ? isa_ret : 0;
#endif
	return isa_ret;
}

static void __exit generic_NCR5380_exit(void)
{
	if (!isa_ret)
		isa_unregister_driver(&generic_NCR5380_isa_driver);
#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
	if (!pnp_ret)
		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
#endif
}

-- 

> +module_init(generic_NCR5380_init);
> +module_exit(generic_NCR5380_exit);
> diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
> index 5951774..b175b92 100644
> --- a/drivers/scsi/g_NCR5380.h
> +++ b/drivers/scsi/g_NCR5380.h
> @@ -14,15 +14,9 @@
>  #ifndef GENERIC_NCR5380_H
>  #define GENERIC_NCR5380_H
>  
> -#define __STRVAL(x) #x
> -#define STRVAL(x) __STRVAL(x)
> -
>  #ifndef SCSI_G_NCR5380_MEM
>  #define DRV_MODULE_NAME "g_NCR5380"
>  
> -#define NCR5380_map_type int
> -#define NCR5380_map_name port
> -
>  #define NCR5380_read(reg) \
>  	inb(instance->io_port + (reg))
>  #define NCR5380_write(reg, value) \
> @@ -38,8 +32,6 @@
>  /* therefore SCSI_G_NCR5380_MEM */
>  #define DRV_MODULE_NAME "g_NCR5380_mmio"
>  
> -#define NCR5380_map_type unsigned long
> -#define NCR5380_map_name base
>  #define NCR53C400_mem_base 0x3880
>  #define NCR53C400_host_buffer 0x3900
>  #define NCR53C400_region_size 0x3a00
> 

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Ondrej Zary]

On Monday 26 September 2016, Finn Thain wrote:

[...]

> Instead of this:
> > +	if (scsi_add_host(instance, pdev)) {
> > +		free_irq(irq, instance);
> > +		goto out_unregister;
> > +	}
> > +	scsi_scan_host(instance);
> > +	return instance;
>
> please use the following (note the missing NCR5380_exit() call):
>
> +	if (scsi_add_host(instance, pdev))
> +		goto out_free_irq;
> +	scsi_scan_host(instance);
> +	return(instance);
> +
> +out_free_irq:
> +	if (instance->irq != NO_IRQ)
> +		free_irq(instance->irq, instance);
> +	NCR5380_exit(instance);
>
> >  out_unregister:
> > -	scsi_unregister(instance);
> > +	scsi_host_put(instance);
> >  out_release:
> >  #ifndef SCSI_G_NCR5380_MEM
> > -	release_region(card.NCR5380_map_name, region_size);
> > +	release_region(base, region_size);
> >  #else
> >  	iounmap(iomem);
> >  	release_mem_region(base, iomem_size);
> >  #endif
> > -	return 0;
> > +	return NULL;
> >  }
> >
> > -/**
> > - *	generic_NCR5380_release_resources	-	free resources
> > - *	@instance: host adapter to clean up
> > - *
> > - *	Free the generic interface resources from this adapter.
> > - *
> > - *	Locks: none
> > - */
> > -
> > -static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
> > +static void generic_NCR5380_release_resources(struct Scsi_Host
> > *instance) {
> > +	scsi_remove_host(instance);
> >  	if (instance->irq != NO_IRQ)
> >  		free_irq(instance->irq, instance);
> >  	NCR5380_exit(instance);
> > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > Scsi_Host *instance) release_mem_region(instance->base,
> > hostdata->iomem_size);
> >  	}
> >  #endif
> > -	return 0;
> > +	scsi_host_put(instance);
>
> The sequence of operations here should be the same as the error path
> above.

I put scsi_host_put() call at the end because the release_mem_region code (in 
the MMIO case) dereferences the hostdata pointer. I don't think it's safe to 
do after scsi_host_put().

[...]

> > +static int pnp_registered;
> > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > +
> > +static int __init generic_NCR5380_init(void)
> > +{
> > +	int ret = 0;
> > +
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +	ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> > +	if (!ret)
> > +		pnp_registered = 1;
> >  #endif
> > +	ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> > +	if (!ret)
> > +		isa_registered = 1;
> > +
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +	if (pnp_registered)
> > +		ret = 0;
> > +#endif
> > +	if (isa_registered)
> > +		ret = 0;
> > +
> > +	return ret;
> > +}
> > +
> > +static void __exit generic_NCR5380_exit(void)
> > +{
> > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > +	if (pnp_registered)
> > +		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> > +#endif
> > +	if (isa_registered)
> > +		isa_unregister_driver(&generic_NCR5380_isa_driver);
> > +}
> > +
>
> I find that hard to follow. This should be equivalent and simpler:
>
> static int __init generic_NCR5380_init(void)
> {
> 	isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> 	pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> 	return pnp_ret ? isa_ret : 0;
> #endif
> 	return isa_ret;
> }
>
> static void __exit generic_NCR5380_exit(void)
> {
> 	if (!isa_ret)
> 		isa_unregister_driver(&generic_NCR5380_isa_driver);
> #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> 	if (!pnp_ret)
> 		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> #endif
> }

Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - 
especially this:
> 	return pnp_ret ? isa_ret : 0;

Also looking at the _exit function, meaning of isa_ret and pnp_ret is not 
obvious there.

Maybe we should have a module_isa_pnp_driver() macro.

-- 
Ondrej Zary

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Finn Thain]

On Tue, 27 Sep 2016, Ondrej Zary wrote:

> On Monday 26 September 2016, Finn Thain wrote:
> 
> [...]
> 
> > > @@ -364,7 +304,7 @@ static int generic_NCR5380_release_resources(struct
> > > Scsi_Host *instance) release_mem_region(instance->base,
> > > hostdata->iomem_size);
> > >  	}
> > >  #endif
> > > -	return 0;
> > > +	scsi_host_put(instance);
> >
> > The sequence of operations here should be the same as the error path
> > above.
> 
> I put scsi_host_put() call at the end because the release_mem_region 
> code (in the MMIO case) dereferences the hostdata pointer. I don't think 
> it's safe to do after scsi_host_put().

It's funny you say that, I already fixed a few of those use-after-free 
bugs in the other 5380 drivers. To avoid the bug, you'd need to use a 
temporary variable, like the fixes I did elsewhere.

Don't worry about changing it, this part of the driver gets revisited in 
my existing patch queue anyway.

> 
> [...]
> 
> > > +static int pnp_registered;
> > > +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
> > > +
> > > +static int __init generic_NCR5380_init(void)
> > > +{
> > > +	int ret = 0;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > +	ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> > > +	if (!ret)
> > > +		pnp_registered = 1;
> > >  #endif
> > > +	ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> > > +	if (!ret)
> > > +		isa_registered = 1;
> > > +
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > +	if (pnp_registered)
> > > +		ret = 0;
> > > +#endif
> > > +	if (isa_registered)
> > > +		ret = 0;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static void __exit generic_NCR5380_exit(void)
> > > +{
> > > +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > > +	if (pnp_registered)
> > > +		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> > > +#endif
> > > +	if (isa_registered)
> > > +		isa_unregister_driver(&generic_NCR5380_isa_driver);
> > > +}
> > > +
> >
> > I find that hard to follow. This should be equivalent and simpler:
> >
> > static int __init generic_NCR5380_init(void)
> > {
> > 	isa_ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > 	pnp_ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
> > 	return pnp_ret ? isa_ret : 0;
> > #endif
> > 	return isa_ret;
> > }
> >
> > static void __exit generic_NCR5380_exit(void)
> > {
> > 	if (!isa_ret)
> > 		isa_unregister_driver(&generic_NCR5380_isa_driver);
> > #if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> > 	if (!pnp_ret)
> > 		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> > #endif
> > }
> 
> Doesn't make it any better, IMHO. Good that it's shorter but not cleaner - 
> especially this:
> > 	return pnp_ret ? isa_ret : 0;

The problem with that line is that pnp_ret is always discarded, same as in 
your version. I think my version makes that clear, which is what matters 
to me.

> 
> Also looking at the _exit function, meaning of isa_ret and pnp_ret is not 
> obvious there.
> 
> Maybe we should have a module_isa_pnp_driver() macro.
> 
> 

FWIW I would hope that the hypothetical definition of this 
`module_isa_pnp_driver' macro would have the same properties as my 
version: shorter, uses no temporaries and uses one less #ifdef, and is 
generally easier for me to read.

-- 

[PATCH 4/3] g_NCR5380: Merge g_NCR5380 and g_NCR5380_mmio

[Ondrej Zary]

Merge the PIO and MMIO code (with the help of ioport_map) in g_NCR5380 and
delete g_NCR5380_mmio.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 drivers/scsi/Kconfig          |   32 ++---
 drivers/scsi/g_NCR5380.c      |  257 +++++++++++++++++++----------------------
 drivers/scsi/g_NCR5380.h      |   33 ++----
 drivers/scsi/g_NCR5380_mmio.c |   10 --
 4 files changed, 135 insertions(+), 197 deletions(-)
 delete mode 100644 drivers/scsi/g_NCR5380_mmio.c

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 7d1b431..fd1bca1 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -780,40 +780,22 @@ config SCSI_ISCI
 	  control unit found in the Intel(R) C600 series chipset.
 
 config SCSI_GENERIC_NCR5380
-	tristate "Generic NCR5380/53c400 SCSI PIO support"
+	tristate "Generic NCR5380/53c400 SCSI"
 	depends on ISA && SCSI
 	select SCSI_SPI_ATTRS
 	---help---
-	  This is a driver for the old NCR 53c80 series of SCSI controllers
-	  on boards using PIO. Most boards such as the Trantor T130 fit this
-	  category, along with a large number of ISA 8bit controllers shipped
-	  for free with SCSI scanners. If you have a PAS16, T128 or DMX3191
-	  you should select the specific driver for that card rather than
-	  generic 5380 support.
+	  This is a driver for the old NCR 53c80 series of SCSI controllers.
+	  Most boards such as the Trantor T130 fit this category, along with
+	  a large number of ISA 8bit controllers shipped for free with SCSI
+	  scanners. If you have a PAS16, T128 or DMX3191 you should select the
+	  specific driver for that card rather than generic 5380 support.
 
 	  It is explained in section 3.8 of the SCSI-HOWTO, available from
-	  <http://www.tldp.org/docs.html#howto>.  If it doesn't work out
-	  of the box, you may have to change some settings in
-	  <file:drivers/scsi/g_NCR5380.h>.
+	  <http://www.tldp.org/docs.html#howto>.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called g_NCR5380.
 
-config SCSI_GENERIC_NCR5380_MMIO
-	tristate "Generic NCR5380/53c400 SCSI MMIO support"
-	depends on ISA && SCSI
-	select SCSI_SPI_ATTRS
-	---help---
-	  This is a driver for the old NCR 53c80 series of SCSI controllers
-	  on boards using memory mapped I/O. 
-	  It is explained in section 3.8 of the SCSI-HOWTO, available from
-	  <http://www.tldp.org/docs.html#howto>.  If it doesn't work out
-	  of the box, you may have to change some settings in
-	  <file:drivers/scsi/g_NCR5380.h>.
-
-	  To compile this driver as a module, choose M here: the
-	  module will be called g_NCR5380_mmio.
-
 config SCSI_IPS
 	tristate "IBM ServeRAID support"
 	depends on PCI && SCSI
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 6cf6b00..7d8fcbf 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -50,7 +50,6 @@ MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC31
 
 MODULE_LICENSE("GPL");
 
-#ifndef SCSI_G_NCR5380_MEM
 /*
  * Configure I/O address of 53C400A or DTC436 by writing magic numbers
  * to ports 0x779 and 0x379.
@@ -72,41 +71,33 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 		cfg = 0x80 | idx | (irq << 4);
 	outb(cfg, 0x379);
 }
-#endif
+
+static unsigned int ncr_53c400a_ports[] __initdata = {
+	0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
+};
+static unsigned int dtc_3181e_ports[] __initdata = {
+	0x220, 0x240, 0x280, 0x2a0, 0x2c0, 0x300, 0x320, 0x340, 0
+};
+static u8 ncr_53c400a_magic[] __initdata = {	/* 53C400A & DTC436 */
+	0x59, 0xb9, 0xc5, 0xae, 0xa6
+};
+static u8 hp_c2502_magic[] __initdata = {	/* HP C2502 */
+	0x0f, 0x22, 0xf0, 0x20, 0x80
+};
 
 static struct Scsi_Host *generic_NCR5380_hw_init(
 			struct scsi_host_template *tpnt, struct device *pdev,
 			int base, int irq, int board)
 {
-	unsigned int *ports;
+	unsigned int *ports = NULL;
 	u8 *magic = NULL;
-#ifndef SCSI_G_NCR5380_MEM
-	int i;
-	int port_idx = -1;
+	int i, port_idx = -1, flags = 0;
 	unsigned long region_size;
-#endif
-	static unsigned int __initdata ncr_53c400a_ports[] = {
-		0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
-	};
-	static unsigned int __initdata dtc_3181e_ports[] = {
-		0x220, 0x240, 0x280, 0x2a0, 0x2c0, 0x300, 0x320, 0x340, 0
-	};
-	static u8 ncr_53c400a_magic[] __initdata = {	/* 53C400A & DTC436 */
-		0x59, 0xb9, 0xc5, 0xae, 0xa6
-	};
-	static u8 hp_c2502_magic[] __initdata = {	/* HP C2502 */
-		0x0f, 0x22, 0xf0, 0x20, 0x80
-	};
-	int flags;
 	struct Scsi_Host *instance;
 	struct NCR5380_hostdata *hostdata;
-#ifdef SCSI_G_NCR5380_MEM
 	void __iomem *iomem;
-	resource_size_t iomem_size;
-#endif
+	bool is_pio = base <= 0xffff;
 
-	ports = NULL;
-	flags = 0;
 	switch (board) {
 	case BOARD_NCR5380:
 		flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
@@ -125,8 +116,7 @@ static struct Scsi_Host *generic_NCR5380_hw_init(
 		break;
 	}
 
-#ifndef SCSI_G_NCR5380_MEM
-	if (ports && magic) {
+	if (is_pio && ports && magic) {
 		/* wakeup sequence for the NCR53C400A and DTC3181E */
 
 		/* Disable the adapter and look for a free io port */
@@ -162,72 +152,75 @@ static struct Scsi_Host *generic_NCR5380_hw_init(
 			port_idx = i;
 		} else
 			return NULL;
-	}
-	else
-	{
+	} else if (is_pio) {
 		/* NCR5380 - no configuration, just grab */
 		region_size = 8;
 		if (!base || !request_region(base, region_size, "ncr5380"))
 			return NULL;
+	} else {	/* MMIO */
+		region_size = NCR53C400_region_size;
+		if (!request_mem_region(base, region_size, "ncr5380"))
+			return NULL;
 	}
-#else
-	iomem_size = NCR53C400_region_size;
-	if (!request_mem_region(base, iomem_size, "ncr5380"))
-		return NULL;
-	iomem = ioremap(base, iomem_size);
-	if (!iomem) {
-		release_mem_region(base, iomem_size);
-		return NULL;
-	}
-#endif
+
+	if (is_pio)
+		iomem = ioport_map(base, region_size);
+	else
+		iomem = ioremap(base, region_size);
+
+	if (!iomem)
+		goto out_release;
+
 	instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata));
 	if (instance == NULL)
-		goto out_release;
+		goto out_unmap;
 	hostdata = shost_priv(instance);
 
-#ifndef SCSI_G_NCR5380_MEM
-	instance->io_port = base;
-	instance->n_io_port = region_size;
-	hostdata->io_width = 1; /* 8-bit PDMA by default */
-
-	/*
-	 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
-	 * the base address.
-	 */
-	switch (board) {
-	case BOARD_NCR53C400:
-		instance->io_port += 8;
-		hostdata->c400_ctl_status = 0;
-		hostdata->c400_blk_cnt = 1;
-		hostdata->c400_host_buf = 4;
-		break;
-	case BOARD_DTC3181E:
-		hostdata->io_width = 2;	/* 16-bit PDMA */
-		/* fall through */
-	case BOARD_NCR53C400A:
-	case BOARD_HP_C2502:
-		hostdata->c400_ctl_status = 9;
-		hostdata->c400_blk_cnt = 10;
-		hostdata->c400_host_buf = 8;
-		break;
-	}
-#else
-	instance->base = base;
 	hostdata->iomem = iomem;
-	hostdata->iomem_size = iomem_size;
-	switch (board) {
-	case BOARD_NCR53C400:
-		hostdata->c400_ctl_status = 0x100;
-		hostdata->c400_blk_cnt = 0x101;
-		hostdata->c400_host_buf = 0x104;
-		break;
-	case BOARD_DTC3181E:
-	case BOARD_NCR53C400A:
-	case BOARD_HP_C2502:
-		pr_err(DRV_MODULE_NAME ": unknown register offsets\n");
-		goto out_unregister;
+
+	if (is_pio) {
+		instance->io_port = base;
+		instance->n_io_port = region_size;
+		hostdata->io_width = 1; /* 8-bit PDMA by default */
+
+		/*
+		 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
+		 * the base address.
+		 */
+		switch (board) {
+		case BOARD_NCR53C400:
+			instance->io_port += 8;
+			hostdata->c400_ctl_status = 0;
+			hostdata->c400_blk_cnt = 1;
+			hostdata->c400_host_buf = 4;
+			break;
+		case BOARD_DTC3181E:
+			hostdata->io_width = 2;	/* 16-bit PDMA */
+			/* fall through */
+		case BOARD_NCR53C400A:
+		case BOARD_HP_C2502:
+			hostdata->c400_ctl_status = 9;
+			hostdata->c400_blk_cnt = 10;
+			hostdata->c400_host_buf = 8;
+			break;
+		}
+	} else {
+		instance->base = base;
+		hostdata->iomem_size = region_size;
+		hostdata->offset = NCR53C400_mem_base;
+		switch (board) {
+		case BOARD_NCR53C400:
+			hostdata->c400_ctl_status = 0x100;
+			hostdata->c400_blk_cnt = 0x101;
+			hostdata->c400_host_buf = 0x104;
+			break;
+		case BOARD_DTC3181E:
+		case BOARD_NCR53C400A:
+		case BOARD_HP_C2502:
+			pr_err(DRV_MODULE_NAME ": unknown register offsets\n");
+			goto out_unregister;
+		}
 	}
-#endif
 
 	if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
 		goto out_unregister;
@@ -252,11 +245,9 @@ static struct Scsi_Host *generic_NCR5380_hw_init(
 		instance->irq = NO_IRQ;
 
 	if (instance->irq != NO_IRQ) {
-#ifndef SCSI_G_NCR5380_MEM
 		/* set IRQ for HP C2502 */
 		if (board == BOARD_HP_C2502)
 			magic_configure(port_idx, instance->irq, magic);
-#endif
 		if (request_irq(instance->irq, generic_NCR5380_intr,
 				0, "NCR5380", instance)) {
 			printk(KERN_WARNING "scsi%d : IRQ%d not free, interrupts disabled\n", instance->host_no, instance->irq);
@@ -278,32 +269,29 @@ static struct Scsi_Host *generic_NCR5380_hw_init(
 
 out_unregister:
 	scsi_host_put(instance);
-out_release:
-#ifndef SCSI_G_NCR5380_MEM
-	release_region(base, region_size);
-#else
+out_unmap:
 	iounmap(iomem);
-	release_mem_region(base, iomem_size);
-#endif
+out_release:
+	if (is_pio)
+		release_region(base, region_size);
+	else
+		release_mem_region(base, region_size);
 	return NULL;
 }
 
 static void generic_NCR5380_release_resources(struct Scsi_Host *instance)
 {
+	struct NCR5380_hostdata *hostdata = shost_priv(instance);
+
 	scsi_remove_host(instance);
 	if (instance->irq != NO_IRQ)
 		free_irq(instance->irq, instance);
 	NCR5380_exit(instance);
-#ifndef SCSI_G_NCR5380_MEM
-	release_region(instance->io_port, instance->n_io_port);
-#else
-	{
-		struct NCR5380_hostdata *hostdata = shost_priv(instance);
-
-		iounmap(hostdata->iomem);
+	iounmap(hostdata->iomem);
+	if (instance->io_port)
+		release_region(instance->io_port, instance->n_io_port);
+	else
 		release_mem_region(instance->base, hostdata->iomem_size);
-	}
-#endif
 	scsi_host_put(instance);
 }
 
@@ -336,18 +324,16 @@ static inline int generic_NCR5380_pread(struct Scsi_Host *instance,
 		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 			; /* FIXME - no timeout */
 
-#ifndef SCSI_G_NCR5380_MEM
-		if (hostdata->io_width == 2)
+		if (instance->io_port && hostdata->io_width == 2)
 			insw(instance->io_port + hostdata->c400_host_buf,
 							dst + start, 64);
-		else
+		else if (instance->io_port)
 			insb(instance->io_port + hostdata->c400_host_buf,
 							dst + start, 128);
-#else
-		/* implies SCSI_G_NCR5380_MEM */
-		memcpy_fromio(dst + start,
-		              hostdata->iomem + NCR53C400_host_buffer, 128);
-#endif
+		else
+			memcpy_fromio(dst + start,
+				hostdata->iomem + NCR53C400_host_buffer, 128);
+
 		start += 128;
 		blocks--;
 	}
@@ -356,18 +342,16 @@ static inline int generic_NCR5380_pread(struct Scsi_Host *instance,
 		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 			; /* FIXME - no timeout */
 
-#ifndef SCSI_G_NCR5380_MEM
-		if (hostdata->io_width == 2)
+		if (instance->io_port && hostdata->io_width == 2)
 			insw(instance->io_port + hostdata->c400_host_buf,
-							dst + start, 64);
-		else
+						dst + start, 64);
+		else if (instance->io_port)
 			insb(instance->io_port + hostdata->c400_host_buf,
-							dst + start, 128);
-#else
-		/* implies SCSI_G_NCR5380_MEM */
-		memcpy_fromio(dst + start,
-		              hostdata->iomem + NCR53C400_host_buffer, 128);
-#endif
+						dst + start, 128);
+		else
+			memcpy_fromio(dst + start,
+				hostdata->iomem + NCR53C400_host_buffer, 128);
+
 		start += 128;
 		blocks--;
 	}
@@ -414,18 +398,17 @@ static inline int generic_NCR5380_pwrite(struct Scsi_Host *instance,
 			break;
 		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 			; // FIXME - timeout
-#ifndef SCSI_G_NCR5380_MEM
-		if (hostdata->io_width == 2)
+
+		if (instance->io_port && hostdata->io_width == 2)
 			outsw(instance->io_port + hostdata->c400_host_buf,
 							src + start, 64);
-		else
+		else if (instance->io_port)
 			outsb(instance->io_port + hostdata->c400_host_buf,
 							src + start, 128);
-#else
-		/* implies SCSI_G_NCR5380_MEM */
-		memcpy_toio(hostdata->iomem + NCR53C400_host_buffer,
-		            src + start, 128);
-#endif
+		else
+			memcpy_toio(hostdata->iomem + NCR53C400_host_buffer,
+				    src + start, 128);
+
 		start += 128;
 		blocks--;
 	}
@@ -433,18 +416,16 @@ static inline int generic_NCR5380_pwrite(struct Scsi_Host *instance,
 		while (NCR5380_read(hostdata->c400_ctl_status) & CSR_HOST_BUF_NOT_RDY)
 			; // FIXME - no timeout
 
-#ifndef SCSI_G_NCR5380_MEM
-		if (hostdata->io_width == 2)
+		if (instance->io_port && hostdata->io_width == 2)
 			outsw(instance->io_port + hostdata->c400_host_buf,
 							src + start, 64);
-		else
+		else if (instance->io_port)
 			outsb(instance->io_port + hostdata->c400_host_buf,
 							src + start, 128);
-#else
-		/* implies SCSI_G_NCR5380_MEM */
-		memcpy_toio(hostdata->iomem + NCR53C400_host_buffer,
-		            src + start, 128);
-#endif
+		else
+			memcpy_toio(hostdata->iomem + NCR53C400_host_buffer,
+				    src + start, 128);
+
 		start += 128;
 		blocks--;
 	}
@@ -544,7 +525,7 @@ static struct isa_driver generic_NCR5380_isa_driver = {
 };
 static int isa_registered;
 
-#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+#ifdef CONFIG_PNP
 static struct pnp_device_id generic_NCR5380_pnp_ids[] = {
 	{ .id = "DTC436e", .driver_data = BOARD_DTC3181E },
 	{ .id = "" }
@@ -585,13 +566,13 @@ static struct pnp_driver generic_NCR5380_pnp_driver = {
 	.remove		= generic_NCR5380_pnp_remove,
 };
 static int pnp_registered;
-#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
+#endif /* CONFIG_PNP */
 
 static int __init generic_NCR5380_init(void)
 {
 	int ret = 0;
 
-#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+#ifdef CONFIG_PNP
 	ret = pnp_register_driver(&generic_NCR5380_pnp_driver);
 	if (!ret)
 		pnp_registered = 1;
@@ -600,7 +581,7 @@ static int __init generic_NCR5380_init(void)
 	if (!ret)
 		isa_registered = 1;
 
-#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+#ifdef CONFIG_PNP
 	if (pnp_registered)
 		ret = 0;
 #endif
@@ -612,7 +593,7 @@ static int __init generic_NCR5380_init(void)
 
 static void __exit generic_NCR5380_exit(void)
 {
-#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+#ifdef CONFIG_PNP
 	if (pnp_registered)
 		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
 #endif
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index b175b92..2b777824 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -14,44 +14,30 @@
 #ifndef GENERIC_NCR5380_H
 #define GENERIC_NCR5380_H
 
-#ifndef SCSI_G_NCR5380_MEM
 #define DRV_MODULE_NAME "g_NCR5380"
 
 #define NCR5380_read(reg) \
-	inb(instance->io_port + (reg))
+	ioread8(((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
+		((struct NCR5380_hostdata *)shost_priv(instance))->offset + \
+		(reg))
 #define NCR5380_write(reg, value) \
-	outb(value, instance->io_port + (reg))
+	iowrite8(value, ((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
+		((struct NCR5380_hostdata *)shost_priv(instance))->offset + \
+		(reg))
 
 #define NCR5380_implementation_fields \
+	int offset; \
+	void __iomem *iomem; \
+	resource_size_t iomem_size; \
 	int c400_ctl_status; \
 	int c400_blk_cnt; \
 	int c400_host_buf; \
 	int io_width;
 
-#else 
-/* therefore SCSI_G_NCR5380_MEM */
-#define DRV_MODULE_NAME "g_NCR5380_mmio"
-
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
 #define NCR53C400_region_size 0x3a00
 
-#define NCR5380_read(reg) \
-	readb(((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
-	      NCR53C400_mem_base + (reg))
-#define NCR5380_write(reg, value) \
-	writeb(value, ((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
-	       NCR53C400_mem_base + (reg))
-
-#define NCR5380_implementation_fields \
-	void __iomem *iomem; \
-	resource_size_t iomem_size; \
-	int c400_ctl_status; \
-	int c400_blk_cnt; \
-	int c400_host_buf;
-
-#endif
-
 #define NCR5380_dma_xfer_len(instance, cmd, phase) \
         generic_NCR5380_dma_xfer_len(instance, cmd)
 #define NCR5380_dma_recv_setup		generic_NCR5380_pread
@@ -73,4 +59,3 @@
 #define BOARD_HP_C2502	4
 
 #endif /* GENERIC_NCR5380_H */
-
diff --git a/drivers/scsi/g_NCR5380_mmio.c b/drivers/scsi/g_NCR5380_mmio.c
deleted file mode 100644
index 8cdde71..0000000
--- a/drivers/scsi/g_NCR5380_mmio.c
+++ /dev/null
@@ -1,10 +0,0 @@
-/*
- *	There is probably a nicer way to do this but this one makes
- *	pretty obvious what is happening. We rebuild the same file with
- *	different options for mmio versus pio.
- */
-
-#define SCSI_G_NCR5380_MEM
-
-#include "g_NCR5380.c"
-
-- 
Ondrej Zary

Re: [PATCH 4/3] g_NCR5380: Merge g_NCR5380 and g_NCR5380_mmio

[Christoph Hellwig]

On Sun, Sep 25, 2016 at 09:39:20PM +0200, Ondrej Zary wrote:
> Merge the PIO and MMIO code (with the help of ioport_map) in g_NCR5380 and
> delete g_NCR5380_mmio.

Awesome!  this looks great to me, but we probably should thrown
in a MODULE_ALIAS for g_NCR5380_mmio so that existing module
configurations keep working.

>  #define NCR5380_read(reg) \
> +	ioread8(((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
> +		((struct NCR5380_hostdata *)shost_priv(instance))->offset + \
> +		(reg))
>  #define NCR5380_write(reg, value) \
> -	outb(value, instance->io_port + (reg))
> +	iowrite8(value, ((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
> +		((struct NCR5380_hostdata *)shost_priv(instance))->offset + \
> +		(reg))

Can we make these #defines point to inline helper functions that avoid
the casting magic, e.g. something like

static inline void g_NCR5380_write(struct Scsi_Host *instance, u32 reg,
		u8 value)
{
	struct NCR5380_hostdata *host = shost_priv(instance);

	iowrite8(value, host->iomem, host->offset + reg);
}

#define NCR5380_write(reg, value)
	g_NCR5380_write(instance, reg, value)

Re: [PATCH 4/3] g_NCR5380: Merge g_NCR5380 and g_NCR5380_mmio

[Finn Thain]

On Sun, 25 Sep 2016, Christoph Hellwig wrote:

> On Sun, Sep 25, 2016 at 09:39:20PM +0200, Ondrej Zary wrote:
> 
> >  #define NCR5380_read(reg) \
> > +	ioread8(((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
> > +		((struct NCR5380_hostdata *)shost_priv(instance))->offset + \
> > +		(reg))
> >  #define NCR5380_write(reg, value) \
> > -	outb(value, instance->io_port + (reg))
> > +	iowrite8(value, ((struct NCR5380_hostdata *)shost_priv(instance))->iomem + \
> > +		((struct NCR5380_hostdata *)shost_priv(instance))->offset + \
> > +		(reg))
> 
> Can we make these #defines point to inline helper functions that avoid
> the casting magic, e.g. something like
> 
> static inline void g_NCR5380_write(struct Scsi_Host *instance, u32 reg,
> 		u8 value)
> {
> 	struct NCR5380_hostdata *host = shost_priv(instance);
> 
> 	iowrite8(value, host->iomem, host->offset + reg);
> }
> 
> #define NCR5380_write(reg, value)
> 	g_NCR5380_write(instance, reg, value)

Let's not do that yet. I have a patch series that fixes the type casting 
and related issues that I will post soon. I expect that Ondrej's patches 
will be ready before mine are, so I'll rebase mine on this work before I 
ask him to test my patches.

-- 

Re: [PATCH 4/3] g_NCR5380: Merge g_NCR5380 and g_NCR5380_mmio

[Christoph Hellwig]

On Mon, Sep 26, 2016 at 10:37:13AM +1000, Finn Thain wrote:
> Let's not do that yet. I have a patch series that fixes the type casting 
> and related issues that I will post soon. I expect that Ondrej's patches 
> will be ready before mine are, so I'll rebase mine on this work before I 
> ask him to test my patches.

Ok, even better.

Re: [PATCH 4/3] g_NCR5380: Merge g_NCR5380 and g_NCR5380_mmio

[Finn Thain]

On Sun, 25 Sep 2016, Ondrej Zary wrote:

> Merge the PIO and MMIO code (with the help of ioport_map) in g_NCR5380 and
> delete g_NCR5380_mmio.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  drivers/scsi/Kconfig          |   32 ++---
>  drivers/scsi/g_NCR5380.c      |  257 +++++++++++++++++++----------------------
>  drivers/scsi/g_NCR5380.h      |   33 ++----
>  drivers/scsi/g_NCR5380_mmio.c |   10 --
>  4 files changed, 135 insertions(+), 197 deletions(-)
>  delete mode 100644 drivers/scsi/g_NCR5380_mmio.c
> 
> diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
> index 7d1b431..fd1bca1 100644
> --- a/drivers/scsi/Kconfig
> +++ b/drivers/scsi/Kconfig
> @@ -780,40 +780,22 @@ config SCSI_ISCI
>  	  control unit found in the Intel(R) C600 series chipset.
>  
>  config SCSI_GENERIC_NCR5380
> -	tristate "Generic NCR5380/53c400 SCSI PIO support"
> +	tristate "Generic NCR5380/53c400 SCSI"
>  	depends on ISA && SCSI
>  	select SCSI_SPI_ATTRS
>  	---help---
> -	  This is a driver for the old NCR 53c80 series of SCSI controllers
> -	  on boards using PIO. Most boards such as the Trantor T130 fit this
> -	  category, along with a large number of ISA 8bit controllers shipped
> -	  for free with SCSI scanners. If you have a PAS16, T128 or DMX3191
> -	  you should select the specific driver for that card rather than
> -	  generic 5380 support.
> +	  This is a driver for the old NCR 53c80 series of SCSI controllers.
> +	  Most boards such as the Trantor T130 fit this category, along with
> +	  a large number of ISA 8bit controllers shipped for free with SCSI
> +	  scanners. If you have a PAS16, T128 or DMX3191 you should select the
> +	  specific driver for that card rather than generic 5380 support.

The pas16 and t128 drivers were removed (see 4.9/scsi-queue). The DMX3191 
isn't an ISA card, so it isn't relevant.

>  
>  	  It is explained in section 3.8 of the SCSI-HOWTO, available from
> -	  <http://www.tldp.org/docs.html#howto>.  If it doesn't work out
> -	  of the box, you may have to change some settings in
> -	  <file:drivers/scsi/g_NCR5380.h>.
> +	  <http://www.tldp.org/docs.html#howto>.

I didn't find anything useful at that url.

Therefore, please change the help text as follows:

diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig
index 98e5d51..928d937 100644
--- a/drivers/scsi/Kconfig
+++ b/drivers/scsi/Kconfig
@@ -780,21 +780,14 @@ config SCSI_ISCI
 	  control unit found in the Intel(R) C600 series chipset.
 
 config SCSI_GENERIC_NCR5380
-	tristate "Generic NCR5380/53c400 SCSI PIO support"
+	tristate "Generic NCR5380/53C400 ISA card SCSI controller support"
 	depends on ISA && SCSI
 	select SCSI_SPI_ATTRS
 	---help---
-	  This is a driver for the old NCR 53c80 series of SCSI controllers
-	  on boards using PIO. Most boards such as the Trantor T130 fit this
-	  category, along with a large number of ISA 8bit controllers shipped
-	  for free with SCSI scanners. If you have a PAS16, T128 or DMX3191
-	  you should select the specific driver for that card rather than
-	  generic 5380 support.
-
-	  It is explained in section 3.8 of the SCSI-HOWTO, available from
-	  <http://www.tldp.org/docs.html#howto>.  If it doesn't work out
-	  of the box, you may have to change some settings in
-	  <file:drivers/scsi/g_NCR5380.h>.
+	  This is a driver for old ISA card SCSI controllers based on the
+	  NCR 5380, 53C80, 53C400 and 53C400A, and compatible DTC devices.
+	  Most boards such as the Trantor T130 fit this category, along
+	  with various 8-bit and 16-bit ISA cards bundled with SCSI scanners.
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called g_NCR5380.


The rest of the patch looks good (aside from the missing module alias that 
Chrisoph mentioned).

Thanks.

-- 

[PATCH v2 0/3] g_NCR5380: Modernization

[Ondrej Zary]

This small patch series removes deprecated code from g_NCR5380 driver
and converts it from scsi_module.c to scsi_add_host().

Tested with:
HP C2502 (53C400A chip)
Canon FG2-5202 (53C400 chip, memory-mapped)
DTC-3181L (DTCT-436P chip, PnP)

---
Changes in v2: 
 - updated Documentation/scsi/g_NCR5380.txt
 - kept old-style module parameters for compatibility
 - added missing NCR5380_exit() call
 - fixed error propagation from init
 - simplified driver registration

-- 
Ondrej Zary

[PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Ondrej Zary]

Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
Use pnp_driver and isa_driver to manage cards.

In order to support multiple cards, new module parameter format is
introduced. The old parameters are kept for compatibility.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
---
 Documentation/scsi/g_NCR5380.txt |   24 ++-
 drivers/scsi/g_NCR5380.c         |  335 ++++++++++++++++++++++----------------
 drivers/scsi/g_NCR5380.h         |    8 -
 3 files changed, 215 insertions(+), 152 deletions(-)

diff --git a/Documentation/scsi/g_NCR5380.txt b/Documentation/scsi/g_NCR5380.txt
index 843cbae..e2c1879 100644
--- a/Documentation/scsi/g_NCR5380.txt
+++ b/Documentation/scsi/g_NCR5380.txt
@@ -28,6 +28,16 @@ time. More info to come in the future.
 This driver works as a module.
 When included as a module, parameters can be passed on the insmod/modprobe
 command line:
+  irq=xx[,...]	the interrupt(s)
+  base=xx[,...]	the port or base address(es) (for port or memory mapped, resp.)
+  card=xx[,...]	card type(s):
+		0 = NCR5380,
+		1 = NCR53C400,
+		2 = NCR53C400A,
+		3 = Domex Technology Corp 3181E (DTC3181E)
+		4 = Hewlett Packard C2502
+
+These old-style parameters can support only one card:
   ncr_irq=xx   the interrupt
   ncr_addr=xx  the port or base address (for port or memory
                mapped, resp.)
@@ -36,11 +46,19 @@ command line:
   ncr_53c400a=1 to set up for a NCR53C400A board
   dtc_3181e=1  to set up for a Domex Technology Corp 3181E board
   hp_c2502=1   to set up for a Hewlett Packard C2502 board
+
 e.g.
-modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
+OLD: modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
+NEW: modprobe g_NCR5380 irq=5 base=0x350 card=0
   for a port mapped NCR5380 board or
-modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
-  for a memory mapped NCR53C400 board with interrupts disabled.
+
+OLD: modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
+NEW: modprobe g_NCR5380 irq=255 base=0xc8000 card=1
+  for a memory mapped NCR53C400 board with interrupts disabled or
+
+NEW: modprobe g_NCR5380 irq=0,7 base=0x240,0x300 card=3,4
+  for two cards: DTC3181 (in non-PnP mode) at 0x240 with no IRQ
+             and HP C2502 at 0x300 with IRQ 7
 
 (255 should be specified for no or DMA interrupt, 254 to autoprobe for an 
      IRQ line if overridden on the command line.)
diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
index 5162de6..cbf0103 100644
--- a/drivers/scsi/g_NCR5380.c
+++ b/drivers/scsi/g_NCR5380.c
@@ -30,24 +30,41 @@
 #include "NCR5380.h"
 #include <linux/init.h>
 #include <linux/ioport.h>
-#include <linux/isapnp.h>
+#include <linux/isa.h>
+#include <linux/pnp.h>
 #include <linux/interrupt.h>
 
+#define MAX_CARDS 8
+
+/* old-style parameters for compatibility */
 static int ncr_irq;
-static int ncr_dma;
 static int ncr_addr;
 static int ncr_5380;
 static int ncr_53c400;
 static int ncr_53c400a;
 static int dtc_3181e;
 static int hp_c2502;
+module_param(ncr_irq, int, 0);
+module_param(ncr_addr, int, 0);
+module_param(ncr_5380, int, 0);
+module_param(ncr_53c400, int, 0);
+module_param(ncr_53c400a, int, 0);
+module_param(dtc_3181e, int, 0);
+module_param(hp_c2502, int, 0);
+
+static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(irq, int, NULL, 0);
+MODULE_PARM_DESC(irq, "IRQ number(s)");
 
-static struct card {
-	NCR5380_map_type NCR5380_map_name;
-	int irq;
-	int dma;
-	int board;		/* Use NCR53c400, Ricoh, etc. extensions ? */
-} card;
+static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
+module_param_array(base, int, NULL, 0);
+MODULE_PARM_DESC(base, "base address(es)");
+
+static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+module_param_array(card, int, NULL, 0);
+MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC3181E, 4=HP C2502)");
+
+MODULE_LICENSE("GPL");
 
 #ifndef SCSI_G_NCR5380_MEM
 /*
@@ -73,17 +90,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
 }
 #endif
 
-/**
- * 	generic_NCR5380_detect	-	look for NCR5380 controllers
- *	@tpnt: the scsi template
- *
- *	Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
- *	and DTC436(ISAPnP) controllers.
- *
- *	Locks: none
- */
-
-static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
+static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
+			struct device *pdev, int base, int irq, int board)
 {
 	unsigned int *ports;
 	u8 *magic = NULL;
@@ -92,80 +100,29 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	int port_idx = -1;
 	unsigned long region_size;
 #endif
-	static unsigned int __initdata ncr_53c400a_ports[] = {
+	static unsigned int ncr_53c400a_ports[] = {
 		0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
 	};
-	static unsigned int __initdata dtc_3181e_ports[] = {
+	static unsigned int dtc_3181e_ports[] = {
 		0x220, 0x240, 0x280, 0x2a0, 0x2c0, 0x300, 0x320, 0x340, 0
 	};
-	static u8 ncr_53c400a_magic[] __initdata = {	/* 53C400A & DTC436 */
+	static u8 ncr_53c400a_magic[] = {	/* 53C400A & DTC436 */
 		0x59, 0xb9, 0xc5, 0xae, 0xa6
 	};
-	static u8 hp_c2502_magic[] __initdata = {	/* HP C2502 */
+	static u8 hp_c2502_magic[] = {	/* HP C2502 */
 		0x0f, 0x22, 0xf0, 0x20, 0x80
 	};
-	int flags;
+	int flags, ret;
 	struct Scsi_Host *instance;
 	struct NCR5380_hostdata *hostdata;
 #ifdef SCSI_G_NCR5380_MEM
-	unsigned long base;
 	void __iomem *iomem;
 	resource_size_t iomem_size;
 #endif
 
-	if (ncr_irq)
-		card.irq = ncr_irq;
-	if (ncr_dma)
-		card.dma = ncr_dma;
-	if (ncr_addr)
-		card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
-	if (ncr_5380)
-		card.board = BOARD_NCR5380;
-	else if (ncr_53c400)
-		card.board = BOARD_NCR53C400;
-	else if (ncr_53c400a)
-		card.board = BOARD_NCR53C400A;
-	else if (dtc_3181e)
-		card.board = BOARD_DTC3181E;
-	else if (hp_c2502)
-		card.board = BOARD_HP_C2502;
-#ifndef SCSI_G_NCR5380_MEM
-	if (isapnp_present()) {
-		struct pnp_dev *dev = NULL;
-		while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) {
-			if (pnp_device_attach(dev) < 0)
-				continue;
-			if (pnp_activate_dev(dev) < 0) {
-				printk(KERN_ERR "dtc436e probe: activate failed\n");
-				pnp_device_detach(dev);
-				continue;
-			}
-			if (!pnp_port_valid(dev, 0)) {
-				printk(KERN_ERR "dtc436e probe: no valid port\n");
-				pnp_device_detach(dev);
-				continue;
-			}
-			if (pnp_irq_valid(dev, 0))
-				card.irq = pnp_irq(dev, 0);
-			else
-				card.irq = NO_IRQ;
-			if (pnp_dma_valid(dev, 0))
-				card.dma = pnp_dma(dev, 0);
-			else
-				card.dma = DMA_NONE;
-			card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0);
-			card.board = BOARD_DTC3181E;
-			break;
-		}
-	}
-#endif
-
-	if (!(card.NCR5380_map_name))
-		return 0;
-
 	ports = NULL;
 	flags = 0;
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR5380:
 		flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
 		break;
@@ -191,17 +148,20 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 		magic_configure(-1, 0, magic);
 
 		region_size = 16;
-
-		if (card.NCR5380_map_name != PORT_AUTO)
+		if (base)
 			for (i = 0; ports[i]; i++) {
-				if (!request_region(ports[i], region_size, "ncr53c80"))
-					continue;
-				if (card.NCR5380_map_name == ports[i])
+				if (base == ports[i]) {	/* index found */
+					if (!request_region(ports[i],
+							    region_size,
+							    "ncr53c80"))
+						return -EBUSY;
 					break;
-				release_region(ports[i], region_size);
-		} else
+				}
+			}
+		else
 			for (i = 0; ports[i]; i++) {
-				if (!request_region(ports[i], region_size, "ncr53c80"))
+				if (!request_region(ports[i], region_size,
+						    "ncr53c80"))
 					continue;
 				if (inb(ports[i]) == 0xff)
 					break;
@@ -211,39 +171,41 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 			/* At this point we have our region reserved */
 			magic_configure(i, 0, magic); /* no IRQ yet */
 			outb(0xc0, ports[i] + 9);
-			if (inb(ports[i] + 9) != 0x80)
-				return 0;
-			card.NCR5380_map_name = ports[i];
+			if (inb(ports[i] + 9) != 0x80) {
+				ret = -ENODEV;
+				goto out_release;
+			}
+			base = ports[i];
 			port_idx = i;
 		} else
-			return 0;
+			return -EINVAL;
 	}
 	else
 	{
-		/* Not a 53C400A style setup - just grab */
+		/* NCR5380 - no configuration, just grab */
 		region_size = 8;
-		if (!request_region(card.NCR5380_map_name,
-		                    region_size, "ncr5380"))
-			return 0;
+		if (!base || !request_region(base, region_size, "ncr5380"))
+			return -EBUSY;
 	}
 #else
-	base = card.NCR5380_map_name;
 	iomem_size = NCR53C400_region_size;
 	if (!request_mem_region(base, iomem_size, "ncr5380"))
-		return 0;
+		return -EBUSY;
 	iomem = ioremap(base, iomem_size);
 	if (!iomem) {
 		release_mem_region(base, iomem_size);
-		return 0;
+		return -ENOMEM;
 	}
 #endif
-	instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
-	if (instance == NULL)
+	instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata));
+	if (instance == NULL) {
+		ret = -ENOMEM;
 		goto out_release;
+	}
 	hostdata = shost_priv(instance);
 
 #ifndef SCSI_G_NCR5380_MEM
-	instance->io_port = card.NCR5380_map_name;
+	instance->io_port = base;
 	instance->n_io_port = region_size;
 	hostdata->io_width = 1; /* 8-bit PDMA by default */
 
@@ -251,7 +213,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
 	 * the base address.
 	 */
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR53C400:
 		instance->io_port += 8;
 		hostdata->c400_ctl_status = 0;
@@ -269,10 +231,10 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 		break;
 	}
 #else
-	instance->base = card.NCR5380_map_name;
+	instance->base = base;
 	hostdata->iomem = iomem;
 	hostdata->iomem_size = iomem_size;
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR53C400:
 		hostdata->c400_ctl_status = 0x100;
 		hostdata->c400_blk_cnt = 0x101;
@@ -282,14 +244,16 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	case BOARD_NCR53C400A:
 	case BOARD_HP_C2502:
 		pr_err(DRV_MODULE_NAME ": unknown register offsets\n");
+		ret = -EINVAL;
 		goto out_unregister;
 	}
 #endif
 
-	if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
+	ret = NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP);
+	if (ret)
 		goto out_unregister;
 
-	switch (card.board) {
+	switch (board) {
 	case BOARD_NCR53C400:
 	case BOARD_DTC3181E:
 	case BOARD_NCR53C400A:
@@ -299,8 +263,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 
 	NCR5380_maybe_reset_bus(instance);
 
-	if (card.irq != IRQ_AUTO)
-		instance->irq = card.irq;
+	if (irq != IRQ_AUTO)
+		instance->irq = irq;
 	else
 		instance->irq = NCR5380_probe_irq(instance, 0xffff);
 
@@ -311,7 +275,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 	if (instance->irq != NO_IRQ) {
 #ifndef SCSI_G_NCR5380_MEM
 		/* set IRQ for HP C2502 */
-		if (card.board == BOARD_HP_C2502)
+		if (board == BOARD_HP_C2502)
 			magic_configure(port_idx, instance->irq, magic);
 #endif
 		if (request_irq(instance->irq, generic_NCR5380_intr,
@@ -326,31 +290,32 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
 		printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no);
 	}
 
-	return 1;
+	ret = scsi_add_host(instance, pdev);
+	if (ret)
+		goto out_free_irq;
+	scsi_scan_host(instance);
+	dev_set_drvdata(pdev, instance);
+	return 0;
 
+out_free_irq:
+	if (instance->irq != NO_IRQ)
+		free_irq(instance->irq, instance);
+	NCR5380_exit(instance);
 out_unregister:
-	scsi_unregister(instance);
+	scsi_host_put(instance);
 out_release:
 #ifndef SCSI_G_NCR5380_MEM
-	release_region(card.NCR5380_map_name, region_size);
+	release_region(base, region_size);
 #else
 	iounmap(iomem);
 	release_mem_region(base, iomem_size);
 #endif
-	return 0;
+	return ret;
 }
 
-/**
- *	generic_NCR5380_release_resources	-	free resources
- *	@instance: host adapter to clean up 
- *
- *	Free the generic interface resources from this adapter.
- *
- *	Locks: none
- */
- 
-static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
+static void generic_NCR5380_release_resources(struct Scsi_Host *instance)
 {
+	scsi_remove_host(instance);
 	if (instance->irq != NO_IRQ)
 		free_irq(instance->irq, instance);
 	NCR5380_exit(instance);
@@ -364,7 +329,7 @@ static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
 		release_mem_region(instance->base, hostdata->iomem_size);
 	}
 #endif
-	return 0;
+	scsi_host_put(instance);
 }
 
 /**
@@ -554,10 +519,9 @@ static int generic_NCR5380_dma_xfer_len(struct Scsi_Host *instance,
 #include "NCR5380.c"
 
 static struct scsi_host_template driver_template = {
+	.module			= THIS_MODULE,
 	.proc_name		= DRV_MODULE_NAME,
 	.name			= "Generic NCR5380/NCR53C400 SCSI",
-	.detect			= generic_NCR5380_detect,
-	.release		= generic_NCR5380_release_resources,
 	.info			= generic_NCR5380_info,
 	.queuecommand		= generic_NCR5380_queue_command,
 	.eh_abort_handler	= generic_NCR5380_abort,
@@ -571,26 +535,115 @@ static struct scsi_host_template driver_template = {
 	.max_sectors		= 128,
 };
 
-#include "scsi_module.c"
 
-module_param(ncr_irq, int, 0);
-module_param(ncr_dma, int, 0);
-module_param(ncr_addr, int, 0);
-module_param(ncr_5380, int, 0);
-module_param(ncr_53c400, int, 0);
-module_param(ncr_53c400a, int, 0);
-module_param(dtc_3181e, int, 0);
-module_param(hp_c2502, int, 0);
-MODULE_LICENSE("GPL");
+static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
+{
+	int ret = generic_NCR5380_init_one(&driver_template, pdev, base[ndev],
+					  irq[ndev], card[ndev]);
+	if (ret) {
+		if (base[ndev])
+			printk(KERN_WARNING "Card not found at address 0x%03x\n",
+			       base[ndev]);
+		return 0;
+	}
 
-#if !defined(SCSI_G_NCR5380_MEM) && defined(MODULE)
-static struct isapnp_device_id id_table[] = {
-	{
-	 ISAPNP_ANY_ID, ISAPNP_ANY_ID,
-	 ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e),
-	 0},
-	{0}
+	return 1;
+}
+
+static int generic_NCR5380_isa_remove(struct device *pdev,
+				   unsigned int ndev)
+{
+	generic_NCR5380_release_resources(dev_get_drvdata(pdev));
+	dev_set_drvdata(pdev, NULL);
+	return 0;
+}
+
+static struct isa_driver generic_NCR5380_isa_driver = {
+	.match		= generic_NCR5380_isa_match,
+	.remove		= generic_NCR5380_isa_remove,
+	.driver		= {
+		.name	= DRV_MODULE_NAME
+	},
+};
+
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+static struct pnp_device_id generic_NCR5380_pnp_ids[] = {
+	{ .id = "DTC436e", .driver_data = BOARD_DTC3181E },
+	{ .id = "" }
+};
+MODULE_DEVICE_TABLE(pnp, generic_NCR5380_pnp_ids);
+
+static int generic_NCR5380_pnp_probe(struct pnp_dev *pdev,
+			       const struct pnp_device_id *id)
+{
+	int base, irq;
+
+	if (pnp_activate_dev(pdev) < 0)
+		return -EBUSY;
+
+	base = pnp_port_start(pdev, 0);
+	irq = pnp_irq(pdev, 0);
+
+	return generic_NCR5380_init_one(&driver_template, &pdev->dev, base, irq,
+				       id->driver_data);
+}
+
+static void generic_NCR5380_pnp_remove(struct pnp_dev *pdev)
+{
+	generic_NCR5380_release_resources(pnp_get_drvdata(pdev));
+	pnp_set_drvdata(pdev, NULL);
+}
+
+static struct pnp_driver generic_NCR5380_pnp_driver = {
+	.name		= DRV_MODULE_NAME,
+	.id_table	= generic_NCR5380_pnp_ids,
+	.probe		= generic_NCR5380_pnp_probe,
+	.remove		= generic_NCR5380_pnp_remove,
 };
+#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
 
-MODULE_DEVICE_TABLE(isapnp, id_table);
+static int pnp_registered, isa_registered;
+
+static int __init generic_NCR5380_init(void)
+{
+	int ret = 0;
+
+	/* compatibility with old-style parameters */
+	if (irq[0] == 0 && base[0] == 0 && card[0] == -1) {
+		irq[0] = ncr_irq;
+		base[0] = ncr_addr;
+		if (ncr_5380)
+			card[0] = BOARD_NCR5380;
+		if (ncr_53c400)
+			card[0] = BOARD_NCR53C400;
+		if (ncr_53c400a)
+			card[0] = BOARD_NCR53C400A;
+		if (dtc_3181e)
+			card[0] = BOARD_DTC3181E;
+		if (hp_c2502)
+			card[0] = BOARD_HP_C2502;
+	}
+
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+	if (!pnp_register_driver(&generic_NCR5380_pnp_driver))
+		pnp_registered = 1;
 #endif
+	ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
+	if (!ret)
+		isa_registered = 1;
+
+	return (pnp_registered || isa_registered) ? 0 : ret;
+}
+
+static void __exit generic_NCR5380_exit(void)
+{
+#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
+	if (pnp_registered)
+		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
+#endif
+	if (isa_registered)
+		isa_unregister_driver(&generic_NCR5380_isa_driver);
+}
+
+module_init(generic_NCR5380_init);
+module_exit(generic_NCR5380_exit);
diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
index 5951774..b175b92 100644
--- a/drivers/scsi/g_NCR5380.h
+++ b/drivers/scsi/g_NCR5380.h
@@ -14,15 +14,9 @@
 #ifndef GENERIC_NCR5380_H
 #define GENERIC_NCR5380_H
 
-#define __STRVAL(x) #x
-#define STRVAL(x) __STRVAL(x)
-
 #ifndef SCSI_G_NCR5380_MEM
 #define DRV_MODULE_NAME "g_NCR5380"
 
-#define NCR5380_map_type int
-#define NCR5380_map_name port
-
 #define NCR5380_read(reg) \
 	inb(instance->io_port + (reg))
 #define NCR5380_write(reg, value) \
@@ -38,8 +32,6 @@
 /* therefore SCSI_G_NCR5380_MEM */
 #define DRV_MODULE_NAME "g_NCR5380_mmio"
 
-#define NCR5380_map_type unsigned long
-#define NCR5380_map_name base
 #define NCR53C400_mem_base 0x3880
 #define NCR53C400_host_buffer 0x3900
 #define NCR53C400_region_size 0x3a00
-- 
Ondrej Zary

Re: [PATCH 3/3] g_NCR5380: Stop using scsi_module.c

[Finn Thain]

Acked-by: Finn Thain <fthain@telegraphics.com.au>

On Tue, 27 Sep 2016, Ondrej Zary wrote:

> Convert g_NCR5380 to use scsi_add_host instead of scsi_module.c
> Use pnp_driver and isa_driver to manage cards.
> 
> In order to support multiple cards, new module parameter format is
> introduced. The old parameters are kept for compatibility.
> 
> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> ---
>  Documentation/scsi/g_NCR5380.txt |   24 ++-
>  drivers/scsi/g_NCR5380.c         |  335 ++++++++++++++++++++++----------------
>  drivers/scsi/g_NCR5380.h         |    8 -
>  3 files changed, 215 insertions(+), 152 deletions(-)
> 
> diff --git a/Documentation/scsi/g_NCR5380.txt b/Documentation/scsi/g_NCR5380.txt
> index 843cbae..e2c1879 100644
> --- a/Documentation/scsi/g_NCR5380.txt
> +++ b/Documentation/scsi/g_NCR5380.txt
> @@ -28,6 +28,16 @@ time. More info to come in the future.
>  This driver works as a module.
>  When included as a module, parameters can be passed on the insmod/modprobe
>  command line:
> +  irq=xx[,...]	the interrupt(s)
> +  base=xx[,...]	the port or base address(es) (for port or memory mapped, resp.)
> +  card=xx[,...]	card type(s):
> +		0 = NCR5380,
> +		1 = NCR53C400,
> +		2 = NCR53C400A,
> +		3 = Domex Technology Corp 3181E (DTC3181E)
> +		4 = Hewlett Packard C2502
> +
> +These old-style parameters can support only one card:
>    ncr_irq=xx   the interrupt
>    ncr_addr=xx  the port or base address (for port or memory
>                 mapped, resp.)
> @@ -36,11 +46,19 @@ command line:
>    ncr_53c400a=1 to set up for a NCR53C400A board
>    dtc_3181e=1  to set up for a Domex Technology Corp 3181E board
>    hp_c2502=1   to set up for a Hewlett Packard C2502 board
> +
>  e.g.
> -modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
> +OLD: modprobe g_NCR5380 ncr_irq=5 ncr_addr=0x350 ncr_5380=1
> +NEW: modprobe g_NCR5380 irq=5 base=0x350 card=0
>    for a port mapped NCR5380 board or
> -modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
> -  for a memory mapped NCR53C400 board with interrupts disabled.
> +
> +OLD: modprobe g_NCR5380 ncr_irq=255 ncr_addr=0xc8000 ncr_53c400=1
> +NEW: modprobe g_NCR5380 irq=255 base=0xc8000 card=1
> +  for a memory mapped NCR53C400 board with interrupts disabled or
> +
> +NEW: modprobe g_NCR5380 irq=0,7 base=0x240,0x300 card=3,4
> +  for two cards: DTC3181 (in non-PnP mode) at 0x240 with no IRQ
> +             and HP C2502 at 0x300 with IRQ 7
>  
>  (255 should be specified for no or DMA interrupt, 254 to autoprobe for an 
>       IRQ line if overridden on the command line.)
> diff --git a/drivers/scsi/g_NCR5380.c b/drivers/scsi/g_NCR5380.c
> index 5162de6..cbf0103 100644
> --- a/drivers/scsi/g_NCR5380.c
> +++ b/drivers/scsi/g_NCR5380.c
> @@ -30,24 +30,41 @@
>  #include "NCR5380.h"
>  #include <linux/init.h>
>  #include <linux/ioport.h>
> -#include <linux/isapnp.h>
> +#include <linux/isa.h>
> +#include <linux/pnp.h>
>  #include <linux/interrupt.h>
>  
> +#define MAX_CARDS 8
> +
> +/* old-style parameters for compatibility */
>  static int ncr_irq;
> -static int ncr_dma;
>  static int ncr_addr;
>  static int ncr_5380;
>  static int ncr_53c400;
>  static int ncr_53c400a;
>  static int dtc_3181e;
>  static int hp_c2502;
> +module_param(ncr_irq, int, 0);
> +module_param(ncr_addr, int, 0);
> +module_param(ncr_5380, int, 0);
> +module_param(ncr_53c400, int, 0);
> +module_param(ncr_53c400a, int, 0);
> +module_param(dtc_3181e, int, 0);
> +module_param(hp_c2502, int, 0);
> +
> +static int irq[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(irq, int, NULL, 0);
> +MODULE_PARM_DESC(irq, "IRQ number(s)");
>  
> -static struct card {
> -	NCR5380_map_type NCR5380_map_name;
> -	int irq;
> -	int dma;
> -	int board;		/* Use NCR53c400, Ricoh, etc. extensions ? */
> -} card;
> +static int base[] = { 0, 0, 0, 0, 0, 0, 0, 0 };
> +module_param_array(base, int, NULL, 0);
> +MODULE_PARM_DESC(base, "base address(es)");
> +
> +static int card[] = { -1, -1, -1, -1, -1, -1, -1, -1 };
> +module_param_array(card, int, NULL, 0);
> +MODULE_PARM_DESC(card, "card type (0=NCR5380, 1=NCR53C400, 2=NCR53C400A, 3=DTC3181E, 4=HP C2502)");
> +
> +MODULE_LICENSE("GPL");
>  
>  #ifndef SCSI_G_NCR5380_MEM
>  /*
> @@ -73,17 +90,8 @@ static void magic_configure(int idx, u8 irq, u8 magic[])
>  }
>  #endif
>  
> -/**
> - * 	generic_NCR5380_detect	-	look for NCR5380 controllers
> - *	@tpnt: the scsi template
> - *
> - *	Scan for the present of NCR5380, NCR53C400, NCR53C400A, DTC3181E
> - *	and DTC436(ISAPnP) controllers.
> - *
> - *	Locks: none
> - */
> -
> -static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
> +static int generic_NCR5380_init_one(struct scsi_host_template *tpnt,
> +			struct device *pdev, int base, int irq, int board)
>  {
>  	unsigned int *ports;
>  	u8 *magic = NULL;
> @@ -92,80 +100,29 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	int port_idx = -1;
>  	unsigned long region_size;
>  #endif
> -	static unsigned int __initdata ncr_53c400a_ports[] = {
> +	static unsigned int ncr_53c400a_ports[] = {
>  		0x280, 0x290, 0x300, 0x310, 0x330, 0x340, 0x348, 0x350, 0
>  	};
> -	static unsigned int __initdata dtc_3181e_ports[] = {
> +	static unsigned int dtc_3181e_ports[] = {
>  		0x220, 0x240, 0x280, 0x2a0, 0x2c0, 0x300, 0x320, 0x340, 0
>  	};
> -	static u8 ncr_53c400a_magic[] __initdata = {	/* 53C400A & DTC436 */
> +	static u8 ncr_53c400a_magic[] = {	/* 53C400A & DTC436 */
>  		0x59, 0xb9, 0xc5, 0xae, 0xa6
>  	};
> -	static u8 hp_c2502_magic[] __initdata = {	/* HP C2502 */
> +	static u8 hp_c2502_magic[] = {	/* HP C2502 */
>  		0x0f, 0x22, 0xf0, 0x20, 0x80
>  	};
> -	int flags;
> +	int flags, ret;
>  	struct Scsi_Host *instance;
>  	struct NCR5380_hostdata *hostdata;
>  #ifdef SCSI_G_NCR5380_MEM
> -	unsigned long base;
>  	void __iomem *iomem;
>  	resource_size_t iomem_size;
>  #endif
>  
> -	if (ncr_irq)
> -		card.irq = ncr_irq;
> -	if (ncr_dma)
> -		card.dma = ncr_dma;
> -	if (ncr_addr)
> -		card.NCR5380_map_name = (NCR5380_map_type) ncr_addr;
> -	if (ncr_5380)
> -		card.board = BOARD_NCR5380;
> -	else if (ncr_53c400)
> -		card.board = BOARD_NCR53C400;
> -	else if (ncr_53c400a)
> -		card.board = BOARD_NCR53C400A;
> -	else if (dtc_3181e)
> -		card.board = BOARD_DTC3181E;
> -	else if (hp_c2502)
> -		card.board = BOARD_HP_C2502;
> -#ifndef SCSI_G_NCR5380_MEM
> -	if (isapnp_present()) {
> -		struct pnp_dev *dev = NULL;
> -		while ((dev = pnp_find_dev(NULL, ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e), dev))) {
> -			if (pnp_device_attach(dev) < 0)
> -				continue;
> -			if (pnp_activate_dev(dev) < 0) {
> -				printk(KERN_ERR "dtc436e probe: activate failed\n");
> -				pnp_device_detach(dev);
> -				continue;
> -			}
> -			if (!pnp_port_valid(dev, 0)) {
> -				printk(KERN_ERR "dtc436e probe: no valid port\n");
> -				pnp_device_detach(dev);
> -				continue;
> -			}
> -			if (pnp_irq_valid(dev, 0))
> -				card.irq = pnp_irq(dev, 0);
> -			else
> -				card.irq = NO_IRQ;
> -			if (pnp_dma_valid(dev, 0))
> -				card.dma = pnp_dma(dev, 0);
> -			else
> -				card.dma = DMA_NONE;
> -			card.NCR5380_map_name = (NCR5380_map_type) pnp_port_start(dev, 0);
> -			card.board = BOARD_DTC3181E;
> -			break;
> -		}
> -	}
> -#endif
> -
> -	if (!(card.NCR5380_map_name))
> -		return 0;
> -
>  	ports = NULL;
>  	flags = 0;
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR5380:
>  		flags = FLAG_NO_PSEUDO_DMA | FLAG_DMA_FIXUP;
>  		break;
> @@ -191,17 +148,20 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  		magic_configure(-1, 0, magic);
>  
>  		region_size = 16;
> -
> -		if (card.NCR5380_map_name != PORT_AUTO)
> +		if (base)
>  			for (i = 0; ports[i]; i++) {
> -				if (!request_region(ports[i], region_size, "ncr53c80"))
> -					continue;
> -				if (card.NCR5380_map_name == ports[i])
> +				if (base == ports[i]) {	/* index found */
> +					if (!request_region(ports[i],
> +							    region_size,
> +							    "ncr53c80"))
> +						return -EBUSY;
>  					break;
> -				release_region(ports[i], region_size);
> -		} else
> +				}
> +			}
> +		else
>  			for (i = 0; ports[i]; i++) {
> -				if (!request_region(ports[i], region_size, "ncr53c80"))
> +				if (!request_region(ports[i], region_size,
> +						    "ncr53c80"))
>  					continue;
>  				if (inb(ports[i]) == 0xff)
>  					break;
> @@ -211,39 +171,41 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  			/* At this point we have our region reserved */
>  			magic_configure(i, 0, magic); /* no IRQ yet */
>  			outb(0xc0, ports[i] + 9);
> -			if (inb(ports[i] + 9) != 0x80)
> -				return 0;
> -			card.NCR5380_map_name = ports[i];
> +			if (inb(ports[i] + 9) != 0x80) {
> +				ret = -ENODEV;
> +				goto out_release;
> +			}
> +			base = ports[i];
>  			port_idx = i;
>  		} else
> -			return 0;
> +			return -EINVAL;
>  	}
>  	else
>  	{
> -		/* Not a 53C400A style setup - just grab */
> +		/* NCR5380 - no configuration, just grab */
>  		region_size = 8;
> -		if (!request_region(card.NCR5380_map_name,
> -		                    region_size, "ncr5380"))
> -			return 0;
> +		if (!base || !request_region(base, region_size, "ncr5380"))
> +			return -EBUSY;
>  	}
>  #else
> -	base = card.NCR5380_map_name;
>  	iomem_size = NCR53C400_region_size;
>  	if (!request_mem_region(base, iomem_size, "ncr5380"))
> -		return 0;
> +		return -EBUSY;
>  	iomem = ioremap(base, iomem_size);
>  	if (!iomem) {
>  		release_mem_region(base, iomem_size);
> -		return 0;
> +		return -ENOMEM;
>  	}
>  #endif
> -	instance = scsi_register(tpnt, sizeof(struct NCR5380_hostdata));
> -	if (instance == NULL)
> +	instance = scsi_host_alloc(tpnt, sizeof(struct NCR5380_hostdata));
> +	if (instance == NULL) {
> +		ret = -ENOMEM;
>  		goto out_release;
> +	}
>  	hostdata = shost_priv(instance);
>  
>  #ifndef SCSI_G_NCR5380_MEM
> -	instance->io_port = card.NCR5380_map_name;
> +	instance->io_port = base;
>  	instance->n_io_port = region_size;
>  	hostdata->io_width = 1; /* 8-bit PDMA by default */
>  
> @@ -251,7 +213,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	 * On NCR53C400 boards, NCR5380 registers are mapped 8 past
>  	 * the base address.
>  	 */
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR53C400:
>  		instance->io_port += 8;
>  		hostdata->c400_ctl_status = 0;
> @@ -269,10 +231,10 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  		break;
>  	}
>  #else
> -	instance->base = card.NCR5380_map_name;
> +	instance->base = base;
>  	hostdata->iomem = iomem;
>  	hostdata->iomem_size = iomem_size;
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR53C400:
>  		hostdata->c400_ctl_status = 0x100;
>  		hostdata->c400_blk_cnt = 0x101;
> @@ -282,14 +244,16 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	case BOARD_NCR53C400A:
>  	case BOARD_HP_C2502:
>  		pr_err(DRV_MODULE_NAME ": unknown register offsets\n");
> +		ret = -EINVAL;
>  		goto out_unregister;
>  	}
>  #endif
>  
> -	if (NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP))
> +	ret = NCR5380_init(instance, flags | FLAG_LATE_DMA_SETUP);
> +	if (ret)
>  		goto out_unregister;
>  
> -	switch (card.board) {
> +	switch (board) {
>  	case BOARD_NCR53C400:
>  	case BOARD_DTC3181E:
>  	case BOARD_NCR53C400A:
> @@ -299,8 +263,8 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  
>  	NCR5380_maybe_reset_bus(instance);
>  
> -	if (card.irq != IRQ_AUTO)
> -		instance->irq = card.irq;
> +	if (irq != IRQ_AUTO)
> +		instance->irq = irq;
>  	else
>  		instance->irq = NCR5380_probe_irq(instance, 0xffff);
>  
> @@ -311,7 +275,7 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  	if (instance->irq != NO_IRQ) {
>  #ifndef SCSI_G_NCR5380_MEM
>  		/* set IRQ for HP C2502 */
> -		if (card.board == BOARD_HP_C2502)
> +		if (board == BOARD_HP_C2502)
>  			magic_configure(port_idx, instance->irq, magic);
>  #endif
>  		if (request_irq(instance->irq, generic_NCR5380_intr,
> @@ -326,31 +290,32 @@ static int __init generic_NCR5380_detect(struct scsi_host_template *tpnt)
>  		printk(KERN_INFO "scsi%d : please jumper the board for a free IRQ.\n", instance->host_no);
>  	}
>  
> -	return 1;
> +	ret = scsi_add_host(instance, pdev);
> +	if (ret)
> +		goto out_free_irq;
> +	scsi_scan_host(instance);
> +	dev_set_drvdata(pdev, instance);
> +	return 0;
>  
> +out_free_irq:
> +	if (instance->irq != NO_IRQ)
> +		free_irq(instance->irq, instance);
> +	NCR5380_exit(instance);
>  out_unregister:
> -	scsi_unregister(instance);
> +	scsi_host_put(instance);
>  out_release:
>  #ifndef SCSI_G_NCR5380_MEM
> -	release_region(card.NCR5380_map_name, region_size);
> +	release_region(base, region_size);
>  #else
>  	iounmap(iomem);
>  	release_mem_region(base, iomem_size);
>  #endif
> -	return 0;
> +	return ret;
>  }
>  
> -/**
> - *	generic_NCR5380_release_resources	-	free resources
> - *	@instance: host adapter to clean up 
> - *
> - *	Free the generic interface resources from this adapter.
> - *
> - *	Locks: none
> - */
> - 
> -static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
> +static void generic_NCR5380_release_resources(struct Scsi_Host *instance)
>  {
> +	scsi_remove_host(instance);
>  	if (instance->irq != NO_IRQ)
>  		free_irq(instance->irq, instance);
>  	NCR5380_exit(instance);
> @@ -364,7 +329,7 @@ static int generic_NCR5380_release_resources(struct Scsi_Host *instance)
>  		release_mem_region(instance->base, hostdata->iomem_size);
>  	}
>  #endif
> -	return 0;
> +	scsi_host_put(instance);
>  }
>  
>  /**
> @@ -554,10 +519,9 @@ static int generic_NCR5380_dma_xfer_len(struct Scsi_Host *instance,
>  #include "NCR5380.c"
>  
>  static struct scsi_host_template driver_template = {
> +	.module			= THIS_MODULE,
>  	.proc_name		= DRV_MODULE_NAME,
>  	.name			= "Generic NCR5380/NCR53C400 SCSI",
> -	.detect			= generic_NCR5380_detect,
> -	.release		= generic_NCR5380_release_resources,
>  	.info			= generic_NCR5380_info,
>  	.queuecommand		= generic_NCR5380_queue_command,
>  	.eh_abort_handler	= generic_NCR5380_abort,
> @@ -571,26 +535,115 @@ static struct scsi_host_template driver_template = {
>  	.max_sectors		= 128,
>  };
>  
> -#include "scsi_module.c"
>  
> -module_param(ncr_irq, int, 0);
> -module_param(ncr_dma, int, 0);
> -module_param(ncr_addr, int, 0);
> -module_param(ncr_5380, int, 0);
> -module_param(ncr_53c400, int, 0);
> -module_param(ncr_53c400a, int, 0);
> -module_param(dtc_3181e, int, 0);
> -module_param(hp_c2502, int, 0);
> -MODULE_LICENSE("GPL");
> +static int generic_NCR5380_isa_match(struct device *pdev, unsigned int ndev)
> +{
> +	int ret = generic_NCR5380_init_one(&driver_template, pdev, base[ndev],
> +					  irq[ndev], card[ndev]);
> +	if (ret) {
> +		if (base[ndev])
> +			printk(KERN_WARNING "Card not found at address 0x%03x\n",
> +			       base[ndev]);
> +		return 0;
> +	}
>  
> -#if !defined(SCSI_G_NCR5380_MEM) && defined(MODULE)
> -static struct isapnp_device_id id_table[] = {
> -	{
> -	 ISAPNP_ANY_ID, ISAPNP_ANY_ID,
> -	 ISAPNP_VENDOR('D', 'T', 'C'), ISAPNP_FUNCTION(0x436e),
> -	 0},
> -	{0}
> +	return 1;
> +}
> +
> +static int generic_NCR5380_isa_remove(struct device *pdev,
> +				   unsigned int ndev)
> +{
> +	generic_NCR5380_release_resources(dev_get_drvdata(pdev));
> +	dev_set_drvdata(pdev, NULL);
> +	return 0;
> +}
> +
> +static struct isa_driver generic_NCR5380_isa_driver = {
> +	.match		= generic_NCR5380_isa_match,
> +	.remove		= generic_NCR5380_isa_remove,
> +	.driver		= {
> +		.name	= DRV_MODULE_NAME
> +	},
> +};
> +
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +static struct pnp_device_id generic_NCR5380_pnp_ids[] = {
> +	{ .id = "DTC436e", .driver_data = BOARD_DTC3181E },
> +	{ .id = "" }
> +};
> +MODULE_DEVICE_TABLE(pnp, generic_NCR5380_pnp_ids);
> +
> +static int generic_NCR5380_pnp_probe(struct pnp_dev *pdev,
> +			       const struct pnp_device_id *id)
> +{
> +	int base, irq;
> +
> +	if (pnp_activate_dev(pdev) < 0)
> +		return -EBUSY;
> +
> +	base = pnp_port_start(pdev, 0);
> +	irq = pnp_irq(pdev, 0);
> +
> +	return generic_NCR5380_init_one(&driver_template, &pdev->dev, base, irq,
> +				       id->driver_data);
> +}
> +
> +static void generic_NCR5380_pnp_remove(struct pnp_dev *pdev)
> +{
> +	generic_NCR5380_release_resources(pnp_get_drvdata(pdev));
> +	pnp_set_drvdata(pdev, NULL);
> +}
> +
> +static struct pnp_driver generic_NCR5380_pnp_driver = {
> +	.name		= DRV_MODULE_NAME,
> +	.id_table	= generic_NCR5380_pnp_ids,
> +	.probe		= generic_NCR5380_pnp_probe,
> +	.remove		= generic_NCR5380_pnp_remove,
>  };
> +#endif /* !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP) */
>  
> -MODULE_DEVICE_TABLE(isapnp, id_table);
> +static int pnp_registered, isa_registered;
> +
> +static int __init generic_NCR5380_init(void)
> +{
> +	int ret = 0;
> +
> +	/* compatibility with old-style parameters */
> +	if (irq[0] == 0 && base[0] == 0 && card[0] == -1) {
> +		irq[0] = ncr_irq;
> +		base[0] = ncr_addr;
> +		if (ncr_5380)
> +			card[0] = BOARD_NCR5380;
> +		if (ncr_53c400)
> +			card[0] = BOARD_NCR53C400;
> +		if (ncr_53c400a)
> +			card[0] = BOARD_NCR53C400A;
> +		if (dtc_3181e)
> +			card[0] = BOARD_DTC3181E;
> +		if (hp_c2502)
> +			card[0] = BOARD_HP_C2502;
> +	}
> +
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +	if (!pnp_register_driver(&generic_NCR5380_pnp_driver))
> +		pnp_registered = 1;
>  #endif
> +	ret = isa_register_driver(&generic_NCR5380_isa_driver, MAX_CARDS);
> +	if (!ret)
> +		isa_registered = 1;
> +
> +	return (pnp_registered || isa_registered) ? 0 : ret;
> +}
> +
> +static void __exit generic_NCR5380_exit(void)
> +{
> +#if !defined(SCSI_G_NCR5380_MEM) && defined(CONFIG_PNP)
> +	if (pnp_registered)
> +		pnp_unregister_driver(&generic_NCR5380_pnp_driver);
> +#endif
> +	if (isa_registered)
> +		isa_unregister_driver(&generic_NCR5380_isa_driver);
> +}
> +
> +module_init(generic_NCR5380_init);
> +module_exit(generic_NCR5380_exit);
> diff --git a/drivers/scsi/g_NCR5380.h b/drivers/scsi/g_NCR5380.h
> index 5951774..b175b92 100644
> --- a/drivers/scsi/g_NCR5380.h
> +++ b/drivers/scsi/g_NCR5380.h
> @@ -14,15 +14,9 @@
>  #ifndef GENERIC_NCR5380_H
>  #define GENERIC_NCR5380_H
>  
> -#define __STRVAL(x) #x
> -#define STRVAL(x) __STRVAL(x)
> -
>  #ifndef SCSI_G_NCR5380_MEM
>  #define DRV_MODULE_NAME "g_NCR5380"
>  
> -#define NCR5380_map_type int
> -#define NCR5380_map_name port
> -
>  #define NCR5380_read(reg) \
>  	inb(instance->io_port + (reg))
>  #define NCR5380_write(reg, value) \
> @@ -38,8 +32,6 @@
>  /* therefore SCSI_G_NCR5380_MEM */
>  #define DRV_MODULE_NAME "g_NCR5380_mmio"
>  
> -#define NCR5380_map_type unsigned long
> -#define NCR5380_map_name base
>  #define NCR53C400_mem_base 0x3880
>  #define NCR53C400_host_buffer 0x3900
>  #define NCR53C400_region_size 0x3a00
>