Messages in this thread | | | Subject | Re: [PATCH v3 2/4] ALSA: hda/cirrus: Add support for CS8409 HDA bridge and CS42L42 companion codec. | From | Vitaly Rodionov <> | Date | Tue, 9 Mar 2021 13:31:17 +0000 |
| |
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); >>
| |