* [PATCH] HDA - add "Independent HP" switch for ad1988
@ 2011-09-17 23:18 Raymond Yau
2011-09-21 8:45 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: Raymond Yau @ 2011-09-17 23:18 UTC (permalink / raw)
To: ALSA Development Mailing List, Takashi Iwai
[-- Attachment #1: Type: text/plain, Size: 275 bytes --]
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
[-- Attachment #2: 0001-Add-Independent-HP-switch-for-ad1988.patch --]
[-- Type: application/octet-stream, Size: 8217 bytes --]
From c214e326158170c15ef762a84d88fedc7e18d8e9 Mon Sep 17 00:00:00 2001
From: Raymond Yau <superquad.vortex2@gmail.com>
Date: Sat, 17 Sep 2011 19:49:48 +0800
Subject: [PATCH] HDA - 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
Signed-off-by: Raymond Yau <superquad.vortex2@gmail.com>
diff --git a/sound/pci/hda/patch_analog.c b/sound/pci/hda/patch_analog.c
index a9b1503..e071b84 100644
--- a/sound/pci/hda/patch_analog.c
+++ b/sound/pci/hda/patch_analog.c
@@ -48,6 +48,8 @@ struct ad198x_spec {
const hda_nid_t *alt_dac_nid;
const struct hda_pcm_stream *stream_analog_alt_playback;
+ int independent_hp;
+ int num_active_streams;
/* capture */
unsigned int num_adc_nids;
@@ -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;
+ 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);
+}
+
+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;
+}
+
/*
* Analog playback callbacks
*/
@@ -310,8 +377,15 @@ static int ad198x_playback_pcm_open(struct hda_pcm_stream *hinfo,
struct snd_pcm_substream *substream)
{
struct ad198x_spec *spec = codec->spec;
- return snd_hda_multi_out_analog_open(codec, &spec->multiout, substream,
+ int err;
+ set_stream_active(codec, true);
+ err = snd_hda_multi_out_analog_open(codec, &spec->multiout, substream,
hinfo);
+ if (err < 0) {
+ set_stream_active(codec, false);
+ return err;
+ }
+ return 0;
}
static int ad198x_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
@@ -333,11 +407,41 @@ static int ad198x_playback_pcm_cleanup(struct hda_pcm_stream *hinfo,
return snd_hda_multi_out_analog_cleanup(codec, &spec->multiout);
}
+static int ad198x_playback_pcm_close(struct hda_pcm_stream *hinfo,
+ struct hda_codec *codec,
+ struct snd_pcm_substream *substream)
+{
+ set_stream_active(codec, false);
+ return 0;
+}
+
+static int ad1988_alt_playback_pcm_open(struct hda_pcm_stream *hinfo,
+ struct hda_codec *codec,
+ struct snd_pcm_substream *substream)
+{
+ struct ad198x_spec *spec = codec->spec;
+ if (!spec->independent_hp)
+ return -EBUSY;
+ set_stream_active(codec, true);
+ return 0;
+}
+
+static int ad1988_alt_playback_pcm_close(struct hda_pcm_stream *hinfo,
+ struct hda_codec *codec,
+ struct snd_pcm_substream *substream)
+{
+ set_stream_active(codec, false);
+ return 0;
+}
+
static const struct hda_pcm_stream ad198x_pcm_analog_alt_playback = {
.substreams = 1,
.channels_min = 2,
.channels_max = 2,
- /* NID is set in ad198x_build_pcms */
+ .ops = {
+ .open = ad1988_alt_playback_pcm_open,
+ .close = ad1988_alt_playback_pcm_close
+ },
};
/*
@@ -413,7 +517,8 @@ static const struct hda_pcm_stream ad198x_pcm_analog_playback = {
.ops = {
.open = ad198x_playback_pcm_open,
.prepare = ad198x_playback_pcm_prepare,
- .cleanup = ad198x_playback_pcm_cleanup
+ .cleanup = ad198x_playback_pcm_cleanup,
+ .close = ad198x_playback_pcm_close
},
};
@@ -2058,7 +2163,6 @@ static int patch_ad1981(struct hda_codec *codec)
enum {
AD1988_6STACK,
AD1988_6STACK_DIG,
- AD1988_6STACK_DIG_FP,
AD1988_3STACK,
AD1988_3STACK_DIG,
AD1988_LAPTOP,
@@ -2168,6 +2272,17 @@ static int ad198x_ch_mode_put(struct snd_kcontrol *kcontrol,
return err;
}
+static const struct snd_kcontrol_new ad1988_hp_mixers[] = {
+ {
+ .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
+ .name = "Independent HP",
+ .info = ad1988_independent_hp_info,
+ .get = ad1988_independent_hp_get,
+ .put = ad1988_independent_hp_put,
+ },
+ { } /* end */
+};
+
/* 6-stack mode */
static const struct snd_kcontrol_new ad1988_6stack_mixers1[] = {
HDA_CODEC_VOLUME("Front Playback Volume", 0x04, 0x0, HDA_OUTPUT),
@@ -2211,7 +2326,6 @@ static const struct snd_kcontrol_new ad1988_6stack_mixers2[] = {
HDA_CODEC_VOLUME("Front Mic Boost Volume", 0x39, 0x0, HDA_OUTPUT),
HDA_CODEC_VOLUME("Mic Boost Volume", 0x3c, 0x0, HDA_OUTPUT),
-
{ } /* end */
};
@@ -3147,7 +3261,6 @@ static int ad1988_auto_init(struct hda_codec *codec)
static const char * const ad1988_models[AD1988_MODEL_LAST] = {
[AD1988_6STACK] = "6stack",
[AD1988_6STACK_DIG] = "6stack-dig",
- [AD1988_6STACK_DIG_FP] = "6stack-dig-fp",
[AD1988_3STACK] = "3stack",
[AD1988_3STACK_DIG] = "3stack-dig",
[AD1988_LAPTOP] = "laptop",
@@ -3206,11 +3319,10 @@ static int patch_ad1988(struct hda_codec *codec)
set_beep_amp(spec, 0x10, 0, HDA_OUTPUT);
if (!spec->multiout.hp_nid)
- spec->multiout.hp_nid = 0x03;
+ spec->multiout.hp_nid = ad1988_alt_dac_nid[0];
switch (board_config) {
case AD1988_6STACK:
case AD1988_6STACK_DIG:
- case AD1988_6STACK_DIG_FP:
spec->multiout.max_channels = 8;
spec->multiout.num_dacs = 4;
if (is_rev2(codec))
@@ -3226,16 +3338,7 @@ static int patch_ad1988(struct hda_codec *codec)
spec->mixers[1] = ad1988_6stack_mixers2;
spec->num_init_verbs = 1;
spec->init_verbs[0] = ad1988_6stack_init_verbs;
- if (board_config == AD1988_6STACK_DIG_FP) {
- spec->multiout.hp_nid = 0;
- spec->slave_vols = ad1988_6stack_fp_slave_vols;
- spec->slave_sws = ad1988_6stack_fp_slave_sws;
- spec->alt_dac_nid = ad1988_alt_dac_nid;
- spec->stream_analog_alt_playback =
- &ad198x_pcm_analog_alt_playback;
- }
- if ((board_config == AD1988_6STACK_DIG) ||
- (board_config == AD1988_6STACK_DIG_FP)) {
+ if (board_config == AD1988_6STACK_DIG) {
spec->multiout.dig_out_nid = AD1988_SPDIF_OUT;
spec->dig_in_nid = AD1988_SPDIF_IN;
}
@@ -3278,6 +3381,15 @@ static int patch_ad1988(struct hda_codec *codec)
break;
}
+ if (spec->autocfg.hp_pins[0]) {
+ spec->mixers[spec->num_mixers++] = ad1988_hp_mixers;
+ spec->slave_vols = ad1988_6stack_fp_slave_vols;
+ spec->slave_sws = ad1988_6stack_fp_slave_sws;
+ spec->alt_dac_nid = ad1988_alt_dac_nid;
+ spec->stream_analog_alt_playback =
+ &ad198x_pcm_analog_alt_playback;
+ }
+
spec->num_adc_nids = ARRAY_SIZE(ad1988_adc_nids);
spec->adc_nids = ad1988_adc_nids;
spec->capsrc_nids = ad1988_capsrc_nids;
--
1.6.0.6
[-- Attachment #3: Type: text/plain, Size: 0 bytes --]
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] HDA - add "Independent HP" switch for ad1988
2011-09-17 23:18 [PATCH] HDA - add "Independent HP" switch for ad1988 Raymond Yau
@ 2011-09-21 8:45 ` Takashi Iwai
2011-09-22 2:32 ` Raymond Yau
0 siblings, 1 reply; 4+ messages in thread
From: Takashi Iwai @ 2011-09-21 8:45 UTC (permalink / raw)
To: Raymond Yau; +Cc: ALSA Development Mailing List
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.
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.
> + 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.
> +}
+
+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.
thanks,
Takashi
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] HDA - add "Independent HP" switch for ad1988
2011-09-21 8:45 ` Takashi Iwai
@ 2011-09-22 2:32 ` Raymond Yau
2011-09-22 10:44 ` Takashi Iwai
0 siblings, 1 reply; 4+ messages in thread
From: Raymond Yau @ 2011-09-22 2:32 UTC (permalink / raw)
To: Takashi Iwai, ALSA Development Mailing List
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"
Especially when hda reconfig is still "experiemental"
The drawback of using hda reconfig or rmmod/insmod to change the model are
1) require root privilege
2) the volume level of the controls are re-initialised by init verbs
of the model
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
This write protect should protect change of value by amixer since
amixer does not check whether the control is inactive when update the
control
The alternative is to implement a new variable in hda_codec.to control
the setup_stream and cleanup_stream of headphone dac
The "Independent HP" switch is similar to "IEC958 Default PCM" , which
use node 0x02 and device 1
9a08160bdbe3148a405f72798f76e2a5d30bd243
Tue, 12 Feb 2008 17:37:26 +0000 (18:37 +0100)
[ALSA] hda-codec - Add "IEC958 Default PCM" switch
Added a new mixer switch to enable/disable the sharing of the default
PCM stream with analog and SPDIF outputs. When "IEC958 Default PCM"
switch is on, the PCM stream is routed both to analog and SPDIF outputs.
This is the behavior in the earlier version.
Turning this switch off has a merit for some codecs, though. Some codec
chips don't support 24bit formats for SPDIF but only for analog outputs.
In this case, you can use 24bit format by disabling this switch.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] HDA - add "Independent HP" switch for ad1988
2011-09-22 2:32 ` Raymond Yau
@ 2011-09-22 10:44 ` Takashi Iwai
0 siblings, 0 replies; 4+ messages in thread
From: Takashi Iwai @ 2011-09-22 10:44 UTC (permalink / raw)
To: Raymond Yau; +Cc: ALSA Development Mailing List
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
> 1) require root privilege
> 2) 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-09-22 10:44 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-09-17 23:18 [PATCH] HDA - add "Independent HP" switch for ad1988 Raymond Yau
2011-09-21 8:45 ` Takashi Iwai
2011-09-22 2:32 ` Raymond Yau
2011-09-22 10:44 ` Takashi Iwai
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.