Re: (no subject)

[Takashi Iwai <tiwai@suse.de>]

At Tue, 11 Nov 2008 10:43:12 +0800,
Yang, Libin wrote:
> 
> Hi Takashi,
> 
> This patch is to support detecting new AMD HD Audio devices with HDA PCI
> class code. Would you please review it? Thanks a lot!

Thanks for the patch!  Some quick review comments below...


> --- alsa-driver-1.0.18.orig/alsa-kernel/pci/hda/hda_intel.c
> 2008-10-29 20:41:35.000000000 +0800
> +++ alsa-driver-1.0.18/alsa-kernel/pci/hda/hda_intel.c	2008-11-11
> 18:33:14.000000000 +0800
> @@ -291,6 +291,7 @@
>  /* Define VIA HD Audio Device ID*/
>  #define VIA_HDAC_DEVICE_ID		0x3288
>  
> +#define PCI_CLASS_MULTIMEDIA_HDA	0x040300

Put this into include/linux/pci_ids.h as a common definition.
And, IMO, it'd be better to name it *_HD_AUDIO than *_HDA.

>  
>  /*
>   */
> @@ -410,6 +411,7 @@
>  	AZX_DRIVER_ULI,
>  	AZX_DRIVER_NVIDIA,
>  	AZX_DRIVER_TERA,
> +	AZX_DRIVER_AMD_AUTO,

We can create a more generic entry, say, AZX_DRIVER_GENERIC.


> @@ -423,6 +425,7 @@
>  	[AZX_DRIVER_ULI] = "HDA ULI M5461",
>  	[AZX_DRIVER_NVIDIA] = "HDA NVidia",
>  	[AZX_DRIVER_TERA] = "HDA Teradici", 
> +	[AZX_DRIVER_AMD_AUTO] = "HDA AMD",

And, make it [AZX_DRIVER_GENERIC] = "HD-Audio Generic" or so.


>  static unsigned int azx_default_codecs[AZX_NUM_DRIVERS] __devinitdata =
> {
>  	[AZX_DRIVER_ICH] = 3,
>  	[AZX_DRIVER_ATI] = 3,
> +	[AZX_DRIVER_AMD_AUTO] = 3,

I thought you'll have up to 4 codecs?


>  static int __devinit azx_codec_create(struct azx *chip, const char
> *model,
> @@ -2146,6 +2150,7 @@
>  			chip->playback_streams = ULI_NUM_PLAYBACK;
>  			chip->capture_streams = ULI_NUM_CAPTURE;
>  			break;
> +		case AZX_DRIVER_AMD_AUTO:

AZX_DRIVER_GENERIC can go to "default".

>  		case AZX_DRIVER_ATIHDMI:
>  			chip->playback_streams = ATIHDMI_NUM_PLAYBACK;
>  			chip->capture_streams = ATIHDMI_NUM_CAPTURE;
> @@ -2373,6 +2378,9 @@
>  	{ PCI_DEVICE(0x10de, 0x0bd7), .driver_data = AZX_DRIVER_NVIDIA
> },
>  	/* Teradici */
>  	{ PCI_DEVICE(0x6549, 0x1200), .driver_data = AZX_DRIVER_TERA },
> +	/* AMD Generic, PCI class code and Vendor ID for HD Audio */
> +	{ PCI_VENDOR_ID_ATI, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> +	  PCI_CLASS_MULTIMEDIA_HDA, 0xffffff, AZX_DRIVER_AMD_AUTO },

Use PCI_DEVICE() macro and C99 style initialization, such as,

	{ PCI_DEVICE(PCI_VENDOR_ID_ATI, PCI_ANY_ID),
	  .class = PCI_CLASS_MULTIMEDIA_HD_AUDIO,
	  .class_mask = 0xffffff,
	  .driver_data = AZX_DRIVER_GENERIC },

thanks,

Takashi