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

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