Re: [PATCH] ITE887x parport_serial driver

[Zwane Mwaikambo <zwane@linuxpower.ca>]

On Wed, 11 Jun 2003 Ted.Wen@ite.com.tw wrote:

> In parport_serial , I request a irq and request region to initialize 887x ,
> so I must run a additional ite887x_removecheck function to check it.
> Some type of 887x  only with serial or only with parport , so that 
> sometimes the port num would be 0 .
>  I want the driver can proceed with runing even the card with no parport , or with on serial.
> ( the original driver return -ENODEV while fail in parport_register or serial_register )
>  I set success = 1 while the port num =0 in the tables , to avoid fail from parport register or serial register.

Still a few things to work out, basically it would be nice to remove any 
none generic functions out of generic ones. Can't you add postremove 
hooks?

> I also fix a bug while printk the parport information.
> The original driver printk parport_serial_pci_tb[i].vendor , 
>   the index "i" is be used to cards[i].....but parport_serial_pci_tbl and cards are not match.
> The bug will show wrong message wile printk parport information,
>  so I fix it to be dev->vendor , to show the correct message.

Thanks that looks correct.

> +ite887x_removecheck(struct pci_dev *dev)
> +{
> +	if (dev->vendor == PCI_VENDOR_ID_ITE
> +	    && dev->device == PCI_DEVICE_ID_ITE_8872) {
> +		free_irq(dev->irq, dev);
> +		release_region(ITE887x_INTA[rm_ite++], 0x8);
> +	}
> +}

This would be the post remove hook function for ITE

> +	if (board->num_ports == 0)
> +		success = 1;
>
>  	for (k = 0; k < board->num_ports; k++) {
>  		int line;
>  		serial_req.irq = dev->irq;

I understand what you're doing there but i'll think about a way to avoid 
the serial_register instead.

> @@ -308,6 +483,8 @@
>  	    cards[i].preinit_hook (dev, PARPORT_IRQ_NONE, PARPORT_DMA_NONE))
>  		return -ENODEV;
>  
> +	if (cards[i].numports == 0)
> +		success = 1;
>  	for (n = 0; n < cards[i].numports; n++) {
>  		struct parport *port;
>  		int lo = cards[i].addr[n].lo;

Ditto.

> @@ -324,8 +501,8 @@
>  		/* TODO: test if sharing interrupts works */
>  		printk (KERN_DEBUG "PCI parallel port detected: %04x:%04x, "
>  			"I/O at %#lx(%#lx)\n",
> -			parport_serial_pci_tbl[i].vendor,
> -			parport_serial_pci_tbl[i].device, io_lo, io_hi);
> +			dev->vendor,
> +			dev->device, io_lo, io_hi);
>  		port = parport_pc_probe_port (io_lo, io_hi, PARPORT_IRQ_NONE,
>  					      PARPORT_DMA_NONE, dev);

Nice catch.

>  		if (port) {
> @@ -359,6 +536,7 @@
>  		return err;
>  	}
>  
> +	ite887x_index = id->driver_data;
>  	if (parport_register (dev, id)) {
>  		pci_set_drvdata (dev, NULL);
>  		kfree (priv);
> @@ -395,6 +573,7 @@
>  	for (i = 0; i < priv->num_par; i++)
>  		parport_pc_unregister_port (priv->port[i]);
>  
> +	ite887x_removecheck(dev);
>  	kfree (priv);
>  	return;

Perhaps you can add a hook so that devices can do cleanup in 
parport_serial_pci_remove, in the same vein as the preinit hooks.

Thanks,
	Zwane
-- 
function.linuxpower.ca