[alsa-devel] [PATCH 2/7] ALSA: CA0132: Add DSP mixers, switches and controls

[Takashi Iwai]

At Fri, 7 Dec 2012 21:35:58 -0800,
Ian Minett wrote:
> 
> From: Ian Minett <ian_minett at creativelabs.com>
> 
> Add the new switches, virtual nodes, controls and parameters used for 
> configuring the firmware DSP.
> Update callbacks and other routines to set up the correct streams and 
> use the new effect switches.
> Add verb tables for DSP init.
> 
> Signed-off-by: Ian Minett <ian_minett at creativelabs.com>

OK, this is a big change in a single shot, and it makes harder to
follow the code.

Could you split this to a several patches?  The change needed for the
headphone/speaker control and the change for the mic are basically
independent.  Also, the enhancement of PCM (e.g. max_channels became 6
now) is also independent from other changes.


> @@ -893,6 +866,8 @@ static int chipio_write_data(struct hda_codec *codec, unsigned int data)
>  				  data >> 16);
>  	}
>  
> +	spec->curr_chip_addx = (res != -EIO) ?
> +					(spec->curr_chip_addx + 4) : ~0UL;

What does this do at all?  A bit more description please.
I suppose it increments the address automatically for the word.


> @@ -2626,17 +2614,63 @@ static bool dspload_wait_loaded(struct hda_codec *codec)
>  	CA0132_CODEC_MUTE_MONO(xname, nid, 3, dir)
>  
>  /*
> + * PCM stuffs
> + */
> +static void ca0132_setup_stream(struct hda_codec *codec, hda_nid_t nid,
> +				 u32 stream_tag,
> +				 int channel_id, int format)
> +{
> +	unsigned int oldval, newval;
> +
> +	if (!nid)
> +		return;
> +
> +	snd_printdd(
> +		   "ca0132_setup_stream: NID=0x%x, stream=0x%x, "
> +		   "channel=%d, format=0x%x\n",
> +		   nid, stream_tag, channel_id, format);
> +
> +	/* update the format-id if changed */
> +	oldval = snd_hda_codec_read(codec, nid, 0,
> +				    AC_VERB_GET_STREAM_FORMAT,
> +				    0);
> +	if (oldval != format) {
> +		msleep(20);
> +		snd_hda_codec_write(codec, nid, 0,
> +				    AC_VERB_SET_STREAM_FORMAT,
> +				    format);
> +	}
> +
> +	oldval = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONV, 0);
> +	newval = (stream_tag << 4) | channel_id;
> +	if (oldval != newval) {
> +		snd_hda_codec_write(codec, nid, 0,
> +				    AC_VERB_SET_CHANNEL_STREAMID,
> +				    newval);
> +	}
> +}
> +
> +static void ca0132_cleanup_stream(struct hda_codec *codec, hda_nid_t nid)
> +{
> +	unsigned int val;
> +
> +	if (!nid)
> +		return;
> +
> +	snd_printdd(KERN_INFO "ca0132_cleanup_stream: NID=0x%x\n", nid);
> +
> +	val = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_CONV, 0);
> +	if (!val)
> +		return;
> +
> +	snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_STREAM_FORMAT, 0);
> +	snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_CHANNEL_STREAMID, 0);
> +}

Do we still need these two special functions?
In the recent version, I already fixed the common helper
snd_hda_multi_out_analog_*() for the case like ca0132.


