Proposed changes to azx/patch_realtek.c

Proposed changes to azx/patch_realtek.c

[Stephen Warren]

[-- Attachment #1: Type: text/plain, Size: 2415 bytes --]

Hello.

I'm working on updating the Azalia audio driver (ALC880 codec) to
support a new PCB with different routing of codec pins to connectors.

As part of this, I took a look at patch_realtek.c and have a few
suggestions on how to simplify things.

As an example of this, I've attached a patch. This isolates and
knowledge of the "board type" to the patch_alc880 function. All
decisions based on the board type are made here, and written into
configuration variables in the spec structure. Then, all other functions
simply pull values out of the spec structure and use them, so they don't
have to have a bunch of code that's conditional on board type, which
could grow unmanageably.

See the attached patch for this change.

Going forward, I'd like to make the following changes:

1)

patch_alc880 fills in the spec structure at run-time. Instead, I'd like
a set of pre-filled-in structures to be part of the driver, and
patch_alc880 just initializes codec->spec to point at one (just like it
currently initializes spec->init_seq to point at a specific list of
initialization instructions with my patch).

Benefit: Can support new boards simply by defining a new pre-defined
data structure and ALC880_xxx enumeration value - no need to actually
edit code in patch_alc880 for this.

2)

Unify 880 and 260 support.

Much of the code for 880 and 260 is very similar - for example,
alc880_build_controls and alc260_build_controls are the same function,
except that (if we use just the 880 code) we would hard-code
spec->build_side_mixer=0 and spec->digital=0, and have to parameterize
the function by putting a pointer to either alc880_base_mixer or
alc260_base_mixer into the spec structure.

Benefit: Identical code for 880 and 260 reduces duplication. Can
configure differences between codecs/boards just by editing the
pre-initialized spec structure values.

I expect to have to define my own xxx_base_mixer function for the PCB
I'm supporting.

3)

Finally add support for my PCB by defining new data structures using the
above features.

Does this all sound like a good idea, and is it something that could be
merged back into the official driver?

Thanks for any comments.

-- 
Stephen Warren, Software Engineer, NVIDIA, Fort Collins, CO
swarren@nvidia.com        http://www.nvidia.com/
swarren@wwwdotorg.org     http://www.wwwdotorg.org/pgp.html

[-- Attachment #2: realtek.diff --]
[-- Type: application/octet-stream, Size: 3021 bytes --]

--- patch_realtek.c.orig	2004-12-15 04:30:47.887441016 -0700
+++ patch_realtek.c	2004-12-15 04:46:56.520186456 -0700
@@ -39,10 +39,12 @@
 };
 
 struct alc_spec {
-	int board_config;
 	struct hda_input_mux *input_mux;
 	unsigned int front_panel: 1;
 	unsigned int digital: 1;
+        unsigned int build_side_mixer: 1;
+        unsigned int channels_max;
+        const struct hda_verb *init_seq;
 	unsigned int num_dac_nids;
 	hda_nid_t *dac_nids;
 	unsigned int num_adc_nids;
 static struct alc_channel_mode fivestack_modes = {
 	.channels = { 6, 8 },
 	.sequences = { fivestack_ch6_init, fivestack_ch8_init }
@@ -361,7 +363,7 @@
 				   ARRAY_SIZE(alc880_base_mixer));
 	if (err < 0)
 		return err;
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG) {
+	if (spec->build_side_mixer) {
 		err = patch_build_controls(codec, alc880_side_mixer,
 					   ARRAY_SIZE(alc880_side_mixer));
 		if (err < 0)
@@ -533,10 +535,9 @@
 static int alc880_init(struct hda_codec *codec)
 {
 	struct alc_spec *spec = codec->spec;
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG)
-		snd_hda_sequence_write(codec, alc880_five_stack_volume);
-	else
-		snd_hda_sequence_write(codec, alc880_three_stack_volume);
+
+	snd_hda_sequence_write(codec, spec->init_seq);
+
 	return 0;
 }
 
@@ -693,11 +694,7 @@
 	info->name = "ALC880 Analog";
 	info->stream[SNDRV_PCM_STREAM_PLAYBACK] = alc880_pcm_analog_playback;
 	info->stream[SNDRV_PCM_STREAM_CAPTURE] = alc880_pcm_analog_capture;
-
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG)
-		info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max = 8;
-	else
-		info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max = 6;
+	info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max = spec->channels_max;
 
 	return 0;
 }
