Re: [PATCH 5/5] ASoC: codecs: Add WCD939x Codec driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



On 25/11/2023 13:07, Konrad Dybcio wrote:
On 23.11.2023 15:49, Neil Armstrong wrote:

<snip>

+
+static int wcd939x_io_init(struct snd_soc_component *component)
+{
+	snd_soc_component_write_field(component, WCD939X_ANA_BIAS,
+				      WCD939X_BIAS_ANALOG_BIAS_EN, 1);
All of these values are BIT()s or 2-4 ORed BIT()s, can you check what they
mean?

Same for almost all other snd_soc_component_ write/modify functions

Ack


[...]

+static int wcd939x_tx_swr_ctrl(struct snd_soc_dapm_widget *w,
+			       struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
+	struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(component);
+	int bank;
+	int rate;
+
+	bank = (wcd939x_swr_get_current_bank(wcd939x->sdw_priv[AIF1_CAP]->sdev)) ? 0 : 1;
+	bank = bank ? 0 : 1;
this is just the return value of wcd939x_swr_get_current_bank NOTed twice

Yep, I also found it wild, will simplify


+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		if (strnstr(w->name, "ADC", sizeof("ADC"))) {
+			int i = 0, mode = 0;
+
+			if (test_bit(WCD_ADC1, &wcd939x->status_mask))
+				mode |= tx_mode_bit[wcd939x->tx_mode[WCD_ADC1]];
+			if (test_bit(WCD_ADC2, &wcd939x->status_mask))
+				mode |= tx_mode_bit[wcd939x->tx_mode[WCD_ADC2]];
+			if (test_bit(WCD_ADC3, &wcd939x->status_mask))
+				mode |= tx_mode_bit[wcd939x->tx_mode[WCD_ADC3]];
+			if (test_bit(WCD_ADC4, &wcd939x->status_mask))
+				mode |= tx_mode_bit[wcd939x->tx_mode[WCD_ADC4]];
+
+			if (mode != 0) {
+				for (i = 0; i < ADC_MODE_ULP2; i++) {
+					if (mode & (1 << i)) {
BIT(i)

Ack


+						i++;
+						break;
+					}
the immediate break makes it look like you're just looking for the first set
bit, 1-indexed, and there's functions for that

Indeed, let's use that


+				}
+			}
+			rate = wcd939x_get_clk_rate(i);
+			wcd939x_set_swr_clk_rate(component, rate, bank);
+			/* Copy clk settings to active bank */
+			wcd939x_set_swr_clk_rate(component, rate, !bank);
This comment is very confusing

It is, not sure if I can clarify it more


[...]

+static void wcd939x_mbhc_get_result_params(struct snd_soc_component *component,
+					   s16 *d1_a, u16 noff,
+						int32_t *zdet)
very weird alignment

Will fix


+{
+	struct wcd939x_priv *wcd939x = snd_soc_component_get_drvdata(component);
+	int i;
+	int val, val1;
+	s16 c1;
+	s32 x1, d1;
+	s32 denom;
+	static const int mincode_param[] = {
static inside a function means something else than static global

+		3277, 1639, 820, 410, 205, 103, 52, 26
+	};
reverse-Christmas-tree

Will fix


+
+	regmap_update_bits(wcd939x->regmap, WCD939X_ANA_MBHC_ZDET, 0x20, 0x20);
+	for (i = 0; i < WCD939X_ZDET_NUM_MEASUREMENTS; i++) {
+		regmap_read(wcd939x->regmap, WCD939X_ANA_MBHC_RESULT_2, &val);
+		if (val & 0x80)
BIT(7), please name it
Will fix


+			break;
+	}
+	val = val << 0x8;
<< 8 makes more sense

Will fix


+	regmap_read(wcd939x->regmap, WCD939X_ANA_MBHC_RESULT_1, &val1);
+	val |= val1;
+	regmap_update_bits(wcd939x->regmap, WCD939X_ANA_MBHC_ZDET, 0x20, 0x00);
BIT()s
Will fix


+	x1 = WCD939X_MBHC_GET_X1(val);
+	c1 = WCD939X_MBHC_GET_C1(val);
+	/* If ramp is not complete, give additional 5ms */
+	if (c1 < 2 && x1)
+		usleep_range(5000, 5050);
udelay, see timers-howto.txt
Will fix


[...]

+static void wcd939x_wcd_mbhc_qfuse_cal(struct snd_soc_component *component,
+				       s32 *z_val, int flag_l_r)
+{
+	s16 q1;
+	int q1_cal;
+
+	q1 = snd_soc_component_read(component, WCD939X_DIGITAL_EFUSE_REG_21 + flag_l_r);
+	if (q1 & 0x80)
BIT
+		q1_cal = (10000 - ((q1 & 0x7F) * 10));
GENMASK
Will fix


+	else
+		q1_cal = (10000 + (q1 * 10));
+	if (q1_cal > 0)
+		*z_val = ((*z_val) * 10000) / q1_cal;
+}
+
+static void wcd939x_wcd_mbhc_calc_impedance(struct snd_soc_component *component,
+					    u32 *zl, uint32_t *zr)
+{
+	struct wcd939x_priv *wcd939x = dev_get_drvdata(component->dev);
+	s16 reg0, reg1, reg2, reg3, reg4;
+	s32 z1l, z1r, z1ls;
+	int z_mono, z_diff1, z_diff2;
+	bool is_fsm_disable = false;
+	struct wcd939x_mbhc_zdet_param zdet_param[] = {
const
Will fix


+		{4, 0, 4, 0x08, 0x14, 0x18}, /* < 32ohm */
+		{4, 0, 6, 0x18, 0x60, 0x78}, /* 32ohm < Z < 400ohm */
+		{1, 4, 5, 0x18, 0x7C, 0x90}, /* 400ohm < Z < 1200ohm */
+		{1, 6, 7, 0x18, 0x7C, 0x90}, /* >1200ohm */
+	};
+	struct wcd939x_mbhc_zdet_param *zdet_param_ptr = NULL;
+	s16 d1_a[][4] = {
const
Will fix


+		{0, 30, 90, 30},
+		{0, 30, 30, 6},
+		{0, 30, 30, 5},
+		{0, 30, 30, 5},
+	};
+	s16 *d1 = NULL;
????
Will fix


[...]

+
+	rc = of_property_read_u32(np, "qcom,micbias1-microvolt",  &prop_val);
similarly to the swr port map, this should be commonized for WCDxxxx

Yes, but where ?



Konrad

Thanks,
Neil




[Index of Archives]     [Pulseaudio]     [Linux Audio Users]     [ALSA Devel]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]

  Powered by Linux