| 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: Thu, 15 Jun 2023 13:21:29 +0200
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
Banajit Goswami <bgoswami@...cinc.com>,
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>, linux-arm-msm@...r.kernel.org,
alsa-devel@...a-project.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Cc: Patrick Lai <quic_plai@...cinc.com>
Subject: Re: [PATCH v3 2/2] ASoC: codecs: wsa884x: Add WSA884x family of
speakers
On 12/06/2023 12:18, Srinivas Kandagatla wrote:
> Thanks Krzyztof,
>
> few minor nits below.
Please trim unrelated context - it's easy to miss a comment between huge
quoted text.
>> +
>> +static const char * const wsa884x_dev_mode_text[] = {
>> + "Speaker", "Receiver"
>> +};
>> +
>> +enum wsa884x_variant {
>> + WSA8840 = 0x0,
>> + WSA8845 = 0x5,
>> + WSA8845H = 0xc,
> A proper defines for this makes more sense than a enum.
OK. I will also put the define next to respective register define.
>
O_REG(2000), },
>> + { WSA884X_ANA_WO_CTL_1, 0x00 },
>> + { WSA884X_OTP_REG_38, 0x00 },
>> + { WSA884X_OTP_REG_40, 0x8 << WSA884X_OTP_REG_40_ISENSE_RESCAL_SHIFT },
>
> FIELD macros?
Ack.
>> +};
>> +
>> +static void wsa884x_set_gain_parameters(struct wsa884x_priv *wsa884x)
>> +{
>> + struct regmap *regmap = wsa884x->regmap;
>> + unsigned int min_gain, igain, vgain, comp_offset;
>> +
>> + /*
>> + * Downstream sets gain parameters customized per boards per use-case.
>> + * Choose here some sane values matching knowon users, like QRD8550
>> + * board:.
>> + *
>> + * Values match here downstream:
>> + * For WSA884X_RECEIVER - G_7P5_DB system gain
>> + * For WSA884X_SPEAKER - G_21_DB system gain
>> + */
>> + if (wsa884x->dev_mode == WSA884X_RECEIVER) {
>> + comp_offset = COMP_OFFSET4;
>> + min_gain = G_M6_DB;
>> + igain = ISENSE_18_DB;
>> + vgain = VSENSE_M12_DB;
>> + } else {
>> + /* WSA884X_SPEAKER */
>> + comp_offset = COMP_OFFSET0;
>> + min_gain = G_0_DB;
>> + igain = ISENSE_12_DB;
>> + vgain = VSENSE_M24_DB;
>> + }
>> +
>> + regmap_update_bits(regmap, WSA884X_ISENSE2,
>> + WSA884X_ISENSE2_ISENSE_GAIN_CTL_MASK,
>> + igain << WSA884X_ISENSE2_ISENSE_GAIN_CTL_SHIFT);
>> + regmap_update_bits(regmap, WSA884X_VSENSE1,
>> + WSA884X_VSENSE1_GAIN_VSENSE_FE_MASK,
>> + vgain << WSA884X_VSENSE1_GAIN_VSENSE_FE_SHIFT);
>> + regmap_update_bits(regmap, WSA884X_GAIN_RAMPING_MIN,
>> + WSA884X_GAIN_RAMPING_MIN_MIN_GAIN_MASK,
>> + min_gain << WSA884X_GAIN_RAMPING_MIN_MIN_GAIN_SHIFT);
>> +
>> + if (wsa884x->port_enable[WSA884X_PORT_COMP]) {
>> + regmap_update_bits(regmap, WSA884X_DRE_CTL_0,
>> + WSA884X_DRE_CTL_0_OFFSET_MASK,
>> + comp_offset);
>> +
>> + regmap_update_bits(regmap, WSA884X_DRE_CTL_1,
>> + WSA884X_DRE_CTL_1_CSR_GAIN_EN_MASK,
>> + 0x0);
>> + } else {
>> + regmap_update_bits(regmap, WSA884X_DRE_CTL_1,
>> + WSA884X_DRE_CTL_1_CSR_GAIN_EN_MASK,
>> + 0x1);
>> + }
>> +}
>> +
>> +static void wsa884x_init(struct wsa884x_priv *wsa884x)
>> +{
>> + unsigned int wo_ctl_0;
>> + unsigned int variant = 0;
>> +
>> + if (!regmap_read(wsa884x->regmap, WSA884X_OTP_REG_0, &variant))
>> + wsa884x->variant = variant & WSA884X_OTP_REG_0_ID_MASK;
>> +
>> + regmap_multi_reg_write(wsa884x->regmap, wsa884x_reg_init,
>> + ARRAY_SIZE(wsa884x_reg_init));
>> +
>> + wo_ctl_0 = 0xc;
>> + wo_ctl_0 |= WSA884X_ANA_WO_CTL_0_MODE_SPEAKER << WSA884X_ANA_WO_CTL_0_MODE_SHIFT;
> FIELD helpers?
>
ack
>> + /* Assume that compander is enabled by default unless it is haptics sku */
>> + if (wsa884x->variant == WSA8845H)
>> + wo_ctl_0 |= WSA884X_ANA_WO_CTL_0_PA_AUX_18_DB << WSA884X_ANA_WO_CTL_0_PA_AUX_SHIFT;
>> + else
>> + wo_ctl_0 |= WSA884X_ANA_WO_CTL_0_PA_AUX_0_DB << WSA884X_ANA_WO_CTL_0_PA_AUX_SHIFT;
> new line
It's write of previous created/computed value, so it's unusual to split
these with new line.
>
>> + regmap_write(wsa884x->regmap, WSA884X_ANA_WO_CTL_0, wo_ctl_0);
>> +
>> + wsa884x_set_gain_parameters(wsa884x);
>> +
>> + wsa884x->hw_init = false;
>> +}
>> +
>> +static int wsa884x_update_status(struct sdw_slave *slave,
>> + enum sdw_slave_status status)
>> +{
>> + struct wsa884x_priv *wsa884x = dev_get_drvdata(&slave->dev);
>> + int ret;
>> +
>> + if (status == SDW_SLAVE_UNATTACHED)
>
> should we not put the regmap in to cache state when device is un-attached?
Makes sense. I checked few other drivers and I did not see such pattern,
but it seems reasonable.
>
>> + wsa884x->hw_init = false;
>> +
>> + if (wsa884x->hw_init || status != SDW_SLAVE_ATTACHED)
>> + return 0;
>> +
>> + regcache_cache_only(wsa884x->regmap, false);
>> + ret = regcache_sync(wsa884x->regmap);
>> + if (ret < 0) {
>> + dev_err(&slave->dev, "Cannot sync regmap cache\n");
>> + return ret;
>> + }
>> +
>> + wsa884x_init(wsa884x);
>> +
>> + return 0;
>> +}
>> +
>> +static int wsa884x_port_prep(struct sdw_slave *slave,
>> + struct sdw_prepare_ch *prepare_ch,
>> + enum sdw_port_prep_ops state)
>> +{
>> + struct wsa884x_priv *wsa884x = dev_get_drvdata(&slave->dev);
>> +
>> + if (state == SDW_OPS_PORT_POST_PREP)
>> + wsa884x->port_prepared[prepare_ch->num - 1] = true;
>> + else
>> + wsa884x->port_prepared[prepare_ch->num - 1] = false;
>> +
>> + return 0;
>> +}
>> +
>> +static const struct sdw_slave_ops wsa884x_slave_ops = {
>> + .update_status = wsa884x_update_status,
>> + .port_prep = wsa884x_port_prep,
>> +};
>> +
>> +static int wsa884x_dev_mode_get(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
>> + struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(component);
>> +
>> + ucontrol->value.enumerated.item[0] = wsa884x->dev_mode;
>> +
>> + return 0;
>> +}
>> +
>> +static int wsa884x_dev_mode_put(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
>> + struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(component);
>> +
>> + if (wsa884x->dev_mode == ucontrol->value.enumerated.item[0])
>> + return 0;
>> +
>> + wsa884x->dev_mode = ucontrol->value.enumerated.item[0];
>> +
>> + return 1;
>> +}
>> +
>> +static int wsa884x_get_swr_port(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
>> + struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(comp);
>> + struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
>> + int portidx = mixer->reg;
>> +
>> + ucontrol->value.integer.value[0] = wsa884x->port_enable[portidx];
>> +
>> + return 0;
>> +}
>> +
>> +static int wsa884x_set_swr_port(struct snd_kcontrol *kcontrol,
>> + struct snd_ctl_elem_value *ucontrol)
>> +{
>> + struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
>> + struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(comp);
>> + struct soc_mixer_control *mixer = (struct soc_mixer_control *)kcontrol->private_value;
>> + int portidx = mixer->reg;
>> +
>> + if (ucontrol->value.integer.value[0]) {
>> + if (wsa884x->port_enable[portidx])
>> + return 0;
>> +
>> + wsa884x->port_enable[portidx] = true;
>> + } else {
>> + if (!wsa884x->port_enable[portidx])
>> + return 0;
>> +
>> + wsa884x->port_enable[portidx] = false;
>> + }
>> +
>> + return 1;
>> +}
>> +
>> +static int wsa884x_codec_probe(struct snd_soc_component *comp)
>> +{
>> + struct wsa884x_priv *wsa884x = snd_soc_component_get_drvdata(comp);
>> +
>> + snd_soc_component_init_regmap(comp, wsa884x->regmap);
>> +
>> + return 0;
>> +}
>> +
>> +static void wsa884x_spkr_post_pmu(struct snd_soc_component *component,
>> + struct wsa884x_priv *wsa884x)
>> +{
>> + unsigned int curr_limit, curr_ovrd_en;
>> +
>> + wsa884x_set_gain_parameters(wsa884x);
>> + if (wsa884x->dev_mode == WSA884X_RECEIVER) {
>> + snd_soc_component_write_field(component, WSA884X_DRE_CTL_0,
>> + WSA884X_DRE_CTL_0_PROG_DELAY_MASK, 0x3);
>> + snd_soc_component_write_field(component, WSA884X_CDC_PATH_MODE,
>> + WSA884X_CDC_PATH_MODE_RXD_MODE_MASK,
>> + 0x1);
>> + snd_soc_component_write_field(component, WSA884X_PWM_CLK_CTL,
>> + WSA884X_PWM_CLK_CTL_PWM_CLK_FREQ_SEL_MASK,
>> + 0x1);
>> + } else {
>> + /* WSA884X_SPEAKER */
>> + snd_soc_component_write_field(component, WSA884X_DRE_CTL_0,
>> + WSA884X_DRE_CTL_0_PROG_DELAY_MASK, 0xf);
>> + }
>> +
>> + if (wsa884x->port_enable[WSA884X_PORT_PBR]) {
>> + curr_ovrd_en = 0x0;
>> + curr_limit = 0x15;
>
> Can we define these hardcoded values?
I don't know their meaning.
>
>> + } else {
>> + curr_ovrd_en = 0x1;
>> + if (wsa884x->dev_mode == WSA884X_RECEIVER)
>> + curr_limit = 0x9;
>> + else
>> + curr_limit = 0x15;
>> + }
>> + snd_soc_component_update_bits(component, WSA884X_CURRENT_LIMIT,
>> + WSA884X_CURRENT_LIMIT_CURRENT_LIMIT_OVRD_EN_MASK,
>> + curr_ovrd_en);
>> + snd_soc_component_update_bits(component, WSA884X_CURRENT_LIMIT,
>> + WSA884X_CURRENT_LIMIT_CURRENT_LIMIT_MASK,
>> + curr_limit << WSA884X_CURRENT_LIMIT_CURRENT_LIMIT_SHIFT);
>
> field apis should make this bit cleaner.
ack
Best regards,
Krzysztof
Powered by blists - more mailing lists