@@ -791,14 +788,17 @@
 static int patch_alc880(struct hda_codec *codec)
 {
 	struct alc_spec *spec;
+	int board_config;
 
 	spec = kcalloc(1, sizeof(*spec), GFP_KERNEL);
 	if (spec == NULL)
 		return -ENOMEM;
 
 	codec->spec = spec;
-	spec->board_config = snd_hda_check_board_config(codec, alc880_cfg_tbl);
-	switch (spec->board_config) {
+
+	board_config = snd_hda_check_board_config(codec, alc880_cfg_tbl);
+
+	switch (board_config) {
 	case ALC880_3ST_DIG:
 	case ALC880_5ST_DIG:
 		spec->digital = 1;
@@ -809,12 +809,25 @@
 		spec->front_panel = 1;
 		break;
 	}
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG) {
+
+	switch (board_config) {
+	case ALC880_5ST:
+	case ALC880_5ST_DIG:
 		spec->channel_setting = 6;
+		spec->channels_max = 8;
 		spec->channel_mode = &fivestack_modes;
-	} else {
+		spec->build_side_mixer = 1;
+		spec->init_seq = alc880_five_stack_volume;
+		break;
+	case ALS880_3ST:
+	case ALS880_5ST:
+	default:
 		spec->channel_setting = 2;
+		spec->channels_max = 6;
 		spec->channel_mode = &threestack_modes;
+		spec->build_side_mixer = 0;
+		spec->init_seq = alc880_three_stack_volume;
+		break;
 	}
 
 	spec->input_mux = &alc880_capture_source;

Re: Proposed changes to azx/patch_realtek.c

[Takashi Iwai]

At Wed, 15 Dec 2004 09:21:25 -0800,
Stephen Warren wrote:
> 
> Hello.
> 
> I'm working on updating the Azalia audio driver (ALC880 codec) to
> support a new PCB with different routing of codec pins to connectors.
> 
> As part of this, I took a look at patch_realtek.c and have a few
> suggestions on how to simplify things.
> 
> As an example of this, I've attached a patch. This isolates and
> knowledge of the "board type" to the patch_alc880 function. All
> decisions based on the board type are made here, and written into
> configuration variables in the spec structure. Then, all other functions
> simply pull values out of the spec structure and use them, so they don't
> have to have a bunch of code that's conditional on board type, which
> could grow unmanageably.
> 
> See the attached patch for this change.

Sorry, your patch can't be applied.  I rewrote the azx and hda_codec
codes largely again, committed to CVS shortly before.  The new code is
a bit better than the older one in this regard.

> Going forward, I'd like to make the following changes:
> 
> 1)
> 
> patch_alc880 fills in the spec structure at run-time. Instead, I'd like
> a set of pre-filled-in structures to be part of the driver, and
> patch_alc880 just initializes codec->spec to point at one (just like it
> currently initializes spec->init_seq to point at a specific list of
> initialization instructions with my patch).
> 
> Benefit: Can support new boards simply by defining a new pre-defined
> data structure and ALC880_xxx enumeration value - no need to actually
> edit code in patch_alc880 for this.

This sounds reasonable.

> 2)
> 
> Unify 880 and 260 support.
> 
> Much of the code for 880 and 260 is very similar - for example,
> alc880_build_controls and alc260_build_controls are the same function,
> except that (if we use just the 880 code) we would hard-code
> spec->build_side_mixer=0 and spec->digital=0, and have to parameterize
> the function by putting a pointer to either alc880_base_mixer or
> alc260_base_mixer into the spec structure.
> 
> Benefit: Identical code for 880 and 260 reduces duplication. Can
> configure differences between codecs/boards just by editing the
> pre-initialized spec structure values.
> 
> I expect to have to define my own xxx_base_mixer function for the PCB
> I'm supporting.

Yes, I thought of that, too.  That's why struct alc_spec is shared
between these codecs.  I left it as it is simply because I have no
test board for ALC260.

Go ahead, clean up as you like ;)


Takashi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/

RE: Proposed changes to azx/patch_realtek.c

[Stephen Warren]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

From: Takashi Iwai [mailto:tiwai@suse.de] 
> Sorry, your patch can't be applied.  I rewrote the azx and hda_codec
> codes largely again, committed to CVS shortly before.  The new code is
> a bit better than the older one in this regard.

Oops. My editor strips trailing white-space from lines, so I tried to
edit the patch file manually to remove those parts - evidently I broke
it!

Anyway, the attached two patches are:
1) The white-space changes
2) My real changes.

Hopefully these should apply!

When you say there's a new driver in CVS, do you mean a new one in the
last day (i.e. since you first rewrote it) or are you simply referring
to your first rewrite.

Thanks.

