Re: [PATCH] HDA - add "Independent HP" switch for ad1988

[Takashi Iwai <tiwai@suse.de>]

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