[alsa-devel] [PATCH] ALSA: HDA: Remove unconnected PCM devices for Intel HDMI

[David Henningsson]

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 at 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)?

-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ALSA-HDA-Remove-unconnected-PCM-devices-for-Intel-HD.patch
Type: text/x-patch
Size: 0 bytes
Desc: not available
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20101123/e837260f/attachment.patch