-- 
Stephen Warren, Software Engineer, NVIDIA, Fort Collins, CO
swarren@nvidia.com        http://www.nvidia.com/
swarren@wwwdotorg.org     http://www.wwwdotorg.org/pgp.html

[-- Attachment #2: diff.1 --]
[-- Type: application/octet-stream, Size: 3376 bytes --]

--- patch_realtek.c.0	2004-12-15 04:30:47.887441016 -0700
+++ patch_realtek.c.1	2004-12-15 05:14:51.098611840 -0700
@@ -1,6 +1,6 @@
 /*
  * Universal Interface for Intel High Definition Audio Codec
- * 
+ *
  * HD audio interface patch for ALC 260/880 codecs
  *
  * Copyright (c) 2004 PeiSen Hou <pshou@realtek.com.tw>
@@ -100,7 +100,7 @@
 	struct alc_spec *spec = codec->spec;
 	return snd_hda_input_mux_info(spec->input_mux, uinfo);
 }
- 
+
 static int alc_mux_enum_get(snd_kcontrol_t *kcontrol, snd_ctl_elem_value_t *ucontrol)
 {
 	struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
@@ -232,7 +232,7 @@
 	{ 0x1a, AC_VERB_SET_AMP_GAIN_MUTE, 0xb000 },
 	{ } /* end */
 };
-	
+
 static struct alc_channel_mode fivestack_modes = {
 	.channels = { 6, 8 },
 	.sequences = { fivestack_ch6_init, fivestack_ch8_init }
@@ -726,19 +726,19 @@
 	{ .pci_vendor = 0x8086, .pci_device = 0xe200, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe201, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe202, .config = ALC880_3ST },
-	{ .pci_vendor = 0x8086, .pci_device = 0xe203, .config = ALC880_3ST }, 
+	{ .pci_vendor = 0x8086, .pci_device = 0xe203, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe204, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe205, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe206, .config = ALC880_3ST },
-	{ .pci_vendor = 0x8086, .pci_device = 0xe207, .config = ALC880_3ST }, 
+	{ .pci_vendor = 0x8086, .pci_device = 0xe207, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe208, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe209, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe20a, .config = ALC880_3ST },
-	{ .pci_vendor = 0x8086, .pci_device = 0xe20b, .config = ALC880_3ST }, 
+	{ .pci_vendor = 0x8086, .pci_device = 0xe20b, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe20c, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe20d, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe20e, .config = ALC880_3ST },
-	{ .pci_vendor = 0x8086, .pci_device = 0xe20f, .config = ALC880_3ST }, 
+	{ .pci_vendor = 0x8086, .pci_device = 0xe20f, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe210, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe211, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xe214, .config = ALC880_3ST },
@@ -750,11 +750,11 @@
 	{ .pci_vendor = 0x8086, .pci_device = 0xe404, .config = ALC880_3ST },
 	{ .pci_vendor = 0x8086, .pci_device = 0xa101, .config = ALC880_3ST },
 	{ .pci_vendor = 0x107b, .pci_device = 0x3031, .config = ALC880_3ST },
-	{ .pci_vendor = 0x107b, .pci_device = 0x4036, .config = ALC880_3ST },   	
+	{ .pci_vendor = 0x107b, .pci_device = 0x4036, .config = ALC880_3ST },
 	{ .pci_vendor = 0x107b, .pci_device = 0x4037, .config = ALC880_3ST },
 	{ .pci_vendor = 0x107b, .pci_device = 0x4038, .config = ALC880_3ST },
 	{ .pci_vendor = 0x107b, .pci_device = 0x4040, .config = ALC880_3ST },
-	{ .pci_vendor = 0x107b, .pci_device = 0x4041, .config = ALC880_3ST }, 
+	{ .pci_vendor = 0x107b, .pci_device = 0x4041, .config = ALC880_3ST },
 
 	/* Back 3 jack, front 2 jack (Internal add Aux-In) */
 	{ .pci_vendor = 0x1025, .pci_device = 0xe310, .config = ALC880_3ST },

[-- Attachment #3: diff.2 --]
[-- Type: application/octet-stream, Size: 2865 bytes --]

--- patch_realtek.c.1	2004-12-15 05:14:51.098611840 -0700
+++ patch_realtek.c.2	2004-12-15 05:15:45.039411592 -0700
@@ -39,10 +39,12 @@
 };
 
 struct alc_spec {
-	int board_config;
 	struct hda_input_mux *input_mux;
 	unsigned int front_panel: 1;
 	unsigned int digital: 1;
+	unsigned int build_side_mixer: 1;
+	unsigned int channels_max;
+	const struct hda_verb *init_seq;
 	unsigned int num_dac_nids;
 	hda_nid_t *dac_nids;
 	unsigned int num_adc_nids;
@@ -361,7 +363,7 @@
 				   ARRAY_SIZE(alc880_base_mixer));
 	if (err < 0)
 		return err;
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG) {
+	if (spec->build_side_mixer) {
 		err = patch_build_controls(codec, alc880_side_mixer,
 					   ARRAY_SIZE(alc880_side_mixer));
 		if (err < 0)
@@ -533,10 +535,9 @@
 static int alc880_init(struct hda_codec *codec)
 {
 	struct alc_spec *spec = codec->spec;
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG)
-		snd_hda_sequence_write(codec, alc880_five_stack_volume);
-	else
-		snd_hda_sequence_write(codec, alc880_three_stack_volume);
+
+	snd_hda_sequence_write(codec, spec->init_seq);
+
 	return 0;
 }
 
@@ -693,11 +694,7 @@
 	info->name = "ALC880 Analog";
 	info->stream[SNDRV_PCM_STREAM_PLAYBACK] = alc880_pcm_analog_playback;
 	info->stream[SNDRV_PCM_STREAM_CAPTURE] = alc880_pcm_analog_capture;
-
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG)
-		info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max = 8;
-	else
-		info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max = 6;
+	info->stream[SNDRV_PCM_STREAM_PLAYBACK].channels_max = spec->channels_max;
 
 	return 0;
 }
