[alsa-devel] [PATCH] Cirrus Logic CS4210-rev-A1 support.
Takashi Iwai
tiwai at suse.de
Wed Jul 6 08:33:25 CEST 2011
Tim,
thanks for the patch. I'm quickly reviewing this, and comments are
below.
At Tue, 5 Jul 2011 11:10:37 -0500,
Tim Howe wrote:
>
> From: Tim Howe <timothyc.howe at gmail.com>
>
>
> Signed-off-by: Tim Howe <tim.howe at cirrus.com>
Please give a concise patch description, what it does / supports,
what's not implemented or any other notes.
> ---
> sound/pci/hda/patch_cirrus.c | 841 ++++++++++++++++++++++++++++++++++++++++--
> 1 files changed, 805 insertions(+), 36 deletions(-)
>
> diff --git a/sound/pci/hda/patch_cirrus.c b/sound/pci/hda/patch_cirrus.c
> index c7b5ca2..7e09722 100644
> --- a/sound/pci/hda/patch_cirrus.c
> +++ b/sound/pci/hda/patch_cirrus.c
> @@ -51,7 +51,7 @@ struct cs_spec {
> unsigned int cur_adc_format;
> hda_nid_t dig_in;
>
> - const struct hda_bind_ctls *capture_bind[2];
> + struct hda_bind_ctls *capture_bind[2];
Don't revert the upstream change.
> @@ -231,7 +239,7 @@ static int cs_capture_pcm_cleanup(struct hda_pcm_stream *hinfo,
>
> /*
> */
> -static const struct hda_pcm_stream cs_pcm_analog_playback = {
> +static struct hda_pcm_stream cs_pcm_analog_playback = {
Ditto (also a few other chunks).
> @@ -331,8 +339,8 @@ static int is_ext_mic(struct hda_codec *codec, unsigned int idx)
> struct cs_spec *spec = codec->spec;
> struct auto_pin_cfg *cfg = &spec->autocfg;
> hda_nid_t pin = cfg->inputs[idx].pin;
> - unsigned int val;
> - if (!is_jack_detectable(codec, pin))
> + unsigned int val = snd_hda_query_pin_caps(codec, pin);
> + if (!(val & AC_PINCAP_PRES_DETECT))
Don't revert the upstream change here, too.
> @@ -346,10 +354,11 @@ static hda_nid_t get_adc(struct hda_codec *codec, hda_nid_t pin,
>
> nid = codec->start_nid;
> for (i = 0; i < codec->num_nodes; i++, nid++) {
> - hda_nid_t pins[2];
> + hda_nid_t pins[HDA_MAX_NUM_INPUTS];
> unsigned int type;
> int j, nums;
> - type = get_wcaps_type(get_wcaps(codec, nid));
> + type = (get_wcaps(codec, nid) & AC_WCAP_TYPE)
> + >> AC_WCAP_TYPE_SHIFT;
Ditto.
> @@ -558,10 +567,10 @@ static int add_output(struct hda_codec *codec, hda_nid_t dac, int idx,
> const char *name;
> int err, index;
> struct snd_kcontrol *kctl;
> - static const char * const speakers[] = {
> + static char *speakers[] = {
> "Front Speaker", "Surround Speaker", "Bass Speaker"
> };
> - static const char * const line_outs[] = {
> + static char *line_outs[] = {
> "Front Line-Out", "Surround Line-Out", "Bass Line-Out"
> };
Ditto.
> @@ -821,8 +830,7 @@ static int build_digital_output(struct hda_codec *codec)
> if (!spec->multiout.dig_out_nid)
> return 0;
>
> - err = snd_hda_create_spdif_out_ctls(codec, spec->multiout.dig_out_nid,
> - spec->multiout.dig_out_nid);
> + err = snd_hda_create_spdif_out_ctls(codec, spec->multiout.dig_out_nid);
The upstream function was changed. This change will bring a compile error.
> @@ -1278,18 +1289,776 @@ static int patch_cs420x(struct hda_codec *codec)
> return err;
> }
>
> +/*
> + Cirrus Logic CS4210
> +
> + 1 DAC => HP(sense) / Speakers,
> + 1 ADC <= LineIn(sense) / MicIn / DMicIn,
> + 1 SPDIF OUT => SPDIF Trasmitter(sense)
> +*/
> +#define CS4210_DAC_NID 0x02
> +#define CS4210_ADC_NID 0x03
> +#define CS421X_VENDOR_NID 0x0B
> +#define CS421X_DMIC_PIN_NID 0x09 /* Port E */
> +#define CS421X_SPDIF_PIN_NID 0x0A /* Port H */
> +
> +#define CS421X_IDX_DEV_CFG 0x01
> +#define CS421X_IDX_ADC_CFG 0x02
> +#define CS421X_IDX_DAC_CFG 0x03
> +#define CS421X_IDX_SPK_CTL 0x04
> +
> +#define SPDIF_EVENT 0x04
> +
> +/* CS4210 boards */
> +enum {
> + CS421X_CDB4210,
> + CS421X_MODELS
> +};
> +
> +/* CS4210 board names */
> +static const char *cs421x_models[CS421X_MODELS] = {
> + [CS421X_CDB4210] = "cdb4210",
> +};
> +
> +static struct snd_pci_quirk cs421x_cfg_tbl[] = {
> + /* Test Intel board + CDB2410 */
> + SND_PCI_QUIRK(0x8086, 0x5001, "DP45SG/CDB4210", CS421X_CDB4210),
> + {} /* terminator */
> +};
> +
> +/* CS4210 board pinconfigs */
> +/* Default CS4210 (CDB4210)*/
> +static struct cs_pincfg cdb4210_pincfgs[] = {
> + { 0x05, 0x0321401f },
> + { 0x06, 0x90170010 },
> + { 0x07, 0x03813031 },
> + { 0x08, 0xb7a70037 },
> + { 0x09, 0xb7a6003e },
> + { 0x0a, 0x034510f0 },
> + {} /* terminator */
> +};
> +
> +static struct cs_pincfg *cs421x_pincfgs[CS421X_MODELS] = {
> + [CS421X_CDB4210] = cdb4210_pincfgs,
> +};
> +
> +static struct hda_verb cs421x_coef_init_verbs[] = {
> + {0x0B, AC_VERB_SET_PROC_STATE, 1},
> + {0x0B, AC_VERB_SET_COEF_INDEX, CS421X_IDX_DEV_CFG},
> + /*
> + Disable Coefficient Index Auto-Increment(DAI)=1,
> + PDREF=0
> + */
> + {0x0B, AC_VERB_SET_PROC_COEF, 0x0001 },
> +
> + {0x0B, AC_VERB_SET_COEF_INDEX, CS421X_IDX_ADC_CFG},
> + /* ADC SZCMode = Digital Soft Ramp */
> + {0x0B, AC_VERB_SET_PROC_COEF, 0x0002 },
> +
> + {0x0B, AC_VERB_SET_COEF_INDEX, CS421X_IDX_DAC_CFG},
> + {0x0B, AC_VERB_SET_PROC_COEF,
> + (0x0002 /* DAC SZCMode = Digital Soft Ramp */
> + | 0x0004 /* Mute DAC on FIFO error */
> + | 0x0008 /* Enable DAC High Pass Filter */
> + )},
> + {} /* terminator */
> +};
> +
> +/* Errata: CS4210 rev A1 Silicon
> + *
> + * http://www.cirrus.com/en/pubs/errata/
> + *
> + * Description: 1. Performance degredation is present in the ADC.
> + * 2. Speaker output is not completely muted upon HP detect.
> + * 3. Noise is present when clipping occurs on the amplified
> + * speaker outputs.
> + *
> + * Workaround: The following verb sequence written to the registers during
> + * initialization will correct the issues listed above.
> + */
> +
> +static struct hda_verb cs421x_coef_init_verbs_A1_silicon_fixes[] = {
> + {0x0B, AC_VERB_SET_PROC_STATE, 0x01}, /* VPW: processing on */
Use tabs instead of spaces.
> +
> +#include <sound/tlv.h>
Put the included at the beginning of the code.
> +static int cs421x_boost_vol_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
Use tabs.
> + unsigned int vol = ucontrol->value.integer.value[0];
> + unsigned int coef =
> + cs_vendor_coef_get(codec, CS421X_IDX_SPK_CTL);
> +
> + coef &= ~0x0003;
> + coef |= (vol & 0x0003);
> + cs_vendor_coef_set(codec, CS421X_IDX_SPK_CTL, coef);
> +
> + return 1;
In general, the put callback should return 0 if the value is
unchanged, and return 1 when changed. Check with the old value,
and give the return value accordingly.
> +static void cs421x_auto_output(struct hda_codec *codec)
OK, it seems that you want more features and checks for auto-mute.
But, a better way would be to merge cs_auto() and this function.
> +{
> + struct cs_spec *spec = codec->spec;
> + struct auto_pin_cfg *cfg = &spec->autocfg;
> + unsigned int caps, hp_present, spdif_present;
> + hda_nid_t nid;
> + int i;
> +
> + spdif_present = 0;
> + if (cfg->dig_outs) {
> + nid = cfg->dig_out_pins[0];
> + caps = snd_hda_query_pin_caps(codec, nid);
> + if (caps & AC_PINCAP_PRES_DETECT) {
Use is_jack_detectable().
> + /*
> + TODO: SPDIF output redirect when SENSE_B is enabled.
> + Shared (SENSE_A) jack (e.g HP/mini-TOSLINK)
> + assumed.
> + */
> + if (snd_hda_jack_detect(codec, nid)
> + /* && spec->sense_b */)
> + spdif_present = 1;
> + }
> + }
> +
> + hp_present = 0;
> + for (i = 0; i < cfg->hp_outs; i++) {
> + nid = cfg->hp_pins[i];
> + caps = snd_hda_query_pin_caps(codec, nid);
> + if (!(caps & AC_PINCAP_PRES_DETECT))
> + continue;
Ditto.
> + hp_present = snd_hda_jack_detect(codec, nid);
> + if (hp_present)
> + break;
> + }
> +
> + /* mute speakers is spdif or hp jack is plugged in */
> + for (i = 0; i < cfg->speaker_outs; i++) {
> + nid = cfg->speaker_pins[i];
> + snd_hda_codec_write(codec, nid, 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL,
> + (hp_present | spdif_present) ? 0 : PIN_OUT);
Use logical-or (||) instead of bits-or(|).
> + }
> +
> + /* mute HPs if spdif jack (SENSE_B) is present */
> + for (i = 0; i < cfg->hp_outs; i++) {
> + nid = cfg->hp_pins[i];
> + snd_hda_codec_write(codec, nid, 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL,
> + (spdif_present & spec->sense_b)? 0 : PIN_HP);
Use logical-and (&&) instead of bits-and (&).
> + }
> +
> + /* SPDIF TX on/off */
> + if (cfg->dig_outs) {
> + nid = cfg->dig_out_pins[0];
> + snd_hda_codec_write(codec, nid, 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL,
> + spdif_present ? PIN_OUT:0);
> +
> + }
> + /* Update board GPIOs if neccessary ... */
> +}
> +
> +/*
> + CS4210 auto-input redirect
> + Switch max 3 inputs of a single ADC (nid 3)
> +*/
> +static void cs421x_auto_input(struct hda_codec *codec)
> +{
> + struct cs_spec *spec = codec->spec;
> + struct auto_pin_cfg *cfg = &spec->autocfg;
> + hda_nid_t nid;
> + unsigned int present;
> +
> + nid = cfg->inputs[spec->automic_idx].pin;
> + present = snd_hda_jack_detect(codec, nid);
> +
> + if (present) {
> + spec->last_input = spec->cur_input;
> + spec->cur_input = spec->automic_idx;
> + } else {
> + spec->cur_input = spec->last_input;
> + }
> +
> + snd_hda_codec_write_cache(codec, spec->cur_adc, 0,
> + AC_VERB_SET_CONNECT_SEL,
> + spec->adc_idx[spec->cur_input]);
> +}
Why not reusing cs_automic()? Also, does it work without
ADC-switching? If ADC-switching is needed, the similar procedure like
cs_automic() is necessary.
> +static void cs421x_pinmux_init(struct hda_codec *codec)
> +{
> + struct cs_spec *spec = codec->spec;
> + unsigned int def_conf, coef;
> +
> + /* GPIO, DMIC_SCL, DMIC_SDA and SENSE_B are multiplexed */
> + coef = cs_vendor_coef_get(codec, CS421X_IDX_DEV_CFG);
> +
> + if (spec->gpio_mask)
> + coef |= 0x0008; /* B1,B2 are GPIOs */
> + else
> + coef &= ~0x0008;
> +
> + if (spec->sense_b)
> + coef |= 0x0010; /* B2 is SENSE_B, not inverted */
> + else
> + coef &= ~0x0010;
> +
> + cs_vendor_coef_set(codec, CS421X_IDX_DEV_CFG, coef);
> +
> + if ((spec->gpio_mask || spec->sense_b) &&
> + is_active_pin(codec, CS421X_DMIC_PIN_NID))
> + {
> + /*
> + GPIO or SENSE_B forced - disconnect the DMIC pin.
> + */
> + def_conf = snd_hda_codec_get_pincfg(codec, CS421X_DMIC_PIN_NID);
> + def_conf &= ~AC_DEFCFG_PORT_CONN;
> + def_conf |= (AC_JACK_PORT_NONE << AC_DEFCFG_PORT_CONN_SHIFT);
> + snd_hda_codec_set_pincfg(codec, CS421X_DMIC_PIN_NID, def_conf);
> + }
> +}
> +
> +/* */
> +static void init_cs421x_output(struct hda_codec *codec)
Once when cs_automute() was changed to support cs421x, this function
can be dropped. It'll be identical with init_output().
> +{
> + struct cs_spec *spec = codec->spec;
> + struct auto_pin_cfg *cfg = &spec->autocfg;
> + int i;
> +
> + /* mute first */
> + for (i = 0; i < spec->multiout.num_dacs; i++)
> + snd_hda_codec_write(codec, spec->multiout.dac_nids[i], 0,
> + AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
> + if (spec->multiout.hp_nid)
> + snd_hda_codec_write(codec, spec->multiout.hp_nid, 0,
> + AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
> + for (i = 0; i < ARRAY_SIZE(spec->multiout.extra_out_nid); i++) {
> + if (!spec->multiout.extra_out_nid[i])
> + break;
> + snd_hda_codec_write(codec, spec->multiout.extra_out_nid[i], 0,
> + AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_MUTE);
> + }
> +
> + /* set appropriate pin controls */
> + for (i = 0; i < cfg->line_outs; i++)
> + snd_hda_codec_write(codec, cfg->line_out_pins[i], 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_OUT);
> + /* HP */
> + for (i = 0; i < cfg->hp_outs; i++) {
> + hda_nid_t nid = cfg->hp_pins[i];
> + snd_hda_codec_write(codec, nid, 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_HP);
> + if (!cfg->speaker_outs)
> + continue;
> + if (get_wcaps(codec, nid) & AC_WCAP_UNSOL_CAP) {
> + snd_hda_codec_write(codec, nid, 0,
> + AC_VERB_SET_UNSOLICITED_ENABLE,
> + AC_USRSP_EN | HP_EVENT);
> + spec->hp_detect = 1;
> + }
> + }
> +
> + /* Speaker */
> + for (i = 0; i < cfg->speaker_outs; i++)
> + snd_hda_codec_write(codec, cfg->speaker_pins[i], 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_OUT);
> +
> + /* SPDIF is enabled on presence detect */
> + if (spec->hp_detect || spec->spdif_detect)
> + cs421x_auto_output(codec);
> +}
> +
> +static void init_cs421x_input(struct hda_codec *codec)
This function is almost identical with init_input(). Should be merged.
> +{
> + struct cs_spec *spec = codec->spec;
> + struct auto_pin_cfg *cfg = &spec->autocfg;
> + int i;
> +
> + for (i = 0; i < cfg->num_inputs; i++) {
> + unsigned int ctl;
> + hda_nid_t pin = cfg->inputs[i].pin;
> +
> + if (!spec->adc_nid[i])
> + continue;
> + /* set appropriate pin control and mute first */
> + ctl = PIN_IN;
> + if (cfg->inputs[i].type == AUTO_PIN_MIC) {
> + unsigned int caps = snd_hda_query_pin_caps(codec, pin);
> + caps >>= AC_PINCAP_VREF_SHIFT;
> + if (caps & AC_PINCAP_VREF_80)
> + ctl = PIN_VREF80;
> + }
> +
> + snd_hda_codec_write(codec, pin, 0,
> + AC_VERB_SET_PIN_WIDGET_CONTROL, ctl);
> + snd_hda_codec_write(codec, spec->adc_nid[i], 0,
> + AC_VERB_SET_AMP_GAIN_MUTE,
> + AMP_IN_MUTE(spec->adc_idx[i]));
> +
> + if (spec->mic_detect && spec->automic_idx == i) {
> + snd_hda_codec_write(codec, pin, 0,
> + AC_VERB_SET_UNSOLICITED_ENABLE,
> + AC_USRSP_EN | MIC_EVENT);
> + }
> + }
> +
> + if (spec->mic_detect)
> + cs421x_auto_input(codec);
> + else {
> + spec->cur_adc = spec->adc_nid[spec->cur_input];
> + snd_hda_codec_write(codec, spec->cur_adc, 0,
> + AC_VERB_SET_CONNECT_SEL,
> + spec->adc_idx[spec->cur_input]);
> + }
> +
> +}
> +
> +static void init_cs421x_digital(struct hda_codec *codec)
> +{
> + struct cs_spec *spec = codec->spec;
> + struct auto_pin_cfg *cfg = &spec->autocfg;
> + int i;
> +
> +
> + for (i = 0; i < cfg->dig_outs; i++) {
> + hda_nid_t nid = cfg->dig_out_pins[i];
> + if (!cfg->speaker_outs)
> + continue;
> + if (get_wcaps(codec, nid) & AC_WCAP_UNSOL_CAP)
> + {
> + snd_hda_codec_write(codec, nid, 0,
> + AC_VERB_SET_UNSOLICITED_ENABLE,
> + AC_USRSP_EN | SPDIF_EVENT);
> + spec->spdif_detect = 1;
> + }
> + }
> +}
> +
> +static int cs421x_init(struct hda_codec *codec)
> +{
> + struct cs_spec *spec = codec->spec;
> +
> + snd_hda_sequence_write(codec, cs421x_coef_init_verbs);
> + snd_hda_sequence_write(codec, cs421x_coef_init_verbs_A1_silicon_fixes);
> +
> + cs421x_pinmux_init(codec);
> +
> + if (spec->gpio_mask) {
> + snd_hda_codec_write(codec, 0x01, 0, AC_VERB_SET_GPIO_MASK,
> + spec->gpio_mask);
> + snd_hda_codec_write(codec, 0x01, 0, AC_VERB_SET_GPIO_DIRECTION,
> + spec->gpio_dir);
> + snd_hda_codec_write(codec, 0x01, 0, AC_VERB_SET_GPIO_DATA,
> + spec->gpio_data);
> + }
> +
> + init_cs421x_output(codec);
> + init_cs421x_input(codec);
> + init_cs421x_digital(codec);
> +
> + return 0;
> +}
> +
> +/*
> + * CS4210 Input MUX (1 ADC)
> + */
> +static int cs421x_mux_enum_info(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_info *uinfo)
> +{
> + struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> + struct cs_spec *spec = codec->spec;
> +
> + return snd_hda_input_mux_info(&spec->input_mux, uinfo);
> +}
> +
> +static int cs421x_mux_enum_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> + struct cs_spec *spec = codec->spec;
> +
> + ucontrol->value.enumerated.item[0] = spec->cur_input;
> + return 0;
> +}
> +
> +static int cs421x_mux_enum_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct hda_codec *codec = snd_kcontrol_chip(kcontrol);
> + struct cs_spec *spec = codec->spec;
> +
> + return snd_hda_input_mux_put(codec, &spec->input_mux, ucontrol,
> + spec->adc_nid[0], &spec->cur_input);
Hm, so CS421x has an input MUX to the ADC?
Then why spec->cur_input and the corresponding ADC is switched in
init_cs421x_input() and cs421x_auto_input()?
So, in summary:
- I see minor coding-style issues in many places. Try
scripts/checkpatch.pl and fix the errors/warnings suggested there.
- Try to avoid reverting the upstream changes (e.g. const addition,
replacements with new helper functions and macros)
- Try to re-use the existing functions as much as possible. Instead
of copying the whole old function and modify a little bit, either
merge into one with some conditional, or split to a core-function
and each codec function calls it with some codec-specific
additions. This will make the code-maintenance much easier.
- I wonder whether the ADC and capture-source handling is correct.
At best, please give alsa-info.sh output so that I can check and
follow the behavior via hda-emu.
thanks,
Takashi
More information about the Alsa-devel
mailing list