[alsa-devel] [PATCH] HDA - add "Independent HP" switch for ad1988

[Raymond Yau]

2011/9/21 Takashi Iwai <tiwai at 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.