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

[Takashi Iwai <tiwai@suse.de>]

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,

Takashi