[alsa-devel] [PATCH] add the nvidia HDMI codec driver for MCP77/79

[Takashi Iwai]

At Wed, 3 Sep 2008 19:05:47 +0800,
Wei Ni wrote:
> 
> 
>  Thanks for your comments. Some reply below:
> 
> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de] 
> Sent: Wednesday, September 03, 2008 4:52 PM
> To: peerchen
> Cc: alsa-devel; linux-kernel; akpm; Peer Chen; Wei Ni
> Subject: Re: [alsa-devel][PATCH] add the nvidia HDMI codec driver for
> MCP77/79
> 
> At Wed, 3 Sep 2008 15:58:17 +0800,
> peerchen wrote:
> > 
> > Add the nvidia HDMI codec driver for MCP77/79. 
> > The patch based on kernel 2.6.27-rc5.
> > 
> > Signed-off-by: Wei Ni <wni at nvidia.com>
> > Signed-off-by: Peer Chen <peerchen at gmail.com>
> 
> Thanks for the patch.  Some comments below.
> 
> 
> > diff -uprN -X linux-2.6.26-Orig/Documentation/dontdiff
> linux-2.6.26-Orig/sound/pci/hda/hda_intel.c
> linux-2.6.26-New/sound/pci/hda/hda_intel.c
> > --- linux-2.6.26-Orig/sound/pci/hda/hda_intel.c	2008-08-28
> 13:47:20.000000000 +0800
> > +++ linux-2.6.26-New/sound/pci/hda/hda_intel.c	2008-08-28
> 15:33:51.000000000 +0800
> > @@ -223,8 +223,8 @@ enum { SDI0, SDI1, SDI2, SDI3, SDO0, SDO
> >  #define RIRB_INT_MASK		0x05
> >  
> >  /* STATESTS int mask: SD2,SD1,SD0 */
> > -#define AZX_MAX_CODECS		3
> > -#define STATESTS_INT_MASK	0x07
> > +#define AZX_MAX_CODECS		4
> > +#define STATESTS_INT_MASK	0x0f
> 
> This may break other platforms.  Expanding STATESTS_INT_MASK is OK and
> would be harmless, but AZX_MAX_CODECS is not.
> AZX_MAX_CODECS is the default number of max codecs.  The chipset
> specific max number of codecs is defined in azx_max_codecs[].
> If Nvidia chipsets supports properly more than 3, change
> azx_max_codecs[AZX_DRIVER_NVIDIA] to 4.
> 
> ==========Wei Ni's Answer: ==============
> There are 2 codec connect to the nvidia AZA controller, one is realtek,
> and the other is nvidia HDMI codec.
> In azx_codec_create(), the chip->codec_mask indicate codec connection.
> This chip->codec_mask=0x9, bit0 indicate realtek codec, and bit3
> indicate hdmi codec.
> In this routine, it use:
> ......
> for (c = 0; c < AZX_MAX_CODECS; c++) {
> 		if ((chip->codec_mask & (1 << c)) & codec_probe_mask) {
> 			struct hda_codec *codec;
> 			err = snd_hda_codec_new(chip->bus, c, &codec);
> 			if (err < 0)
> 				continue;
> 			codecs++;
> 			if (codec->afg)
> 				audio_codecs++;
> 		}
> 	}
> if (!audio_codecs) {
> 		/* probe additional slots if no codec is found */
> 		for (; c < azx_max_codecs[chip->driver_type]; c++) {
> 			if ((chip->codec_mask & (1 << c)) &
> codec_probe_mask) {
> 				err = snd_hda_codec_new(chip->bus, c,
> NULL);
> 				if (err < 0)
> 					continue;
> 				codecs++;
> 			}
> 		}
> 	}
> ......
> According to these codes, It only can get the realtek codec and can not
> get hdmi codec, if just change azx_max_codecs[AZX_DRIVER_NVIDIA] to 4
> I think if remove "if (!audio_codecs)", it will be better, and I only
> need to chang azx_max_codecs[AZX_DRIVER_NVIDIA] to 4

Well, the check is there for a good reason.

There are many machines with broken BIOS reporting the 4th slot
available although it isn't actually.  It's a workaround to skip such
a bogus information.  So, it cannot be changed so easily.

We need to sort out this in a better way...  Oh well.


> > +static unsigned short convert_from_spdif_status_nv(unsigned int
> sbits)
(snip)
> These are almost identical with the functions in hda_codec.c.
> Is your intention to allow a different set of IEC958 status bits
> controls for HDMI *in addition* to SPDIF?  If so, we should change
> the codes in hda_codec.c to allow other control names.
> 
> ==========Wei Ni's Answer: ==============
> There are 2 codec connect to the nvidia AZA controller, one is realtek,
> the other is nvidia HDMI codec.
> If I use the default SPDIF routine: snd_hda_create_spdif_out_ctls(), the
> driver can not be insmod success.
> I traced this issue, the realtek's SPDIF used this IEC958 first, and
> then HDMI codec can't use IEC958 again.
> ( I traced it based on linux-2.6.25 )

2.6.25 is too old as a reference.  The fix for multiple SPDIF devices
is already on the recent kernel.


Takashi