Re: [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[Takashi Iwai <tiwai@suse.de>]

At Tue, 23 Nov 2010 19:06:39 +0800,
Wu Fengguang wrote:
> 
> On Tue, Nov 23, 2010 at 10:29:12AM +0100, David Henningsson wrote:
> > On 2010-11-23 08:12, Takashi Iwai wrote:
> > >At Fri, 12 Nov 2010 16:46:34 +0100,
> > >David Henningsson wrote:
> > >>
> > >>Since I'm a little new in HDMI land and the patch is non-trivial, I
> > >>wouldn't mind some comments on this one. Also thanks to Jaroslav who
> > >>cleared a few things out about the logical and physical devices which
> > >>helped me figure out what I should do about it :-)
> > >>
> > >>Anyway, here's the patch.
> > >>
> > >>Some newer chips have more than one HDMI output, but usually not
> > >>all of them are exposed as physical jacks. Removing the unused
> > >>PCM devices (as indicated by BIOS in the pin config default) will
> > >>reduce user confusion as they currently have to choose between
> > >>several HDMI devices, some of them not working anyway.
> > >>
> > >>Signed-off-by: David Henningsson<david.henningsson@canonical.com>
> > >
> > >The patch looks mostly OK.
> > >But this gives a few errors and warnings via checkpatch.pl.
> > >Could you fix errors, at least?
> > >
> > >Also some review comments:
> > >
> > >>  static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t nid)
> > >>  {
> > >>+	int i;
> > >>  	struct hdmi_spec *spec = codec->spec;
> > >>
> > >>-	if (spec->num_cvts>= MAX_HDMI_CVTS) {
> > >>-		snd_printk(KERN_WARNING
> > >>-			   "HDMI: no space for converter %d\n", nid);
> > >>+	for (i = 0; spec->pin_cvt[i] != nid; i++)
> > >
> > >I'd write with a normal loop like for (i = 0; i<  spec->num_pins; i++)
> > >This is easier to read.
> > >
> > >>+		if (!spec->pin[i]) {
> > >>+			snd_printd("HDMI: Skipping node %d (no connection)\n", nid);
> > >
> > >Better to use snd_printdd().  snd_printd() is enabled on many distros,
> > >and such an unnecessary kernel message may make user worry.
> > >
> > >>@@ -951,17 +954,33 @@ static int hdmi_parse_codec(struct hda_codec *codec)
> > >...
> > >>  		case AC_WID_PIN:
> > >>  			caps = snd_hda_param_read(codec, nid, AC_PAR_PIN_CAP);
> > >>  			if (!(caps&  (AC_PINCAP_HDMI | AC_PINCAP_DP)))
> > >>  				continue;
> > >>+
> > >>+			config = snd_hda_codec_read(codec, nid, 0,
> > >>+					     AC_VERB_GET_CONFIG_DEFAULT, 0);
> > >>+			if (((config&  AC_DEFCFG_PORT_CONN)>>
> > >>+			    AC_DEFCFG_PORT_CONN_SHIFT) == AC_JACK_PORT_NONE)
> > >
> > >You can use get_defcfg_connect() here.
> > 
> > Thanks for the review, Takashi! I usually run checkpatch.pl these
> > days but this one must have slipped. What do you think of this patch
> > (untested)?
> 
> Thanks for the patch, I'll help test it out :)
> 
> I didn't do such feature because I'm not sure if the BIOS provided
> info can be trusted, or if that's merely the default configuration
> that may be changed runtime on user request (eg. switching from 
> A+A view to A+B view etc.).

Well, whether this BIOS setup is really reliable is still an open
question.  David, do you know (or have) the case whether your patch
works on a real machine, right?

Other than that, I don't see any problem in the patch.
I'm going to apply it (but for 2.6.38) later.


thanks,

Takashi