[alsa-devel] [PATCH] snd-hda-intel: Jack Mode changes for Sigmatel boards

[Takashi Iwai]

Hi,

sorry for the late review.  Some comments below.

At Wed, 20 May 2009 12:35:41 +0000,
Nickolas Lloyd wrote:
> 
> +static int stac92xx_dc_bias_info(struct snd_kcontrol *kcontrol,
> +                struct snd_ctl_elem_info *uinfo)
> +{
> +    int i;
> +    static char *texts[] = {
> +        "Mic In", "Line In", "Line Out"
> +    };
> +
> +    if (kcontrol->private_value & 0x100)

Please avoid a magic number.  Define a constant if needed.

> +        i = 3;
> +    else
> +        i = 2;
> +
> +    uinfo->type = SNDRV_CTL_ELEM_TYPE_ENUMERATED;
> +    uinfo->value.enumerated.items = i;
> +    uinfo->count = 1;
> +    if (uinfo->value.enumerated.item >= i)
> +        uinfo->value.enumerated.item = i-1;

Didn't checkpatch.pl complain?  :)

> +static int stac92xx_dc_bias_get(struct snd_kcontrol *kcontrol,
> +                struct snd_ctl_elem_value *ucontrol)
> +{
> +    struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> +    hda_nid_t nid = kcontrol->private_value & 0x0ff;

Better to define a macro to retrieve a value.

> +    unsigned int vref = stac92xx_vref_get(codec, nid);
> +
> +    if (vref == stac92xx_get_vref(codec, nid))

Well, these two functions, stac92xx_vref_get() and stac92xx_get_vref()
are too confusing.  We have to rename one of them, at least, or at best
clean up a bit more.

> @@ -3261,18 +3294,34 @@ static int stac92xx_auto_create_multi_ou
>  
>      if (spec->line_switch) {
>          err = stac92xx_add_control(spec, STAC_CTL_WIDGET_IO_SWITCH,
> -                       "Line In as Output Switch",
> +                       "Line In Jack Mode",
>                         spec->line_switch << 8);
>          if (err < 0)
>              return err;
>      }
>  
> -    if (spec->mic_switch) {
> -        err = stac92xx_add_control(spec, STAC_CTL_WIDGET_DC_BIAS,
> -                       "Mic Jack Mode",
> -                       spec->mic_switch);
> -        if (err < 0)
> -            return err;
> +    nid = cfg->input_pins[AUTO_PIN_MIC];
> +    idx = 0;
> +again:
> +    if (nid) {
> +        def_conf = snd_hda_codec_get_pincfg(codec, nid);
> +        if (!((get_defcfg_connect(def_conf)) & AC_JACK_PORT_FIXED)) {
> +            if (snd_hda_query_pin_caps(codec, nid)
> +                & (AC_PINCAP_VREF_GRD << AC_PINCAP_VREF_SHIFT))
> +                stac92xx_add_control_idx(spec,
> +                    STAC_CTL_WIDGET_DC_BIAS, idx++,
> +                    "Mic Jack Mode", nid
> +                    | ((nid == spec->mic_switch)
> +                    ? 0x100 : 0));
> +            else if (nid == spec->mic_switch)
> +                stac92xx_add_control_idx(spec,
> +                    STAC_CTL_WIDGET_IO_SWITCH, idx++,
> +                    "Mic Jack Mode", ((nid << 8) | 1));
> +        }
> +    }
> +    if (nid == cfg->input_pins[AUTO_PIN_MIC]) {
> +        nid = cfg->input_pins[AUTO_PIN_FRONT_MIC];
> +        goto again;
>      }

Hmm, this code flow is too complex and hackish to follow.
There must be a better way.  For example, the complicated neted if
checks can be put out to a function.  The compiler is clever enough to
inline, and here is no critical code path for speed.


Could you fix and repost please?


thanks,

Takashi