[alsa-devel] [PATCH 1/1] ALC889A chip identification

[Takashi Iwai]

At Fri, 15 May 2009 02:34:07 +0200,
Torben Schulz wrote:
> 
> With this patch the ALC889A chip will be identified correctly. The functions patch_alc882() and patch_alc883() have been altered to set the stream names correctly. Without this patch 'aplay -l' would identify an ALC889A chip as ALC885 for example. Apart from that a kernel info message will be printed if ALC885 or ALC889A chips are handled by the wrong patch function.
> 
> Signed-off by: Torben Schulz <public at letorbi.de>

Thanks for the patch!  Just a few comments below...


> --- alsa-driver/alsa-kernel/pci/hda/patch_realtek.c	2009-05-14 18:23:17.000000000 +0200
> +++ alsa-driver-mod/alsa-kernel/pci/hda/patch_realtek.c	2009-05-15 01:08:16.000000000 +0200
> @@ -7338,8 +7338,17 @@
> 		setup_preset(spec, &alc882_presets[board_config]);
> 
> 	if (codec->vendor_id == 0x10ec0885) {
> -		spec->stream_name_analog = "ALC885 Analog";
> -		spec->stream_name_digital = "ALC885 Digital";
> +		if ((codec->revision_id == 0x100101) ||
> + 		    (codec->revision_id == 0x100103)) {
> + 			spec->stream_name_analog = "ALC889A Analog";
> + 			spec->stream_name_digital = "ALC889A Digital";
> +			printk(KERN_INFO
> +			       "hda_codec: ALC889A handled by patch_alc882(),"
> +				" should be handled by patch_alc883()\n");

It's rather a driver internal thing and not necessarily to be told to
each user :)  If any, let it be a verbose debug print with
snd_printdd().

> + 		} else {
> + 			spec->stream_name_analog = "ALC885 Analog";
> + 			spec->stream_name_digital = "ALC885 Digital";
> + 		}
> 	} else {
> 		spec->stream_name_analog = "ALC882 Analog";
> 		spec->stream_name_digital = "ALC882 Digital";
> @@ -9231,6 +9240,27 @@
> 		setup_preset(spec, &alc883_presets[board_config]);
> 
> 	switch (codec->vendor_id) {
> + 	case 0x10ec0885:
> + 		if ((codec->revision_id == 0x100101) ||
> + 		    (codec->revision_id == 0x100103)) {

This if block can be skipped (thus less indent level) by ...

> + 			spec->stream_name_analog = "ALC889A Analog";
> + 			spec->stream_name_digital = "ALC889A Digital";
> +			if (!spec->num_adc_nids) {
> +				spec->num_adc_nids = ARRAY_SIZE(alc883_adc_nids);
> +				spec->adc_nids = alc883_adc_nids;
> +			}
> +			if (!spec->capsrc_nids)
> +				spec->capsrc_nids = alc883_capsrc_nids;
> +			spec->capture_style = CAPT_MIX; /* matrix-style capture */
> +			spec->init_amp = ALC_INIT_DEFAULT; /* always initialize */
> + 		} else {
> + 			spec->stream_name_analog = "ALC885 Analog";
> + 			spec->stream_name_digital = "ALC885 Digital";
> +			printk(KERN_INFO
> +			       "hda_codec: ALC885 handled by patch_alc883(),"
> +				" should be handled by patch_alc882()\n");

... checking the ALC889A at the beginning of patch_alc883() since
using patch_alc883() for ALC885 is an error indeed.


Could you fix and repost?


BTW, regarding the strings: basically we can do a better clean-up by
using already existing strings.  For example, make new fields,
hda_codec.vendor_name and hda_codec.chip_name, which points the vendor
id string in hda_codec.c and the chip name string in
snd_hda_codec_preset struct respectively.  Then all PCM name strings
can be created automatically.

But, it's another level of clean-up to be done later, after your
fixes.


thanks,

Takashi