| Message ID | 20210306111934.4832-1-vitalyr@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec | expand |
On Sat, 06 Mar 2021 12:19:30 +0100, Vitaly Rodionov wrote: > > Dell's laptops Inspiron 3500, Inspiron 3501, Inspiron 3505 are using > Cirrus Logic CS8409 HDA bridge with CS42L42 companion codec. > > The CS8409 is a multichannel HD audio routing controller. > CS8409 includes support for four channels of digital > microphone data and two bidirectional ASPs for up to 32 > channels of TDM data or 4 channels of I2S data. The CS8409 is > intended to be used with a remote companion codec that implements > high performance analog functions in close physical > proximity to the end-equipment audio port or speaker driver. > > The CS42L42 is a low-power audio codec with integrated MIPI > SoundWire interface or I2C/I2S/TDM interfaces designed > for portable applications. It provides a high-dynamic range, > stereo DAC for audio playback and a mono high-dynamic-range > ADC for audio capture > > Changes since version 1: > > ALSA: hda/cirrus: Increase AUTO_CFG_MAX_INS from 8 to 18 > * No change > > ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 > companion codec. > * Removed redundant fields in fixup table > * Handle gpio via spec->gpio_dir, spec->gpio_data and spec->gpio_mask > * Moved cs8409_cs42l42_init() from patch 2, to handle resume correctly > > ALSA: hda/cirrus: Add jack detect interrupt support from CS42L42 > companion codec. > * Run scripts/checkpatch.pl, fixed new warnings > > ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control > * Moved control values to cache to avoid i2c read at each time. > > Stefan Binding (1): > ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control > > Vitaly Rodionov (3): > ALSA: hda/cirrus: Increase AUTO_CFG_MAX_INS from 8 to 18 > ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 > companion codec. > ALSA: hda/cirrus: Add jack detect interrupt support from CS42L42 > companion codec. Applied all four patches now. Thanks. Takashi
> +/* Enable I2C clocks */ > +static void cs8409_enable_i2c_clock(struct hda_codec *codec, unsigned int flag) > +{ > + unsigned int retval = 0; > + unsigned int newval = 0; initializations not needed > + retval = cs_vendor_coef_get(codec, 0x0); > + newval = (flag) ? (retval | 0x8) : (retval & 0xfffffff7); > + cs_vendor_coef_set(codec, 0x0, newval); > +} > + > +/* Wait I2C transaction */ > +static int cs8409_i2c_wait_complete(struct hda_codec *codec) > +{ > + int repeat = 5; > + unsigned int retval = 0; initialization not needed. > + > + do { > + retval = cs_vendor_coef_get(codec, CIR_I2C_STATUS); > + if ((retval & 0x18) != 0x18) { > + usleep_range(2000, 4000); > + --repeat; > + } else > + break; > + > + } while (repeat); > + > + return repeat > 0 ? 0 : -1; can we simplify by returning !!repeat ? > +} > + > +/* CS8409 slave i2cRead */ > +static unsigned int cs8409_i2c_read(struct hda_codec *codec, > + unsigned int i2c_address, > + unsigned int i2c_reg, > + unsigned int paged) > +{ > + unsigned int i2c_reg_data; > + unsigned int retval = 0; > + > + cs8409_enable_i2c_clock(codec, 1); > + cs_vendor_coef_set(codec, CIR_I2C_ADDR, i2c_address); > + > + if (paged) { > + cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg >> 8); > + if (cs8409_i2c_wait_complete(codec) == -1) { > + codec_err(codec, > + "%s() Paged Transaction Failed 0x%02x : 0x%04x = 0x%02x\n", > + __func__, i2c_address, i2c_reg, retval); return an error? > + } > + } > + > + i2c_reg_data = (i2c_reg << 8) & 0x0ffff; > + cs_vendor_coef_set(codec, CIR_I2C_QREAD, i2c_reg_data); > + if (cs8409_i2c_wait_complete(codec) == -1) { > + codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x = 0x%02x\n", > + __func__, i2c_address, i2c_reg, retval); return and error? > + } > + > + /* Register in bits 15-8 and the data in 7-0 */ > + retval = cs_vendor_coef_get(codec, CIR_I2C_QREAD); > + retval &= 0x0ff; > + > + cs8409_enable_i2c_clock(codec, 0); > + > + return retval; > +} > + > +/* CS8409 slave i2cWrite */ > +static unsigned int cs8409_i2c_write(struct hda_codec *codec, > + unsigned int i2c_address, unsigned int i2c_reg, > + unsigned int i2c_data, > + unsigned int paged) > +{ > + unsigned int retval = 0; > + unsigned int i2c_reg_data = 0; > + > + cs8409_enable_i2c_clock(codec, 1); > + cs_vendor_coef_set(codec, CIR_I2C_ADDR, i2c_address); > + > + if (paged) { > + cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg >> 8); > + if (cs8409_i2c_wait_complete(codec) == -1) { > + codec_err(codec, > + "%s() Paged Transaction Failed 0x%02x : 0x%04x = 0x%02x\n", > + __func__, i2c_address, i2c_reg, retval); return error? > + } > + } > + > + i2c_reg_data = ((i2c_reg << 8) & 0x0ff00) | (i2c_data & 0x0ff); > + cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg_data); > + > + if (cs8409_i2c_wait_complete(codec) == -1) { > + codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x = 0x%02x\n", > + __func__, i2c_address, i2c_reg, retval); return error? > + } > + > + cs8409_enable_i2c_clock(codec, 0); > + > + return retval; > +} > + > +/* Assert/release RTS# line to CS42L42 */ > +static void cs8409_cs42l42_reset(struct hda_codec *codec) > +{ > + /* Assert RTS# line */ > + snd_hda_codec_write(codec, > + codec->core.afg, 0, AC_VERB_SET_GPIO_DATA, 0); > + /* wait ~10ms */ > + usleep_range(10000, 15000); > + /* Release RTS# line */ > + snd_hda_codec_write(codec, > + codec->core.afg, 0, AC_VERB_SET_GPIO_DATA, GPIO5_INT); > + /* wait ~10ms */ > + usleep_range(10000, 15000); > + > + /* Clear interrupts status */ > + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1308, 1); > + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1309, 1); > + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130A, 1); > + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130F, 1); clear on read? > +static int cs8409_cs42l42_fixup(struct hda_codec *codec) > +{ > + int err = 0; useless init > + struct cs_spec *spec = codec->spec; > + unsigned int pincap = 0; > + > + /* Basic initial sequence for specific hw configuration */ > + snd_hda_sequence_write(codec, cs8409_cs42l42_init_verbs); > + > + /* CS8409 is simple HDA bridge and intended to be used with a remote > + * companion codec. Most of input/output PIN(s) have only basic > + * capabilities. NID(s) 0x24 and 0x34 have only OUTC and INC > + * capabilities and no presence detect capable (PDC) and call to > + * snd_hda_gen_build_controls() will mark them as non detectable > + * phantom jacks. However, in this configuration companion codec > + * CS42L42 is connected to these pins and it has jack detect > + * capabilities. We have to override pin capabilities, > + * otherwise they will not be created as input devices. > + */ > + _snd_hdac_read_parm(&codec->core, > + CS8409_CS42L42_HP_PIN_NID, AC_PAR_PIN_CAP, &pincap); > + > + snd_hdac_override_parm(&codec->core, > + CS8409_CS42L42_HP_PIN_NID, AC_PAR_PIN_CAP, > + (pincap | (AC_PINCAP_IMP_SENSE | AC_PINCAP_PRES_DETECT))); > + > + _snd_hdac_read_parm(&codec->core, CS8409_CS42L42_AMIC_PIN_NID, > + AC_PAR_PIN_CAP, &pincap); > + > + snd_hdac_override_parm(&codec->core, > + CS8409_CS42L42_AMIC_PIN_NID, AC_PAR_PIN_CAP, > + (pincap | (AC_PINCAP_IMP_SENSE | AC_PINCAP_PRES_DETECT))); > + > + snd_hda_override_wcaps(codec, CS8409_CS42L42_HP_PIN_NID, > + (get_wcaps(codec, CS8409_CS42L42_HP_PIN_NID) | AC_WCAP_UNSOL_CAP)); > + > + snd_hda_override_wcaps(codec, CS8409_CS42L42_AMIC_PIN_NID, > + (get_wcaps(codec, CS8409_CS42L42_AMIC_PIN_NID) | AC_WCAP_UNSOL_CAP)); > + > + snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_PRE_PROBE); > + > + err = snd_hda_parse_pin_defcfg(codec, &spec->gen.autocfg, 0, 0); > + if (err < 0) > + return err; > + > + err = snd_hda_gen_parse_auto_config(codec, &spec->gen.autocfg); > + if (err < 0) > + return err; > + > + snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_PROBE); > + > + return err; > +} > + > +static int patch_cs8409(struct hda_codec *codec) > +{ > + struct cs_spec *spec; > + int err = -EINVAL; > + > + spec = cs_alloc_spec(codec, CS8409_VENDOR_NID); > + if (!spec) > + return -ENOMEM; > + > + snd_hda_pick_fixup(codec, > + cs8409_models, cs8409_fixup_tbl, cs8409_fixups); > + > + codec_dbg(codec, "Picked ID=%d, VID=%08x, DEV=%08x\n", > + codec->fixup_id, > + codec->bus->pci->subsystem_vendor, > + codec->bus->pci->subsystem_device); > + > + switch (codec->fixup_id) { > + /* Dell platforms with CS42L42 companion codec */ > + case CS8409_BULLSEYE: > + case CS8409_WARLOCK: > + case CS8409_CYBORG: > + > + snd_hda_add_verbs(codec, cs8409_cs42l42_add_verbs); > + > + codec->patch_ops = cs8409_cs42l42_patch_ops; > + > + spec->gen.suppress_auto_mute = 1; > + spec->gen.no_primary_hp = 1; > + /* GPIO 5 out, 3,4 in */ > + spec->gpio_dir = GPIO5_INT; > + spec->gpio_data = 0; > + spec->gpio_mask = 0x03f; > + > + err = cs8409_cs42l42_fixup(codec); > + > + if (err > 0) better keep the convention that errors are negative and zero is success. > + err = cs8409_cs42l42_hw_init(codec); > + break; > + > + default: > + codec_err(codec, "VID=%08x, DEV=%08x not supported\n", > + codec->bus->pci->subsystem_vendor, > + codec->bus->pci->subsystem_device); > + break; > + } > + if (err < 0) > + cs_free(codec); > + else > + snd_hda_codec_set_name(codec, "CS8409/CS42L42"); > + > + return err; > +} > > /* > * patch entries > @@ -1229,6 +1804,7 @@ static const struct hda_device_id snd_hda_id_cirrus[] = { > HDA_CODEC_ENTRY(0x10134208, "CS4208", patch_cs4208), > HDA_CODEC_ENTRY(0x10134210, "CS4210", patch_cs4210), > HDA_CODEC_ENTRY(0x10134213, "CS4213", patch_cs4213), > + HDA_CODEC_ENTRY(0x10138409, "CS8409", patch_cs8409), > {} /* terminator */ > }; > MODULE_DEVICE_TABLE(hdaudio, snd_hda_id_cirrus); >
On 3/6/21 5:19 AM, Vitaly Rodionov wrote: > From: Stefan Binding <sbinding@opensource.cirrus.com> > > CS8409 does not support Volume Control for NIDs 0x24 (the Headphones), > or 0x34 (The Headset Mic). > However, CS42L42 codec does support gain control for both. Volume Control for both > We can add support for Volume Controls, by writing the the CS42L42 > regmap via i2c commands, using custom info, get and put volume > functions, saved in the control. > > Tested on DELL Inspiron-3500, DELL Inspiron-3501, DELL Inspiron-3500 > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > --- > > Changes in v3: > - Added restore volumes after resume > - Removed redundant debug logging after testing > > > sound/pci/hda/patch_cirrus.c | 200 +++++++++++++++++++++++++++++++++++ > 1 file changed, 200 insertions(+) > > diff --git a/sound/pci/hda/patch_cirrus.c b/sound/pci/hda/patch_cirrus.c > index 1d2f6a1224e6..6a9e5c803977 100644 > --- a/sound/pci/hda/patch_cirrus.c > +++ b/sound/pci/hda/patch_cirrus.c > @@ -21,6 +21,9 @@ > /* > */ > > +#define CS42L42_HP_CH (2U) > +#define CS42L42_HS_MIC_CH (1U) > + > struct cs_spec { > struct hda_gen_spec gen; > > @@ -42,6 +45,9 @@ struct cs_spec { > > unsigned int cs42l42_hp_jack_in:1; > unsigned int cs42l42_mic_jack_in:1; > + unsigned int cs42l42_volume_init:1; can you describe what this field is? it looks like it's only tracking a one-time initialization? > + char cs42l42_hp_volume[CS42L42_HP_CH]; > + char cs42l42_hs_mic_volume[CS42L42_HS_MIC_CH]; > > struct mutex cs8409_i2c_mux; > > @@ -1260,6 +1266,14 @@ static int patch_cs4213(struct hda_codec *codec) > #define CIR_I2C_QWRITE 0x005D > #define CIR_I2C_QREAD 0x005E > > +#define CS8409_CS42L42_HP_VOL_REAL_MIN (-63) > +#define CS8409_CS42L42_HP_VOL_REAL_MAX (0) > +#define CS8409_CS42L42_AMIC_VOL_REAL_MIN (-97) > +#define CS8409_CS42L42_AMIC_VOL_REAL_MAX (12) > +#define CS8409_CS42L42_REG_HS_VOLUME_CHA (0x2301) > +#define CS8409_CS42L42_REG_HS_VOLUME_CHB (0x2303) > +#define CS8409_CS42L42_REG_AMIC_VOLUME (0x1D03) > + > struct cs8409_i2c_param { > unsigned int addr; > unsigned int reg; > @@ -1580,6 +1594,165 @@ static unsigned int cs8409_i2c_write(struct hda_codec *codec, > return retval; > } > > +static int cs8409_cs42l42_volume_info(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_info *uinfo) > +{ > + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); > + u16 nid = get_amp_nid(kcontrol); > + u8 chs = get_amp_channels(kcontrol); > + > + codec_dbg(codec, "%s() nid: %d\n", __func__, nid); > + switch (nid) { > + case CS8409_CS42L42_HP_PIN_NID: > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = chs == 3 ? 2 : 1; > + uinfo->value.integer.min = CS8409_CS42L42_HP_VOL_REAL_MIN; > + uinfo->value.integer.max = CS8409_CS42L42_HP_VOL_REAL_MAX; > + break; > + case CS8409_CS42L42_AMIC_PIN_NID: > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = chs == 3 ? 2 : 1; > + uinfo->value.integer.min = CS8409_CS42L42_AMIC_VOL_REAL_MIN; > + uinfo->value.integer.max = CS8409_CS42L42_AMIC_VOL_REAL_MAX; > + break; > + default: > + break; > + } > + return 0; > +} > + > +static void cs8409_cs42l42_update_volume(struct hda_codec *codec) > +{ > + struct cs_spec *spec = codec->spec; > + > + mutex_lock(&spec->cs8409_i2c_mux); > + spec->cs42l42_hp_volume[0] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR, > + CS8409_CS42L42_REG_HS_VOLUME_CHA, 1)); > + spec->cs42l42_hp_volume[1] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR, > + CS8409_CS42L42_REG_HS_VOLUME_CHB, 1)); > + spec->cs42l42_hs_mic_volume[0] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR, > + CS8409_CS42L42_REG_AMIC_VOLUME, 1)); > + mutex_unlock(&spec->cs8409_i2c_mux); > + spec->cs42l42_volume_init = 1; > +} > + > +static int cs8409_cs42l42_volume_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; > + hda_nid_t nid = get_amp_nid(kcontrol); > + int chs = get_amp_channels(kcontrol); > + long *valp = ucontrol->value.integer.value; > + > + if (!spec->cs42l42_volume_init) { > + snd_hda_power_up(codec); > + cs8409_cs42l42_update_volume(codec); > + snd_hda_power_down(codec); > + } > + switch (nid) { > + case CS8409_CS42L42_HP_PIN_NID: > + if (chs & 1) BIT(0)? > + *valp++ = spec->cs42l42_hp_volume[0]; > + if (chs & 2) BIT(1)? > + *valp++ = spec->cs42l42_hp_volume[1]; > + break;
On 08/03/2021 3:33 pm, Pierre-Louis Bossart wrote: Hi Pierre-Louis Thanks a lot for your comments. Since this patch set has been merged we will address your comments in a next fix patch. > >> +/* Enable I2C clocks */ >> +static void cs8409_enable_i2c_clock(struct hda_codec *codec, >> unsigned int flag) >> +{ >> + unsigned int retval = 0; >> + unsigned int newval = 0; > > initializations not needed Will fix that. > >> + retval = cs_vendor_coef_get(codec, 0x0); >> + newval = (flag) ? (retval | 0x8) : (retval & 0xfffffff7); >> + cs_vendor_coef_set(codec, 0x0, newval); >> +} >> + >> +/* Wait I2C transaction */ >> +static int cs8409_i2c_wait_complete(struct hda_codec *codec) >> +{ >> + int repeat = 5; >> + unsigned int retval = 0; > > initialization not needed. Will fix that. > >> + >> + do { >> + retval = cs_vendor_coef_get(codec, CIR_I2C_STATUS); >> + if ((retval & 0x18) != 0x18) { >> + usleep_range(2000, 4000); >> + --repeat; >> + } else >> + break; >> + >> + } while (repeat); >> + >> + return repeat > 0 ? 0 : -1; > > can we simplify by returning !!repeat ? Yes, we can simplify this code as well. > >> +} >> + >> +/* CS8409 slave i2cRead */ >> +static unsigned int cs8409_i2c_read(struct hda_codec *codec, >> + unsigned int i2c_address, >> + unsigned int i2c_reg, >> + unsigned int paged) >> +{ >> + unsigned int i2c_reg_data; >> + unsigned int retval = 0; >> + >> + cs8409_enable_i2c_clock(codec, 1); >> + cs_vendor_coef_set(codec, CIR_I2C_ADDR, i2c_address); >> + >> + if (paged) { >> + cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg >> 8); >> + if (cs8409_i2c_wait_complete(codec) == -1) { >> + codec_err(codec, >> + "%s() Paged Transaction Failed 0x%02x : 0x%04x = >> 0x%02x\n", >> + __func__, i2c_address, i2c_reg, retval); > > return an error? Yes, you are right there is no reason to continue if we hit an error here. Will be fixed. > >> + } >> + } >> + >> + i2c_reg_data = (i2c_reg << 8) & 0x0ffff; >> + cs_vendor_coef_set(codec, CIR_I2C_QREAD, i2c_reg_data); >> + if (cs8409_i2c_wait_complete(codec) == -1) { >> + codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x = >> 0x%02x\n", >> + __func__, i2c_address, i2c_reg, retval); > > return and error? Same as above > >> + } >> + >> + /* Register in bits 15-8 and the data in 7-0 */ >> + retval = cs_vendor_coef_get(codec, CIR_I2C_QREAD); >> + retval &= 0x0ff; >> + >> + cs8409_enable_i2c_clock(codec, 0); >> + >> + return retval; >> +} >> + >> +/* CS8409 slave i2cWrite */ >> +static unsigned int cs8409_i2c_write(struct hda_codec *codec, >> + unsigned int i2c_address, unsigned int i2c_reg, >> + unsigned int i2c_data, >> + unsigned int paged) >> +{ >> + unsigned int retval = 0; >> + unsigned int i2c_reg_data = 0; >> + >> + cs8409_enable_i2c_clock(codec, 1); >> + cs_vendor_coef_set(codec, CIR_I2C_ADDR, i2c_address); >> + >> + if (paged) { >> + cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg >> 8); >> + if (cs8409_i2c_wait_complete(codec) == -1) { >> + codec_err(codec, >> + "%s() Paged Transaction Failed 0x%02x : 0x%04x = >> 0x%02x\n", >> + __func__, i2c_address, i2c_reg, retval); > > return error? Same as above > >> + } >> + } >> + >> + i2c_reg_data = ((i2c_reg << 8) & 0x0ff00) | (i2c_data & 0x0ff); >> + cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg_data); >> + >> + if (cs8409_i2c_wait_complete(codec) == -1) { >> + codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x = >> 0x%02x\n", >> + __func__, i2c_address, i2c_reg, retval); > > return error? Same as above > >> + } >> + >> + cs8409_enable_i2c_clock(codec, 0); >> + >> + return retval; >> +} >> + >> +/* Assert/release RTS# line to CS42L42 */ >> +static void cs8409_cs42l42_reset(struct hda_codec *codec) >> +{ >> + /* Assert RTS# line */ >> + snd_hda_codec_write(codec, >> + codec->core.afg, 0, AC_VERB_SET_GPIO_DATA, 0); >> + /* wait ~10ms */ >> + usleep_range(10000, 15000); >> + /* Release RTS# line */ >> + snd_hda_codec_write(codec, >> + codec->core.afg, 0, AC_VERB_SET_GPIO_DATA, GPIO5_INT); >> + /* wait ~10ms */ >> + usleep_range(10000, 15000); >> + >> + /* Clear interrupts status */ >> + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1308, 1); >> + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1309, 1); >> + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130A, 1); >> + cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130F, 1); > > clear on read? This is how CS42L42 works, to clear interrupts we have to read interrupts status registers. > > >> +static int cs8409_cs42l42_fixup(struct hda_codec *codec) >> +{ >> + int err = 0; > > useless init Will fix. > >> + struct cs_spec *spec = codec->spec; >> + unsigned int pincap = 0; >> + >> + /* Basic initial sequence for specific hw configuration */ >> + snd_hda_sequence_write(codec, cs8409_cs42l42_init_verbs); >> + >> + /* CS8409 is simple HDA bridge and intended to be used with a >> remote >> + * companion codec. Most of input/output PIN(s) have only basic >> + * capabilities. NID(s) 0x24 and 0x34 have only OUTC and INC >> + * capabilities and no presence detect capable (PDC) and call to >> + * snd_hda_gen_build_controls() will mark them as non detectable >> + * phantom jacks. However, in this configuration companion codec >> + * CS42L42 is connected to these pins and it has jack detect >> + * capabilities. We have to override pin capabilities, >> + * otherwise they will not be created as input devices. >> + */ >> + _snd_hdac_read_parm(&codec->core, >> + CS8409_CS42L42_HP_PIN_NID, AC_PAR_PIN_CAP, &pincap); >> + >> + snd_hdac_override_parm(&codec->core, >> + CS8409_CS42L42_HP_PIN_NID, AC_PAR_PIN_CAP, >> + (pincap | (AC_PINCAP_IMP_SENSE | AC_PINCAP_PRES_DETECT))); >> + >> + _snd_hdac_read_parm(&codec->core, CS8409_CS42L42_AMIC_PIN_NID, >> + AC_PAR_PIN_CAP, &pincap); >> + >> + snd_hdac_override_parm(&codec->core, >> + CS8409_CS42L42_AMIC_PIN_NID, AC_PAR_PIN_CAP, >> + (pincap | (AC_PINCAP_IMP_SENSE | AC_PINCAP_PRES_DETECT))); >> + >> + snd_hda_override_wcaps(codec, CS8409_CS42L42_HP_PIN_NID, >> + (get_wcaps(codec, CS8409_CS42L42_HP_PIN_NID) | >> AC_WCAP_UNSOL_CAP)); >> + >> + snd_hda_override_wcaps(codec, CS8409_CS42L42_AMIC_PIN_NID, >> + (get_wcaps(codec, CS8409_CS42L42_AMIC_PIN_NID) | >> AC_WCAP_UNSOL_CAP)); >> + >> + snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_PRE_PROBE); >> + >> + err = snd_hda_parse_pin_defcfg(codec, &spec->gen.autocfg, 0, 0); >> + if (err < 0) >> + return err; >> + >> + err = snd_hda_gen_parse_auto_config(codec, &spec->gen.autocfg); >> + if (err < 0) >> + return err; >> + >> + snd_hda_apply_fixup(codec, HDA_FIXUP_ACT_PROBE); >> + >> + return err; >> +} >> + >> +static int patch_cs8409(struct hda_codec *codec) >> +{ >> + struct cs_spec *spec; >> + int err = -EINVAL; >> + >> + spec = cs_alloc_spec(codec, CS8409_VENDOR_NID); >> + if (!spec) >> + return -ENOMEM; >> + >> + snd_hda_pick_fixup(codec, >> + cs8409_models, cs8409_fixup_tbl, cs8409_fixups); >> + >> + codec_dbg(codec, "Picked ID=%d, VID=%08x, DEV=%08x\n", >> + codec->fixup_id, >> + codec->bus->pci->subsystem_vendor, >> + codec->bus->pci->subsystem_device); >> + >> + switch (codec->fixup_id) { >> + /* Dell platforms with CS42L42 companion codec */ >> + case CS8409_BULLSEYE: >> + case CS8409_WARLOCK: >> + case CS8409_CYBORG: >> + >> + snd_hda_add_verbs(codec, cs8409_cs42l42_add_verbs); >> + >> + codec->patch_ops = cs8409_cs42l42_patch_ops; >> + >> + spec->gen.suppress_auto_mute = 1; >> + spec->gen.no_primary_hp = 1; >> + /* GPIO 5 out, 3,4 in */ >> + spec->gpio_dir = GPIO5_INT; >> + spec->gpio_data = 0; >> + spec->gpio_mask = 0x03f; >> + >> + err = cs8409_cs42l42_fixup(codec); >> + >> + if (err > 0) > > better keep the convention that errors are negative and zero is success. Agreed. Will fix. > >> + err = cs8409_cs42l42_hw_init(codec); >> + break; >> + >> + default: >> + codec_err(codec, "VID=%08x, DEV=%08x not supported\n", >> + codec->bus->pci->subsystem_vendor, >> + codec->bus->pci->subsystem_device); >> + break; >> + } >> + if (err < 0) >> + cs_free(codec); >> + else >> + snd_hda_codec_set_name(codec, "CS8409/CS42L42"); >> + >> + return err; >> +} >> /* >> * patch entries >> @@ -1229,6 +1804,7 @@ static const struct hda_device_id >> snd_hda_id_cirrus[] = { >> HDA_CODEC_ENTRY(0x10134208, "CS4208", patch_cs4208), >> HDA_CODEC_ENTRY(0x10134210, "CS4210", patch_cs4210), >> HDA_CODEC_ENTRY(0x10134213, "CS4213", patch_cs4213), >> + HDA_CODEC_ENTRY(0x10138409, "CS8409", patch_cs8409), >> {} /* terminator */ >> }; >> MODULE_DEVICE_TABLE(hdaudio, snd_hda_id_cirrus); >>
Hi Pierre-Louis, Thank you for your comments. It looks like this patch set has been merged, so we will look to address your comments in a future patch. -----Original Message----- From: Alsa-devel <alsa-devel-bounces@alsa-project.org> On Behalf Of Pierre-Louis Bossart Sent: 08 March 2021 15:40 To: Vitaly Rodionov <vitalyr@opensource.cirrus.com>; Jaroslav Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com> Cc: patches@opensource.cirrus.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Stefan Binding <sbinding@opensource.cirrus.com> Subject: Re: [PATCH v3 4/4] ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control On 3/6/21 5:19 AM, Vitaly Rodionov wrote: > From: Stefan Binding <sbinding@opensource.cirrus.com> > > CS8409 does not support Volume Control for NIDs 0x24 (the Headphones), > or 0x34 (The Headset Mic). > However, CS42L42 codec does support gain control for both. Volume Control for both > We can add support for Volume Controls, by writing the the CS42L42 > regmap via i2c commands, using custom info, get and put volume > functions, saved in the control. > > Tested on DELL Inspiron-3500, DELL Inspiron-3501, DELL Inspiron-3500 > > Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com> > Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com> > --- > > Changes in v3: > - Added restore volumes after resume > - Removed redundant debug logging after testing > > > sound/pci/hda/patch_cirrus.c | 200 +++++++++++++++++++++++++++++++++++ > 1 file changed, 200 insertions(+) > > diff --git a/sound/pci/hda/patch_cirrus.c > b/sound/pci/hda/patch_cirrus.c index 1d2f6a1224e6..6a9e5c803977 100644 > --- a/sound/pci/hda/patch_cirrus.c > +++ b/sound/pci/hda/patch_cirrus.c > @@ -21,6 +21,9 @@ > /* > */ > > +#define CS42L42_HP_CH (2U) > +#define CS42L42_HS_MIC_CH (1U) > + > struct cs_spec { > struct hda_gen_spec gen; > > @@ -42,6 +45,9 @@ struct cs_spec { > > unsigned int cs42l42_hp_jack_in:1; > unsigned int cs42l42_mic_jack_in:1; > + unsigned int cs42l42_volume_init:1; can you describe what this field is? it looks like it's only tracking a one-time initialization? Yes, this field is used to track one-time initialization. Since the CS42L42 codec needs to be reset on init, the volume values need to be restored afterwards. This flag allows us to check whether we have previously cached the volume values, so they can be restored. > + char cs42l42_hp_volume[CS42L42_HP_CH]; > + char cs42l42_hs_mic_volume[CS42L42_HS_MIC_CH]; > > struct mutex cs8409_i2c_mux; > > @@ -1260,6 +1266,14 @@ static int patch_cs4213(struct hda_codec *codec) > #define CIR_I2C_QWRITE 0x005D > #define CIR_I2C_QREAD 0x005E > > +#define CS8409_CS42L42_HP_VOL_REAL_MIN (-63) > +#define CS8409_CS42L42_HP_VOL_REAL_MAX (0) > +#define CS8409_CS42L42_AMIC_VOL_REAL_MIN (-97) #define > +CS8409_CS42L42_AMIC_VOL_REAL_MAX (12) #define > +CS8409_CS42L42_REG_HS_VOLUME_CHA (0x2301) #define > +CS8409_CS42L42_REG_HS_VOLUME_CHB (0x2303) > +#define CS8409_CS42L42_REG_AMIC_VOLUME (0x1D03) > + > struct cs8409_i2c_param { > unsigned int addr; > unsigned int reg; > @@ -1580,6 +1594,165 @@ static unsigned int cs8409_i2c_write(struct hda_codec *codec, > return retval; > } > > +static int cs8409_cs42l42_volume_info(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_info *uinfo) { > + struct hda_codec *codec = snd_kcontrol_chip(kcontrol); > + u16 nid = get_amp_nid(kcontrol); > + u8 chs = get_amp_channels(kcontrol); > + > + codec_dbg(codec, "%s() nid: %d\n", __func__, nid); > + switch (nid) { > + case CS8409_CS42L42_HP_PIN_NID: > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = chs == 3 ? 2 : 1; > + uinfo->value.integer.min = CS8409_CS42L42_HP_VOL_REAL_MIN; > + uinfo->value.integer.max = CS8409_CS42L42_HP_VOL_REAL_MAX; > + break; > + case CS8409_CS42L42_AMIC_PIN_NID: > + uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER; > + uinfo->count = chs == 3 ? 2 : 1; > + uinfo->value.integer.min = CS8409_CS42L42_AMIC_VOL_REAL_MIN; > + uinfo->value.integer.max = CS8409_CS42L42_AMIC_VOL_REAL_MAX; > + break; > + default: > + break; > + } > + return 0; > +} > + > +static void cs8409_cs42l42_update_volume(struct hda_codec *codec) { > + struct cs_spec *spec = codec->spec; > + > + mutex_lock(&spec->cs8409_i2c_mux); > + spec->cs42l42_hp_volume[0] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR, > + CS8409_CS42L42_REG_HS_VOLUME_CHA, 1)); > + spec->cs42l42_hp_volume[1] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR, > + CS8409_CS42L42_REG_HS_VOLUME_CHB, 1)); > + spec->cs42l42_hs_mic_volume[0] = -(cs8409_i2c_read(codec, CS42L42_I2C_ADDR, > + CS8409_CS42L42_REG_AMIC_VOLUME, 1)); > + mutex_unlock(&spec->cs8409_i2c_mux); > + spec->cs42l42_volume_init = 1; > +} > + > +static int cs8409_cs42l42_volume_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; > + hda_nid_t nid = get_amp_nid(kcontrol); > + int chs = get_amp_channels(kcontrol); > + long *valp = ucontrol->value.integer.value; > + > + if (!spec->cs42l42_volume_init) { > + snd_hda_power_up(codec); > + cs8409_cs42l42_update_volume(codec); > + snd_hda_power_down(codec); > + } > + switch (nid) { > + case CS8409_CS42L42_HP_PIN_NID: > + if (chs & 1) BIT(0)? Will fix that. > + *valp++ = spec->cs42l42_hp_volume[0]; > + if (chs & 2) BIT(1)? Will fix that. > + *valp++ = spec->cs42l42_hp_volume[1]; > + break;
Dell's laptops Inspiron 3500, Inspiron 3501, Inspiron 3505 are using Cirrus Logic CS8409 HDA bridge with CS42L42 companion codec. The CS8409 is a multichannel HD audio routing controller. CS8409 includes support for four channels of digital microphone data and two bidirectional ASPs for up to 32 channels of TDM data or 4 channels of I2S data. The CS8409 is intended to be used with a remote companion codec that implements high performance analog functions in close physical proximity to the end-equipment audio port or speaker driver. The CS42L42 is a low-power audio codec with integrated MIPI SoundWire interface or I2C/I2S/TDM interfaces designed for portable applications. It provides a high-dynamic range, stereo DAC for audio playback and a mono high-dynamic-range ADC for audio capture Changes since version 1: ALSA: hda/cirrus: Increase AUTO_CFG_MAX_INS from 8 to 18 * No change ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec. * Removed redundant fields in fixup table * Handle gpio via spec->gpio_dir, spec->gpio_data and spec->gpio_mask * Moved cs8409_cs42l42_init() from patch 2, to handle resume correctly ALSA: hda/cirrus: Add jack detect interrupt support from CS42L42 companion codec. * Run scripts/checkpatch.pl, fixed new warnings ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control * Moved control values to cache to avoid i2c read at each time. Stefan Binding (1): ALSA: hda/cirrus: Add Headphone and Headset MIC Volume Control Vitaly Rodionov (3): ALSA: hda/cirrus: Increase AUTO_CFG_MAX_INS from 8 to 18 ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec. ALSA: hda/cirrus: Add jack detect interrupt support from CS42L42 companion codec. sound/pci/hda/hda_auto_parser.h | 2 +- sound/pci/hda/hda_local.h | 2 +- sound/pci/hda/patch_cirrus.c | 1081 +++++++++++++++++++++++++++++++ 3 files changed, 1083 insertions(+), 2 deletions(-)