At Thu, 22 Sep 2011 10:32:24 +0800, Raymond Yau wrote:
2011/9/21 Takashi Iwai tiwai@suse.de:
At Sun, 18 Sep 2011 07:18:28 +0800, Raymond Yau wrote:
Add "Independent HP" switch for ad1988
- add playback device 2 "AD1988 HP" for 7.1+2 multi streaming playback
- add "Independent HP" switch to enable/disable the feature
switch is inactive and write protect when device 0 or device 2 is opened
- remove 6stack-dig-fp model
Any rationale to remove this model? I'm fine with the removal, but need to know the reason.
After the previous patch
"Add Headphone Playback Volume" for ad1988 -use DAC0 instead of DAC1 for Port -A headphone
All models already have a "Headphone Playback Volume" control by using DAC0 node 0x3 instead of sharing DAC1 node 0x4 (HDA_FRONT) with Port B
- green jack
int snd_hda_multi_out_analog_prepare(struct hda_codec *codec, struct hda_multi_out *mout, unsigned int stream_tag, unsigned int format, struct snd_pcm_substream *substream) if (!mout->no_share_stream && mout->hp_nid && mout->hp_nid != nids[HDA_FRONT]) /* headphone out will just decode front left/right (stereo) */ snd_hda_codec_setup_stream(codec, mout->hp_nid, stream_tag, 0, format);
In this patch, playback device 2 (AD198x Headphone) is added to all models of ad1988/ad1989 so there is no need to keep the model "6stack-dig-fp"
OK, then please describe this reason shortly in the log, too.
Especially when hda reconfig is still "experiemental"
The drawback of using hda reconfig or rmmod/insmod to change the model are
- require root privilege
- the volume level of the controls are re-initialised by init verbs
of the model
Yes, the reconfiguration is for development, not for the production usage.
I don't like "6stack-dig-fp" model because "6stack-dig" create a digital input device which is not present in my motherboard.
Also, regarding the patch:
@@ -302,6 +304,71 @@ static int ad198x_check_power_status(struct hda_codec *codec, hda_nid_t nid) } #endif
+static void activate_ctl(struct hda_codec *codec, const char *name, int active) +{
- struct snd_kcontrol *ctl = snd_hda_find_mixer_ctl(codec, name);
- if (ctl) {
- ctl->vd[0].access &= ~SNDRV_CTL_ELEM_ACCESS_INACTIVE;
- ctl->vd[0].access |= active ? 0:SNDRV_CTL_ELEM_ACCESS_INACTIVE;
- ctl->vd[0].access &= ~SNDRV_CTL_ELEM_ACCESS_WRITE;
Please use tabs.
OK
- ctl->vd[0].access |= active ? SNDRV_CTL_ELEM_ACCESS_WRITE:0;
- snd_ctl_notify(codec->bus->card,
- SNDRV_CTL_EVENT_MASK_INFO, &ctl->id);
- }
+}
- static void set_stream_active(struct hda_codec *codec, bool active)
+{
- struct ad198x_spec *spec = codec->spec;
- if (active)
- spec->num_active_streams++;
- else
- spec->num_active_streams--;
- activate_ctl(codec, "Independent HP", spec->num_active_streams == 0);
- printk(KERN_INFO "set stream active : active stream %d \n", spec->num_active_streams);
Avoid debug prints in the final code. If any, use snd_printdd() or such.
I will remove this statement as I already know the cause of codec cleanup function is called twice even in non sticky pcm mode
+}
+static int ad1988_independent_hp_info(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_info *uinfo)
+{
- static const char * const texts[] = { "OFF", "ON", NULL};
- int index;
- uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
- uinfo->count = 1;
- uinfo->value.enumerated.items = 2;
- index = uinfo->value.enumerated.item;
- if (index >= 2)
- index = 1;
- strcpy(uinfo->value.enumerated.name, texts[index]);
- return 0;
+}
+static int ad1988_independent_hp_get(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct ad198x_spec *spec = codec->spec;
- ucontrol->value.enumerated.item[0] = spec->independent_hp;
- return 0;
+}
+static int ad1988_independent_hp_put(struct snd_kcontrol *kcontrol,
- struct snd_ctl_elem_value *ucontrol)
+{
- struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
- struct ad198x_spec *spec = codec->spec;
- unsigned int select = ucontrol->value.enumerated.item[0];
- if (spec->independent_hp != select) {
- spec->independent_hp = select;
- if (spec->independent_hp)
- spec->multiout.hp_nid = 0;
- else
- spec->multiout.hp_nid = spec->alt_dac_nid[0];
- return 1;
- }
- return 0;
+}
Changing spec->multiout.hp_nid dynamically here is racy. If the value is changed during the PCM stream is opened, the HP-setup might be kept inconsistent at close.
Do you mean set_stream_active() check in ad198x_playback_pcm_open(), ad1988_alt_playback_pcm_open() , ad198x_playback_pcm_close() and ad1988_alt_playback_pcm_close()
ctl->vd[0].access &= ~SNDRV_CTL_ELEM_ACCESS_WRITE;
still not enough to protect the value of the control
Ah, OK, that'll protect. But it means that you cannot change the value during the PCM stream is opened. It's a drawback, too. But, I don't mind in either way. Maybe the current protection is easy enough.
Takashi