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@gmail.com
Signed-off-by: Tim Howe tim.howe@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];
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
- 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