From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
Andy Gross <agross@kernel.org>,
Bjorn Andersson <andersson@kernel.org>,
Konrad Dybcio <konrad.dybcio@linaro.org>,
Banajit Goswami <bgoswami@quicinc.com>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Patrick Lai <quic_plai@quicinc.com>
Subject: Re: [PATCH v3 2/2] ASoC: codecs: wsa884x: Add WSA884x family of speakers
Date: Thu, 15 Jun 2023 13:21:29 +0200 [thread overview]
Message-ID: <8e9b62f3-d8c6-fea4-738b-c502f7167131@linaro.org> (raw)
In-Reply-To: <493895cc-378a-d908-372b-0a873cc0800e@linaro.org>
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
prev parent reply other threads:[~2023-06-15 11:21 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-06-12 9:57 [PATCH v3 1/2] ASoC: dt-bindings: qcom,wsa8840: Add WSA884x family of speakers Krzysztof Kozlowski
2023-06-12 9:57 ` [PATCH v3 2/2] ASoC: codecs: wsa884x: " Krzysztof Kozlowski
2023-06-12 10:18 ` Srinivas Kandagatla
2023-06-15 11:21 ` Krzysztof Kozlowski [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e9b62f3-d8c6-fea4-738b-c502f7167131@linaro.org \
--to=krzysztof.kozlowski@linaro.org \
--cc=agross@kernel.org \
--cc=alsa-devel@alsa-project.org \
--cc=andersson@kernel.org \
--cc=bgoswami@quicinc.com \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=konrad.dybcio@linaro.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=perex@perex.cz \
--cc=quic_plai@quicinc.com \
--cc=robh+dt@kernel.org \
--cc=srinivas.kandagatla@linaro.org \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.