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@xxxxxxxxxx>
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
