Re: [PATCH 1/2 v1] pata_via.c: support VX855 and future chips whose IDE controller use 0x0571.

[Tejun Heo <tj@kernel.org>]

Hello, Joseph.

JosephChan@via.com.tw wrote:
> How about this one? Thanks.

Yeap, I like this one much better but I think this can be made a bit
prettier.

> --- a/drivers/ata/pata_via.c	2008-12-25 07:26:37.000000000 +0800
> +++ b/drivers/ata/pata_via.c	2009-01-17 03:34:55.000000000 +0800
> @@ -63,6 +63,7 @@
>  
>  #define DRV_NAME "pata_via"
>  #define DRV_VERSION "0.3.3"
> +#define SINGLE	1 /* To identify single channel controllers */

It's customary to prefix identifiers with something which is somewhat
unique, say, VIA_IDFLAG_SINGLE.  And if it's gonna be flags, the libata way
of doing it is...

enum {
     VIA_IDFLAG_SINGLE		= (1 << 0),
     VIA_IDFLAG_WHATEVER_NEXT	= (1 << 1),
};

and so on...

> @@ -460,6 +463,7 @@
>  	static int printed_version;
>  	u8 enable;
>  	u32 timing;
> +	int single_port = (int) id->driver_data;

How about unsigned long flags = id->driver_data; here

> +	if (single_port)
> +		ppi[1] = &ata_dummy_port_info;
> +	

and flags & VIA_IDFLAG_SINGLE here?

>  	if (!config->id) {
> -		printk(KERN_WARNING "via: Unknown VIA SouthBridge, disabling.\n");
> -		return -ENODEV;
> -	}
> -	pci_dev_put(isa);
> +		printk(KERN_WARNING "via: Unknown VIA SouthBridge.\n");
> +		config = via_isa_bridges;
> +	} else
> +		pci_dev_put(isa);

Also, you can just bypass whole southbridge thing.  Add another flag,
say, VIA_IDFLAG_IGN_SOUTH or something and just skip the whole thing
if the flags is set.

Thanks.

-- 
tejun