| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 28 Nov 2023 10:22:17 +0100
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Banajit Goswami <bgoswami@...cinc.com>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>
Cc: linux-arm-msm@...r.kernel.org, alsa-devel@...a-project.org,
linux-sound@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] ASoC: codecs: Add WCD939x Codec driver
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
Powered by blists - more mailing lists