[alsa-devel] Unifying sound/pci/hda/patch_*hdmi.c

[Takashi Iwai]

At Thu, 16 Sep 2010 11:10:13 -0700,
Stephen Warren wrote:
> 
> Takashi Iwai wrote:
> > the below is a patch for unifying three patch_*hdmi.c into a single
> > file.  I'd like to push this to 2.6.37, so please review, especially
> > whether it can break Nvidia hardware (since I have no h/w available).
> 
> First off, in general, I think working towards unifying all the HDMI
> code is a great goal.
> 
> However, this patch worries me a bit because it significantly changes
> the way some parts of the code work at the same time as performing the
> refactoring...
> 
> In particular, after a brief skim, I see the following issues:
> 
> +static int patch_nvhdmi_8ch_89(struct hda_codec *codec)
> +{
> +	struct hdmi_spec *spec;
> +	int err = patch_generic_hdmi(codec);
> +
> +	if (err < 0)
> +		return err;
> +	spec = codec->spec;
> +	spec->old_pin_detect = 1;
> +	spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x;
> 
> That last line is definitely wrong as far as I can tell. Just delete
> it?

Ah, crap, this has to be removed.


> +	return 0;
> +}
> 
> +static int patch_nvhdmi_8ch_7x(struct hda_codec *codec)
> +{
> +	struct hdmi_spec *spec;
> +	int err = patch_generic_hdmi(codec);
> 
> The old nvhdmi_8ch_7x code was custom like nvhdmi_2ch. Unfortunately, I'm
> not familiar with the old HW nor why it used custom code instead of the
> generic parser. This seems like a risky change without testing.

Looks like I did wrong copy&paste here and there.  It was intended to
call patch_nvhdmi_2ch() instead.  The additional fix patch is below.


> One behavioral change I see is that patch_generic_hdmi will call
> snd_hda_eld_proc_new, which wasn't previously called for this codec type.
> Yet, nvhdmi_patch_ops_8ch_7x doesn't point unsol_event at hdmi_unsol_event,
> so the ELD stuff won't work. I don't know if the unsolicited event logic in
> the HW works on this chip or not, given the Linux driver hasn't used it
> before. I suppose it has to work, but who knows...
> 
> Related,
> 
> +static struct hda_codec_ops nvhdmi_patch_ops_2ch = {
> +	.build_controls = generic_hdmi_build_controls,
> +	.build_pcms = generic_hdmi_build_pcms,
> +	.init = nvhdmi_7x_init,
> +	.free = generic_hdmi_free,
> +};
> 
> Generic_hdmi_free calls snd_hda_eld_proc_free. Since nvhdmi_7x_init
> doesn't call generic_hdmi_init, will that attempt to free ELD structures
> that aren't allocated?

The call of snd_hda_eld_proc_free() is harmless, so I chose one
function to reduce rather than having two. 


> Once the above issues are thought through, I can try to find time to test
> this on HW. Unfortunately, I definitely don't have any of the "2ch" HW
> available to test at all. I do have a small subset of the "8ch_7x" and
> "8ch_89" HW (although my only HDMI sink at work only supports 2-channel
> audio). Also, my allotted time to work on the audio driver is very small
> right now, plus I'll be away from the office next week and hence won't
> have HW access to test anything anyway. Sorry I suck at being a tester!

OK, no problem.
Thanks for review, anyway.


Takashi

---
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index c6d183e..7d4ed7f 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1352,7 +1352,6 @@ static int patch_nvhdmi_8ch_89(struct hda_codec *codec)
 		return err;
 	spec = codec->spec;
 	spec->old_pin_detect = 1;
-	spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x;
 	return 0;
 }
 
@@ -1382,12 +1381,13 @@ static int patch_nvhdmi_2ch(struct hda_codec *codec)
 static int patch_nvhdmi_8ch_7x(struct hda_codec *codec)
 {
 	struct hdmi_spec *spec;
-	int err = patch_generic_hdmi(codec);
+	int err = patch_nvhdmi_2ch(codec);
 
 	if (err < 0)
 		return err;
 	spec = codec->spec;
 	spec->multiout.max_channels = 8;
+	spec->pcm_playback = &nvhdmi_pcm_playback_8ch_7x;
 	codec->patch_ops = nvhdmi_patch_ops_8ch_7x;
 	return 0;
 }