> @@ -2653,7 +2689,16 @@ static int ca0132_playback_pcm_cleanup(struct hda_pcm_stream *hinfo,
>  			struct snd_pcm_substream *substream)
>  {
>  	struct ca0132_spec *spec = codec->spec;
> -	return snd_hda_multi_out_analog_cleanup(codec, &spec->multiout);
> +
> +	if (spec->dsp_state == DSP_DOWNLOADING)
> +		return 0;
> +
> +	/*Allow stream some time to flush effects tail*/
> +	msleep(50);

This is a long stall.  Do we need this unconditionally?


> +static int ca0132_select_out(struct hda_codec *codec)
> +{
> +	struct ca0132_spec *spec = codec->spec;
> +	unsigned int pin_ctl;
> +	int jack_present;
> +	int auto_jack;
> +	unsigned int tmp;
> +	int err;
> +
> +	snd_printdd(KERN_INFO "ca0132_select_out\n");
> +
> +	snd_hda_power_up(codec);
> +
> +	auto_jack = spec->vnode_lswitch[VNID_HP_ASEL - VNODE_START_NID];
> +
> +	if (auto_jack)
> +		jack_present = snd_hda_jack_detect(codec, spec->out_pins[1]);
> +	else
> +		jack_present =
> +			spec->vnode_lswitch[VNID_HP_SEL - VNODE_START_NID];
> +
> +	if (jack_present)
> +		spec->cur_out_type = HEADPHONE_OUT;
> +	else
> +		spec->cur_out_type = SPEAKER_OUT;
> +
> +	if (spec->cur_out_type == SPEAKER_OUT) {
> +		snd_printdd(KERN_INFO "ca0132_select_out speaker\n");
> +		/*speaker out config*/
> +		tmp = FLOAT_ONE;
> +		err = dspio_set_param(codec, 0x80, 0x04,
> +					&tmp, sizeof(unsigned int));

Looking through the whole code, dspio_set_param() is called only for a
32bit int.  Define the simplified version like:

static int dspio_set_int_param(struct hda_codec *codec, int mod_id,
				int req, unsigned int val)
{
	return dspio_set_param(codec, mod_id, req, &val, sizeof
}


> +/*
> + * When changing Node IDs for Mixer Controls below, make sure to update
> + * Node IDs in ca0132_config() as well.
> + */
> +static struct snd_kcontrol_new ca0132_mixer[] = {
> +	CA0132_CODEC_VOL("Master Playback Volume", VNID_SPK, HDA_OUTPUT),
> +	CA0132_CODEC_MUTE("Master Playback Switch", VNID_SPK, HDA_OUTPUT),
> +	CA0132_CODEC_VOL("Capture Volume", VNID_MIC, HDA_INPUT),
> +	CA0132_CODEC_MUTE("Capture Switch", VNID_MIC, HDA_INPUT),
> +	HDA_CODEC_VOLUME("Analog-Mic2 Capture Volume", 0x08, 0, HDA_INPUT),
> +	HDA_CODEC_MUTE("Analog-Mic2 Capture Switch", 0x08, 0, HDA_INPUT),
> +	HDA_CODEC_VOLUME("What U Hear Capture Volume", 0x0a, 0, HDA_INPUT),
> +	HDA_CODEC_MUTE("What U Hear Capture Switch", 0x0a, 0, HDA_INPUT),
> +	CA0132_CODEC_MUTE_MONO("Mic1-Boost (30dB) Capture Switch",
> +			       0x12, 1, HDA_INPUT),
> +	CA0132_CODEC_MUTE_MONO("HP/Speaker Playback Switch",
> +			       VNID_HP_SEL, 1, HDA_OUTPUT),

This allows user to choose which output is used, right?  Then this
should be an enum.  Otherwise it's not clear what it serves for.


> +	CA0132_CODEC_MUTE_MONO("AMic1/DMic Capture Switch",
> +			       VNID_AMIC1_SEL, 1, HDA_INPUT),

Ditto.


> +	CA0132_CODEC_MUTE_MONO("HP/Speaker Auto Detect Playback Switch",
> +			       VNID_HP_ASEL, 1, HDA_OUTPUT),

In other codec drivers, it's called as "Auto-Mute Mode".


> +	CA0132_CODEC_MUTE_MONO("AMic1/DMic Auto Detect Capture Switch",
> +			       VNID_AMIC1_ASEL, 1, HDA_INPUT),

There is no standard name for this yet, though...

I'll look into more details once when the patch is split.  Otherwise
it's really difficult to follow all changes.


thanks,

Takashi