@@ -791,14 +788,17 @@
 static int patch_alc880(struct hda_codec *codec)
 {
 	struct alc_spec *spec;
+	int board_config;
 
 	spec = kcalloc(1, sizeof(*spec), GFP_KERNEL);
 	if (spec == NULL)
 		return -ENOMEM;
 
 	codec->spec = spec;
-	spec->board_config = snd_hda_check_board_config(codec, alc880_cfg_tbl);
-	switch (spec->board_config) {
+
+	board_config = snd_hda_check_board_config(codec, alc880_cfg_tbl);
+
+	switch (board_config) {
 	case ALC880_3ST_DIG:
 	case ALC880_5ST_DIG:
 		spec->digital = 1;
@@ -809,12 +809,25 @@
 		spec->front_panel = 1;
 		break;
 	}
-	if (spec->board_config == ALC880_5ST || spec->board_config == ALC880_5ST_DIG) {
+
+	switch (board_config) {
+	case ALC880_5ST:
+	case ALC880_5ST_DIG:
 		spec->channel_setting = 6;
+		spec->channels_max = 8;
 		spec->channel_mode = &fivestack_modes;
-	} else {
+		spec->build_side_mixer = 1;
+		spec->init_seq = alc880_five_stack_volume;
+		break;
+	case ALS880_3ST:
+	case ALS880_5ST:
+	default:
 		spec->channel_setting = 2;
+		spec->channels_max = 6;
 		spec->channel_mode = &threestack_modes;
+		spec->build_side_mixer = 0;
+		spec->init_seq = alc880_three_stack_volume;
+		break;
 	}
 
 	spec->input_mux = &alc880_capture_source;

Re: Proposed changes to azx/patch_realtek.c

[Takashi Iwai]

At Wed, 15 Dec 2004 09:45:07 -0800,
Stephen Warren wrote:
> 
> From: Takashi Iwai [mailto:tiwai@suse.de] 
> > Sorry, your patch can't be applied.  I rewrote the azx and hda_codec
> > codes largely again, committed to CVS shortly before.  The new code is
> > a bit better than the older one in this regard.
> 
> Oops. My editor strips trailing white-space from lines, so I tried to
> edit the patch file manually to remove those parts - evidently I broke
> it!

Ah, maybe I misled you.

I couldn't apply your patch just because I had changed the codes
before I got your patches.  Please check the latest code (again after
anon cvs tree is sync'ed...)

> Anyway, the attached two patches are:
> 1) The white-space changes
> 2) My real changes.
> 
> Hopefully these should apply!
> 
> When you say there's a new driver in CVS, do you mean a new one in the
> last day (i.e. since you first rewrote it) or are you simply referring
> to your first rewrite.

No, the changes were committed today, sometime afternoon CET.
You can see a new file hda_local.h in the latest state.

I'd recommend you to subscribe to alsa-cvs ML.  Then you'll receive
the changes on CVS tree (as patches) almost in real time.


thanks,

Takashi


-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users.
Discover which products truly live up to the hype. Start reading now. 
http://productguide.itmanagersjournal.com/