> -----Original Message-----
> From: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Sent: Tuesday, September 24, 2024 5:36 PM
> To: Jack Yu <jack.yu@xxxxxxxxxxx>; broonie@xxxxxxxxxx;
> lgirdwood@xxxxxxxxx
> Cc: alsa-devel@xxxxxxxxxxxxxxxx; lars@xxxxxxxxxx; Flove(HsinFu)
> <flove@xxxxxxxxxxx>; Oder Chiou <oder_chiou@xxxxxxxxxxx>; Shuming [范書
> 銘] <shumingf@xxxxxxxxxxx>; Derek [方德義] <derek.fang@xxxxxxxxxxx>;
> Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v2] ASoC: rt721-sdca: Add RT721 SDCA driver
>
>
> External mail.
>
>
>
> On 9/24/24 11:03, Jack Yu wrote:
> > This is the initial codec driver for rt721-sdca.
>
> I wouldn't hurt to provide a short description. 722 has 3 functions, what about
> 721?
Okay, I'll add short description here in next v3-patch.
>
>
> > + case SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC,
> RT721_SDCA_ENT_USER_FU05, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_L):
> > + case SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC,
> RT721_SDCA_ENT_USER_FU05, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_R):
> > + case SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC,
> RT721_SDCA_ENT_USER_FU0F, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_L):
> > + case SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC,
> RT721_SDCA_ENT_USER_FU0F, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_R):
> > + case SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC,
> RT721_SDCA_ENT_PLATFORM_FU44,
> > + RT721_SDCA_CTL_FU_CH_GAIN, CH_L):
> > + case SDW_SDCA_CTL(FUNC_NUM_JACK_CODEC,
> RT721_SDCA_ENT_PLATFORM_FU44,
> > + RT721_SDCA_CTL_FU_CH_GAIN, CH_R):
> > + case SDW_SDCA_CTL(FUNC_NUM_MIC_ARRAY,
> RT721_SDCA_ENT_USER_FU1E, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_01):
> > + case SDW_SDCA_CTL(FUNC_NUM_MIC_ARRAY,
> RT721_SDCA_ENT_USER_FU1E, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_02):
> > + case SDW_SDCA_CTL(FUNC_NUM_MIC_ARRAY,
> RT721_SDCA_ENT_USER_FU1E, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_03):
> > + case SDW_SDCA_CTL(FUNC_NUM_MIC_ARRAY,
> RT721_SDCA_ENT_USER_FU1E, RT721_SDCA_CTL_FU_VOLUME,
> > + CH_04):
> > + case SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_USER_FU06, RT721_SDCA_CTL_FU_VOLUME, CH_L):
> > + case SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_USER_FU06, RT721_SDCA_CTL_FU_VOLUME, CH_R):
> > + return true;
>
> That tells us it has 3 functions (jack, mic, amp), so what's different to RT722?
> could we have a single driver for both parts? A lot of this driver seems
> copy-pasted-renamed.
>
RT721 has 3 functions just like RT722, but it's still a different codec and from internal discussion,
we think it's better to separate the driver for future code management.
> > +static int rt721_sdca_read_prop(struct sdw_slave *slave) {
> > + struct sdw_slave_prop *prop = &slave->prop;
> > + int nval;
> > + int i, j;
> > + u32 bit;
> > + unsigned long addr;
> > + struct sdw_dpn_prop *dpn;
> > +
> > + sdw_slave_read_prop(slave);
>
> I thought we agreed to use a helper to read only the number of lanes from
> platform firmware?
>
> Bard, did you share this already?
>
> > + prop->scp_int1_mask = SDW_SCP_INT1_BUS_CLASH |
> SDW_SCP_INT1_PARITY;
> > + prop->quirks = SDW_SLAVE_QUIRKS_INVALID_INITIAL_PARITY;
> > +
> > + prop->paging_support = true;
> > +
> > + /*
> > + * port = 1 for headphone playback
> > + * port = 2 for headset-mic capture
> > + * port = 3 for speaker playback
> > + * port = 6 for digital-mic capture
> > + */
> > + prop->source_ports = BIT(6) | BIT(2); /* BITMAP: 01000100 */
> > + prop->sink_ports = BIT(3) | BIT(1); /* BITMAP: 00001010 */
> > +
> > + nval = hweight32(prop->source_ports);
> > + prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
> > + sizeof(*prop->src_dpn_prop), GFP_KERNEL);
> > + if (!prop->src_dpn_prop)
> > + return -ENOMEM;
> > +
> > + i = 0;
> > + dpn = prop->src_dpn_prop;
> > + addr = prop->source_ports;
> > + for_each_set_bit(bit, &addr, 32) {
> > + dpn[i].num = bit;
> > + dpn[i].type = SDW_DPN_FULL;
> > + dpn[i].simple_ch_prep_sm = true;
> > + dpn[i].ch_prep_timeout = 10;
> > + i++;
> > + }
> > +
> > + /* do this again for sink now */
> > + nval = hweight32(prop->sink_ports);
> > + prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
> > + sizeof(*prop->sink_dpn_prop), GFP_KERNEL);
> > + if (!prop->sink_dpn_prop)
> > + return -ENOMEM;
> > +
> > + j = 0;
> > + dpn = prop->sink_dpn_prop;
> > + addr = prop->sink_ports;
> > + for_each_set_bit(bit, &addr, 32) {
> > + dpn[j].num = bit;
> > + dpn[j].type = SDW_DPN_FULL;
> > + dpn[j].simple_ch_prep_sm = true;
> > + dpn[j].ch_prep_timeout = 10;
> > + j++;
> > + }
> > +
> > + /* set the timeout values */
> > + prop->clk_stop_timeout = 200;
> > +
> > + /* wake-up event */
> > + prop->wake_capable = 1;
> > +
> > + /* Three data lanes are supported by rt721-sdca codec */
> > + prop->lane_control_support = true;
>
> this doesn't seem needed if we already read information on lane support.
It seems we still need it to be set as latest upsteam code still need it.
If the upstream code is updated regarding to this, we'll do the modification to it.
>
> > +static void rt721_sdca_dmic_preset(struct rt721_sdca_priv *rt721) {
> > + /* Power down group1/2/3_mic_pdb */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_MISC_POWER_CTL31, 0x8000);
> > + /* VREF_HV_EN_AUTO_FAST */
> > + rt721_sdca_index_write(rt721, RT721_ANA_POW_PART,
> > + RT721_VREF1_HV_CTRL1, 0xe000);
> > + /* Power up group1/2/3_mic_pdb */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_MISC_POWER_CTL31, 0x8007);
> > + /* Set AD07/08 power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL9, 0x2a2a);
> > + /* Set AD10 power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL10, 0x2a00);
> > + /* Set DMIC1/DMIC2 power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL6, 0x2a2a);
> > + /* Set DMIC1/DMIC2 IT entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL5, 0x2626);
> > + /* Set AD10 FU entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL8, 0x1e00);
> > + /* Set DMIC1/DMIC2 FU input gain floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL7, 0x1515);
> > + /* Set DMIC2 FU input gain channel floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_CH_FLOAT_CTL3, 0x0304);
> > + /* Set AD10 FU channel floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_CH_FLOAT_CTL4, 0x0304);
> > + /* vf71f_r12_07_06 and vf71f_r13_07_06 = 2’b00 */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_HDA_LEGACY_CTL1, 0x0000);
> > + /* Enable vf707_r12_05/vf707_r13_05 */
> > + regmap_write(rt721->regmap,
> > + SDW_SDCA_CTL(FUNC_NUM_MIC_ARRAY,
> RT721_SDCA_ENT_IT26,
> > + RT721_SDCA_CTL_VENDOR_DEF, 0), 0x01);
> > + /* Set usd_flag_sel, usd_in_sel */
> > + regmap_write(rt721->mbq_regmap, 0x5910009, 0x2e01);
> > + /* Set RC calibration */
> > + rt721_sdca_index_write(rt721, RT721_RC_CALIB_CTRL,
> > + RT721_RC_CALIB_CTRL0, 0x0b00);
> > + rt721_sdca_index_write(rt721, RT721_RC_CALIB_CTRL,
> > + RT721_RC_CALIB_CTRL0, 0x0b40);
> > + /* Fine tune PDE2A latency */
> > + regmap_write(rt721->regmap, 0x2f5c, 0x25); }
>
> Humm, isn't all this supposed to be handled with blind writes? It seems odd to
> hard-code all this, no?
It seems there is no api or function to support blind write from ACPI from latest upstream code,
and we think it's easier for us to manage the different function's blind write.
>
> > +static void rt721_sdca_amp_preset(struct rt721_sdca_priv *rt721) {;
> > + /* Power down group1/2/3_mic_pdb */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_MISC_POWER_CTL31, 0x8000);
> > + /* VREF_HV_EN_AUTO_FAST */
> > + rt721_sdca_index_write(rt721, RT721_ANA_POW_PART,
> > + RT721_VREF1_HV_CTRL1, 0xe000);
> > + /* Power up group1/2/3_mic_pdb */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_MISC_POWER_CTL31, 0x8007);
> > + /* Reset dc_cal_top */
> > + regmap_write(rt721->mbq_regmap, 0x5810000, 0x6420);
> > + /* Turn back to normal dc_cal_top */
> > + regmap_write(rt721->mbq_regmap, 0x5810000, 0x6421);
> > + /* W1C Trigger Calibration */
> > + regmap_write(rt721->mbq_regmap, 0x5810000, 0xe421);
> > + /* DAC04 FU entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_CH_FLOAT_CTL6, 0x5561);
> > + /* Set EAPD high */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_REG,
> > + RT721_GPIO_PAD_CTRL5, 0x8003);
> > + /* Enable vf707_r14 */
> > + regmap_write(rt721->regmap,
> > + SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_OT23,
> > + RT721_SDCA_CTL_VENDOR_DEF, 0), 0x04);
> > + /* FU 23 SPK mute control - L */
> > + regmap_write(rt721->regmap,
> > + SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_PDE23,
> > + RT721_SDCA_CTL_FU_MUTE, CH_01), 0x00);
> > + /* FU 23 SPK mute control - R */
> > + regmap_write(rt721->regmap,
> > + SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_PDE23,
> > + RT721_SDCA_CTL_FU_MUTE, CH_02), 0x00);
> > + /* FU 55 DAC04 mute control - L */
> > + regmap_write(rt721->regmap,
> > + SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_FU55,
> > + RT721_SDCA_CTL_FU_MUTE, CH_01), 0x00);
> > + /* FU 55 DAC04 mute control - R */
> > + regmap_write(rt721->regmap,
> > + SDW_SDCA_CTL(FUNC_NUM_AMP,
> RT721_SDCA_ENT_FU55,
> > + RT721_SDCA_CTL_FU_MUTE, CH_02), 0x00); }
> > +
> > +static void rt721_sdca_jack_preset(struct rt721_sdca_priv *rt721) {
> > + /* Power down group1/2/3_mic_pdb */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_MISC_POWER_CTL31, 0x8000);
> > + /* VREF_HV_EN_AUTO_FAST */
> > + rt721_sdca_index_write(rt721, RT721_ANA_POW_PART,
> > + RT721_VREF1_HV_CTRL1, 0xe000);
> > + /* Power up group1/2/3_mic_pdb */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_MISC_POWER_CTL31, 0x8007);
> > + /* GE0 mode related control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_GE_REL_CTRL1, 0x8011);
> > + /* Button A, B, C, D bypass mode */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_UMP_HID_CTRL3, 0xcf00);
> > + /* HID1 slot enable */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_UMP_HID_CTRL4, 0x000f);
> > + /* Report ID for HID1 */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_UMP_HID_CTRL1, 0x1100);
> > + /* OSC/OOC for slot 2, 3 */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_UMP_HID_CTRL5, 0x0c12);
> > + /* Set JD de-bounce clock control */
> > + rt721_sdca_index_write(rt721, RT721_JD_CTRL,
> > + RT721_JD_1PIN_GAT_CTRL2, 0xc002);
> > + /* RC calibration -1 */
> > + rt721_sdca_index_write(rt721, RT721_RC_CALIB_CTRL,
> > + RT721_RC_CALIB_CTRL0, 0x0b00);
> > + /* RC calibration -2 */
> > + rt721_sdca_index_write(rt721, RT721_RC_CALIB_CTRL,
> > + RT721_RC_CALIB_CTRL0, 0x0b40);
> > + /* pow_clk_12p288mhz_dre03 change to register mode */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_UAJ_TOP_TCON14, 0x3333);
> > + /* Tune calibration timing control */
> > + regmap_write(rt721->mbq_regmap, 0x5810035, 0x0036);
> > + /* calibration HP amp output select control from Efuse */
> > + regmap_write(rt721->mbq_regmap, 0x5810030, 0xee00);
> > + /* FSM related control */
> > + rt721_sdca_index_write(rt721, RT721_CAP_PORT_CTRL,
> > + RT721_HP_AMP_2CH_CAL1, 0x0140);
> > + /* HP calibration related control */
> > + regmap_write(rt721->mbq_regmap, 0x5810000, 0x0021);
> > + /* W1C HP calibration*/
> > + regmap_write(rt721->mbq_regmap, 0x5810000, 0x8021);
> > + /* reg_sel_cin_hp_0010/0011 */
> > + rt721_sdca_index_write(rt721, RT721_CAP_PORT_CTRL,
> > + RT721_HP_AMP_2CH_CAL18, 0x5522);
> > + regmap_write(rt721->mbq_regmap, 0x5b10007, 0x2000);
> > + /* sel_sensing_lr_hp */
> > + regmap_write(rt721->mbq_regmap, 0x5B10017, 0x1b0f);
> > + /* Release HP-JD */
> > + rt721_sdca_index_write(rt721, RT721_CBJ_CTRL,
> > + RT721_CBJ_A0_GAT_CTRL1, 0x2a02);
> > + /* en_osw gating auto done bit */
> > + rt721_sdca_index_write(rt721, RT721_CAP_PORT_CTRL,
> > + RT721_HP_AMP_2CH_CAL4, 0xa105);
> > + /* pow_clk_en_sw_amp_detect_sel to register mode */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_UAJ_TOP_TCON14, 0x3b33);
> > + /* cp_sw_hp to auto mode */
> > + regmap_write(rt721->mbq_regmap, 0x310400, 0x3023);
> > + /* pow_clk_en_sw_amp_detect power up */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_UAJ_TOP_TCON14, 0x3f33);
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_UAJ_TOP_TCON13, 0x6048);
> > + /* switch size detect threshold */
> > + regmap_write(rt721->mbq_regmap, 0x310401, 0x3000);
> > + regmap_write(rt721->mbq_regmap, 0x310402, 0x1b00);
> > + /* en_hp_amp_detect auto mode */
> > + regmap_write(rt721->mbq_regmap, 0x310300, 0x000f);
> > + /* amp detect threshold */
> > + regmap_write(rt721->mbq_regmap, 0x310301, 0x3000);
> > + regmap_write(rt721->mbq_regmap, 0x310302, 0x1b00);
> > + /* gating_sdw_link_rst_n_1_cbj_reg */
> > + rt721_sdca_index_write(rt721, RT721_VENDOR_ANA_CTL,
> > + RT721_UAJ_TOP_TCON17, 0x0008);
> > + /* CKXEN_SDAC chopper function */
> > + rt721_sdca_index_write(rt721, RT721_DAC_CTRL,
> > + RT721_DAC_2CH_CTRL3, 0x55ff);
> > + /* CKXSEL_SDAC chopper frequency */
> > + rt721_sdca_index_write(rt721, RT721_DAC_CTRL,
> > + RT721_DAC_2CH_CTRL4, 0xcc00);
> > + /* Bias current for SDAC */
> > + rt721_sdca_index_write(rt721, RT721_ANA_POW_PART,
> > + RT721_MBIAS_LV_CTRL2, 0x6677);
> > + /* VREF2 level selection */
> > + rt721_sdca_index_write(rt721, RT721_ANA_POW_PART,
> > + RT721_VREF2_LV_CTRL1, 0x7600);
> > + /* ADC09/MIC2 power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL2, 0x1234);
> > + /* LINE2 power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL3, 0x3512);
> > + /* DAC03/HP power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL1, 0x4040);
> > + /* ADC27 power entity floating control */
> > + rt721_sdca_index_write(rt721, RT721_HDA_SDCA_FLOAT,
> > + RT721_ENT_FLOAT_CTL4, 0x1201);
> > + /* Fine tune PDE40 latency */
> > + regmap_write(rt721->regmap, 0x2f58, 0x07); }
>
> same here, shouldn't this come from ACPI/blind write tables?
>
Same reply as above one.
> > +enum rt721_sdca_jd_src {
> > + RT721_JD_NULL,
> > + RT721_JD1,
> > + RT721_JD2,
> > +};
>
> is this supported in SDCA? I can't recall seeing this for other drivers.
I'll remove this in next patch.
