Re: patch to support Samsung NC10 mini notebook

[Takashi Iwai <tiwai@suse.de>]

At Sun, 10 May 2009 21:21:33 +0200,
Chris Pockele wrote:
> 
> Hello,
> 
> The attached file is a patch to support the sound hardware in the
> Samsung NC10 mini notebook.
> The Samsung NC10 is and Intel Atom-based notebook with Realtek ALC272
> HDA codecs.
> Sound is supported partly in Linux depending on ALSA version, but
> there were always some issues (wrong mixer behaviour, PC beep always
> muted, ...). This patch tries to solve most of these issues.
> I updated the patch so it would work using the alsa-driver-20090509
> snapshot from ftp.kernel.org/pub/linux/kernel/people/tiwai/snapshot/
> (originally developed from alsa-driver-20090416 snapshot).

Thanks for the patch!
After a brief look, the changes look good.  But there are some issues
to be fixed.  See some comments below.

> Fixes:
>  - mute internal speaker / microphone when plugging in external
> headphone / microphone
>  - stereo capture from external microphone
>  - separate volume controls for headphone and internal speakers +
> "Master" for both
>  - "PC Beep" mixer control
>  - "Mic boost" and "External mic boost" controls
>  - ...
> 
> A few issues remaining:
>  - Recording PCM output does not work yet. It works on the version of
> Windows that comes with the NC10.
>  - When choosing recording source "Internal Mic" while an external
> microphone is plugged in, the internal microphone will be used instead
> of the external one, as it should. However, when plugging in the
> external microphone after choosing the recording source setting, it
> WILL override the internal microphone.

First off, please give a text as a change log together with your
sign-off so that the patch can be merged to the upstream.


> diff -ru alsa-driver/alsa-kernel/pci/hda/patch_realtek.c alsa-driver-20090509-MOD/alsa-kernel/pci/hda/patch_realtek.c
> --- alsa-driver/alsa-kernel/pci/hda/patch_realtek.c	2009-05-09 00:05:02.000000000 +0200
> +++ alsa-driver-20090509-MOD/alsa-kernel/pci/hda/patch_realtek.c	2009-05-10 13:07:22.000000000 +0200
> @@ -190,6 +190,7 @@
>  	ALC663_ASUS_MODE6,
>  	ALC272_DELL,
>  	ALC272_DELL_ZM1,
> +	ALC272_SAMSUNG_NC10,	/*20090509 Samsung NC10 support - chris.pockele.f1@gmail.com*/

This kind of comment isn't needed because we track all changes via GIT.

> +#ifndef CONFIG_SND_DEBUG
> +static struct hda_input_mux alc272_nc10_capture_source = {
> +	.num_items = 2,
> +	.items = {
> +		{ "Autoselect Mic", 0x0 },
> +		{ "Internal Mic", 0x1 },
> +	},
> +};
> +#else
> +static struct hda_input_mux alc272_nc10_capture_source = {

CONFIG_SND_DEBUG is set on some distros as default, so the driver
shouldn't change the fundamental behavior by that option.
If you'd like to keep the test code, just comment out with some other
ifdef.   Even an ifdef 0 with a comment would be OK.


> +static struct snd_kcontrol_new alc272_nc10_mixer[] = {
> +	/* Master Playback automatically created from Speaker and Headphone */
> +	HDA_CODEC_VOLUME("Speaker Playback Volume", 0x02, 0x0, HDA_OUTPUT),
> +	HDA_CODEC_MUTE("Speaker Playback Switch", 0x14, 0x0, HDA_OUTPUT),
> +	HDA_CODEC_VOLUME("Headphone Playback Volume", 0x03, 0x0, HDA_OUTPUT),
> +	HDA_CODEC_MUTE("Headphone Playback Switch", 0x21, 0x0, HDA_OUTPUT),
> +
> +	HDA_CODEC_VOLUME("Ext Mic Playback Volume", 0x0b, 0x0, HDA_INPUT),
> +	HDA_CODEC_MUTE("Ext Mic Playback Switch", 0x0b, 0x0, HDA_INPUT),
> +	HDA_CODEC_VOLUME("Ext Mic Boost Playback Volume", 0x18, 0, HDA_INPUT),

The mic boost is for both playback and capture.  Thus, simply name it as
"... Mic Boost".

> +	HDA_CODEC_VOLUME("Int Mic Playback Volume", 0x0b, 0x1, HDA_INPUT),
> +	HDA_CODEC_MUTE("Int Mic Playback Switch", 0x0b, 0x1, HDA_INPUT),
> +	HDA_CODEC_VOLUME("Int Mic Boost Playback Volume", 0x19, 0, HDA_INPUT),

Ditto.

> +};
> +
>  #ifdef CONFIG_SND_HDA_POWER_SAVE
>  #define alc662_loopbacks	alc880_loopbacks
>  #endif
> @@ -16184,6 +16234,9 @@
>  	[ALC663_ASUS_MODE4] = "asus-mode4",
>  	[ALC663_ASUS_MODE5] = "asus-mode5",
>  	[ALC663_ASUS_MODE6] = "asus-mode6",
> +	[ALC272_DELL] = "dell272",
> +	[ALC272_DELL_ZM1] = "dell-zm1",

These models have been already added pretty recently.
Please rebase your patch with the latest snapshot.


> +	[ALC272_SAMSUNG_NC10] = "samsung-nc10",

Also, don't forget to update
Documentation/sound/alsa/HD-Audio-Models.txt, too.


Could you fix the issues above and repost?

thanks,

Takashi