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 Henningssondavid.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.).
Thanks, Fengguang