From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: robh@kernel.org, alsa-devel@alsa-project.org,
bgoswami@codeaurora.org, vinod.koul@linaro.org,
devicetree@vger.kernel.org, linus.walleij@linaro.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
broonie@kernel.org, lee.jones@linaro.org,
spapothi@codeaurora.org
Subject: Re: [alsa-devel] [PATCH v2 05/11] ASoC: wcd934x: add playback dapm widgets
Date: Sun, 20 Oct 2019 22:05:22 +0200 [thread overview]
Message-ID: <e0049071-7fb7-7f9a-e79f-102c1a9c8d20@intel.com> (raw)
In-Reply-To: <20191018001849.27205-6-srinivas.kandagatla@linaro.org>
On 2019-10-18 02:18, Srinivas Kandagatla wrote:
> +static int wcd934x_codec_enable_slim(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kc,
> + int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> + struct wcd934x_codec *wcd = snd_soc_component_get_drvdata(comp);
> + struct wcd_slim_codec_dai_data *dai = &wcd->dai[w->shift];
> +
> + switch (event) {
> + case SND_SOC_DAPM_POST_PMU:
> + wcd934x_codec_enable_int_port(dai, comp);
> + break;
> + case SNDRV_PCM_TRIGGER_STOP:
> + break;
Any reason for mentioning _TRIGGER_STOP here?
> + case SND_SOC_DAPM_POST_PMD:
> + kfree(dai->sconfig.chs);
> +
> + break;
Comment for kfree depending on _event_ would be advised.
> + }
> +
> + return 0;
> +}
> +
> +static void wcd934x_codec_hd2_control(struct snd_soc_component *component,
> + u16 interp_idx, int event)
> +{
> + u16 hd2_scale_reg;
> + u16 hd2_enable_reg = 0;
> +
> + switch (interp_idx) {
> + case INTERP_HPHL:
> + hd2_scale_reg = WCD934X_CDC_RX1_RX_PATH_SEC3;
> + hd2_enable_reg = WCD934X_CDC_RX1_RX_PATH_CFG0;
> + break;
> + case INTERP_HPHR:
> + hd2_scale_reg = WCD934X_CDC_RX2_RX_PATH_SEC3;
> + hd2_enable_reg = WCD934X_CDC_RX2_RX_PATH_CFG0;
> + break;
> + }
What's the rest of this function for if switch-case does not match?
Without hd2_enable_reg > 0 you might as well return immediately.
> +
> + if (hd2_enable_reg && SND_SOC_DAPM_EVENT_ON(event)) {
> + snd_soc_component_update_bits(component, hd2_scale_reg,
> + WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_MASK,
> + WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_0P3125);
> + snd_soc_component_update_bits(component, hd2_enable_reg,
> + WCD934X_CDC_RX_PATH_CFG_HD2_EN_MASK,
> + WCD934X_CDC_RX_PATH_CFG_HD2_ENABLE);
> + }
> +
> + if (hd2_enable_reg && SND_SOC_DAPM_EVENT_OFF(event)) {
> + snd_soc_component_update_bits(component, hd2_enable_reg,
> + WCD934X_CDC_RX_PATH_CFG_HD2_EN_MASK,
> + WCD934X_CDC_RX_PATH_CFG_HD2_DISABLE);
> + snd_soc_component_update_bits(component, hd2_scale_reg,
> + WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_MASK,
> + WCD934X_CDC_RX_PATH_SEC_HD2_ALPHA_0P0000);
> + }
> +}
> +
> +static void wcd934x_codec_hphdelay_lutbypass(struct snd_soc_component *comp,
> + u16 interp_idx, int event)
> +{
> + u8 hph_dly_mask;
> + u16 hph_lut_bypass_reg = 0;
> + u16 hph_comp_ctrl7 = 0;
> +
> + switch (interp_idx) {
> + case INTERP_HPHL:
> + hph_dly_mask = 1;
> + hph_lut_bypass_reg = WCD934X_CDC_TOP_HPHL_COMP_LUT;
> + hph_comp_ctrl7 = WCD934X_CDC_COMPANDER1_CTL7;
> + break;
> + case INTERP_HPHR:
> + hph_dly_mask = 2;
> + hph_lut_bypass_reg = WCD934X_CDC_TOP_HPHR_COMP_LUT;
> + hph_comp_ctrl7 = WCD934X_CDC_COMPANDER2_CTL7;
> + break;
> + default:
> + break;
> + }
'Default' made it here, what was not the case for most of other
switch-case. Keep code consistent would be appreciated.
Moreover, in the following function "wcd934x_config_compander", you do
decide to do all the processing directly within switch-case. I see no
reason why you should not do that here too.
Again, once switch-case fails to find match, the rest of function does
not do much, really.
> +
> + if (hph_lut_bypass_reg && SND_SOC_DAPM_EVENT_ON(event)) {
> + snd_soc_component_update_bits(comp, WCD934X_CDC_CLSH_TEST0,
> + hph_dly_mask, 0x0);
> + snd_soc_component_update_bits(comp, hph_lut_bypass_reg,
> + WCD934X_HPH_LUT_BYPASS_MASK,
> + WCD934X_HPH_LUT_BYPASS_ENABLE);
> + }
> +
> + if (hph_lut_bypass_reg && SND_SOC_DAPM_EVENT_OFF(event)) {
> + snd_soc_component_update_bits(comp, WCD934X_CDC_CLSH_TEST0,
> + hph_dly_mask, hph_dly_mask);
> + snd_soc_component_update_bits(comp, hph_lut_bypass_reg,
> + WCD934X_HPH_LUT_BYPASS_MASK,
> + WCD934X_HPH_LUT_BYPASS_DISABLE);
> + }
> +}
> +
> +static int wcd934x_config_compander(struct snd_soc_component *comp,
> + int interp_n, int event)
> +{
> + struct wcd934x_codec *wcd = dev_get_drvdata(comp->dev);
> + int compander;
> + u16 comp_ctl0_reg, rx_path_cfg0_reg;
> +
> + /* EAR does not have compander */
> + if (!interp_n)
> + return 0;
> +
> + compander = interp_n - 1;
> + if (!wcd->comp_enabled[compander])
> + return 0;
> +
> + comp_ctl0_reg = WCD934X_CDC_COMPANDER1_CTL0 + (compander * 8);
> + rx_path_cfg0_reg = WCD934X_CDC_RX1_RX_PATH_CFG0 + (compander * 20);
> +
> + switch (event) {
> + case SND_SOC_DAPM_PRE_PMU:
> + /* Enable Compander Clock */
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_CLK_EN_MASK,
> + WCD934X_COMP_CLK_ENABLE);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_SOFT_RST_MASK,
> + WCD934X_COMP_SOFT_RST_ENABLE);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_SOFT_RST_MASK,
> + WCD934X_COMP_SOFT_RST_DISABLE);
> + snd_soc_component_update_bits(comp, rx_path_cfg0_reg,
> + WCD934X_HPH_CMP_EN_MASK,
> + WCD934X_HPH_CMP_ENABLE);
> + break;
> + case SND_SOC_DAPM_POST_PMD:
> + snd_soc_component_update_bits(comp, rx_path_cfg0_reg,
> + WCD934X_HPH_CMP_EN_MASK,
> + WCD934X_HPH_CMP_DISABLE);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_HALT_MASK,
> + WCD934X_COMP_HALT);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_SOFT_RST_MASK,
> + WCD934X_COMP_SOFT_RST_ENABLE);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_SOFT_RST_MASK,
> + WCD934X_COMP_SOFT_RST_DISABLE);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_CLK_EN_MASK, 0x0);
> + snd_soc_component_update_bits(comp, comp_ctl0_reg,
> + WCD934X_COMP_SOFT_RST_MASK, 0x0);
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int wcd934x_codec_enable_mix_path(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kc, int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> + int offset_val = 0;
> + u16 gain_reg, mix_reg;
> + int val = 0;
> +
> + gain_reg = WCD934X_CDC_RX0_RX_VOL_MIX_CTL +
> + (w->shift * WCD934X_RX_PATH_CTL_OFFSET);
> + mix_reg = WCD934X_CDC_RX0_RX_PATH_MIX_CTL +
> + (w->shift * WCD934X_RX_PATH_CTL_OFFSET);
> +
> + switch (event) {
> + case SND_SOC_DAPM_PRE_PMU:
> + /* Clk enable */
> + snd_soc_component_update_bits(comp, mix_reg,
> + WCD934X_CDC_RX_MIX_CLK_EN_MASK,
> + WCD934X_CDC_RX_MIX_CLK_ENABLE);
> + break;
> +
> + case SND_SOC_DAPM_POST_PMU:
> + val = snd_soc_component_read32(comp, gain_reg);
> + val += offset_val;
> + snd_soc_component_write(comp, gain_reg, val);
> + break;
> + case SND_SOC_DAPM_POST_PMD:
> + break;
Redundant case?.
> + };
> +
> + return 0;
> +}
> +
> +static int wcd934x_codec_set_iir_gain(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> + int reg = w->reg;
> +
> + switch (event) {
> + case SND_SOC_DAPM_POST_PMU:
> + /* B1 GAIN */
> + snd_soc_component_write(comp, reg,
> + snd_soc_component_read32(comp, reg));
> + /* B2 GAIN */
> + reg++;
> + snd_soc_component_write(comp, reg,
> + snd_soc_component_read32(comp, reg));
> + /* B3 GAIN */
> + reg++;
> + snd_soc_component_write(comp, reg,
> + snd_soc_component_read32(comp, reg));
> + /* B4 GAIN */
> + reg++;
> + snd_soc_component_write(comp, reg,
> + snd_soc_component_read32(comp, reg));
> + /* B5 GAIN */
> + reg++;
> + snd_soc_component_write(comp, reg,
> + snd_soc_component_read32(comp, reg));
> + break;
> + default:
> + break;
> + }
> + return 0;
Missing newline before return - based on format of your other functions.
> +}
> +
> +static int wcd934x_codec_enable_main_path(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol,
> + int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> + u16 gain_reg;
> + u32 val;
> +
> + gain_reg = WCD934X_CDC_RX0_RX_VOL_CTL + (w->shift *
> + WCD934X_RX_PATH_CTL_OFFSET);
> +
> + switch (event) {
> + case SND_SOC_DAPM_POST_PMU:
> + val = snd_soc_component_read32(comp, gain_reg);
> + snd_soc_component_write(comp, gain_reg, val);
> + break;
> + };
In the function above, "wcd934x_codec_set_iir_gain", you decided against
declaring local 'val' for storing _read32, though here, only for a
single use-case, you made a difference. Let's keep it consistent and run
with one or the other.
Also, is there any value for assigning gain_reg outside of switch-case?
> +
> + return 0;
> +}
> +static int wcd934x_codec_enable_hphl_pa(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol,
> + int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> +
> + switch (event) {
> + case SND_SOC_DAPM_PRE_PMU:
> + break;
Redundant case.
> + case SND_SOC_DAPM_POST_PMU:
> + /*
> + * 7ms sleep is required after PA is enabled as per
> + * HW requirement. If compander is disabled, then
> + * 20ms delay is needed.
> + */
> + usleep_range(20000, 20100);
> +
> + snd_soc_component_update_bits(comp, WCD934X_HPH_L_TEST,
> + WCD934X_HPH_OCP_DET_MASK,
> + WCD934X_HPH_OCP_DET_ENABLE);
> + /* Remove Mute on primary path */
> + snd_soc_component_update_bits(comp, WCD934X_CDC_RX1_RX_PATH_CTL,
> + WCD934X_RX_PATH_PGA_MUTE_EN_MASK,
> + 0);
> + /* Enable GM3 boost */
> + snd_soc_component_update_bits(comp, WCD934X_HPH_CNP_WG_CTL,
> + WCD934X_HPH_GM3_BOOST_EN_MASK,
> + WCD934X_HPH_GM3_BOOST_ENABLE);
> + /* Enable AutoChop timer at the end of power up */
> + snd_soc_component_update_bits(comp,
> + WCD934X_HPH_NEW_INT_HPH_TIMER1,
> + WCD934X_HPH_AUTOCHOP_TIMER_EN_MASK,
> + WCD934X_HPH_AUTOCHOP_TIMER_ENABLE);
> + /* Remove mix path mute */
> + snd_soc_component_update_bits(comp,
> + WCD934X_CDC_RX1_RX_PATH_MIX_CTL,
> + WCD934X_CDC_RX_PGA_MUTE_EN_MASK, 0x00);
> + break;
> + case SND_SOC_DAPM_PRE_PMD:
> + /* Enable DSD Mute before PA disable */
> +
> + snd_soc_component_update_bits(comp, WCD934X_HPH_L_TEST,
> + WCD934X_HPH_OCP_DET_MASK,
> + WCD934X_HPH_OCP_DET_DISABLE);
> + snd_soc_component_update_bits(comp, WCD934X_CDC_RX1_RX_PATH_CTL,
> + WCD934X_RX_PATH_PGA_MUTE_EN_MASK,
> + WCD934X_RX_PATH_PGA_MUTE_ENABLE);
> + snd_soc_component_update_bits(comp,
> + WCD934X_CDC_RX1_RX_PATH_MIX_CTL,
> + WCD934X_RX_PATH_PGA_MUTE_EN_MASK,
> + WCD934X_RX_PATH_PGA_MUTE_ENABLE);
> + break;
> + case SND_SOC_DAPM_POST_PMD:
> + /*
> + * 5ms sleep is required after PA disable. If compander is
> + * disabled, then 20ms delay is needed after PA disable.
> + */
> + usleep_range(20000, 20100);
Superfluous identation.
> + break;
> + };
> +
> + return 0;
> +}
> +
> +static int wcd934x_codec_enable_hphr_pa(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol,
> + int event)
> +{
> + struct snd_soc_component *comp = snd_soc_dapm_to_component(w->dapm);
> +
> + switch (event) {
> + case SND_SOC_DAPM_PRE_PMU:
> + break;
Redundant case.
> + case SND_SOC_DAPM_POST_PMU:
> + /*
> + * 7ms sleep is required after PA is enabled as per
> + * HW requirement. If compander is disabled, then
> + * 20ms delay is needed.
> + */
> + usleep_range(20000, 20100);
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
next prev parent reply other threads:[~2019-10-20 20:06 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-18 0:18 [alsa-devel] [PATCH v2 00/11] ASoC: Add support to WCD9340/WCD9341 codec Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 01/11] ASoC: dt-bindings: add dt bindings for WCD9340/WCD9341 audio codec Srinivas Kandagatla
2019-10-25 20:43 ` Rob Herring
2019-10-28 12:40 ` Srinivas Kandagatla
2019-10-28 12:45 ` Srinivas Kandagatla
2019-10-29 20:47 ` Rob Herring
2019-10-30 9:55 ` Srinivas Kandagatla
2019-11-05 19:08 ` Rob Herring
2019-11-06 18:09 ` Srinivas Kandagatla
2019-11-06 19:20 ` Rob Herring
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec Srinivas Kandagatla
2019-10-21 10:46 ` Lee Jones
2019-10-21 11:25 ` Srinivas Kandagatla
2019-10-21 11:45 ` Lee Jones
2019-10-21 12:14 ` Srinivas Kandagatla
2019-10-21 13:19 ` Lee Jones
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 03/11] ASoC: " Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 04/11] ASoC: wcd934x: add basic controls Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 05/11] ASoC: wcd934x: add playback dapm widgets Srinivas Kandagatla
2019-10-20 20:05 ` Cezary Rojewski [this message]
2019-10-21 9:46 ` Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 06/11] ASoC: wcd934x: add capture " Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 07/11] ASoC: wcd934x: add audio routings Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 08/11] dt-bindings: pinctrl: qcom-wcd934x: Add bindings for gpio Srinivas Kandagatla
2019-10-25 21:00 ` Rob Herring
2019-10-28 12:41 ` Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 09/11] pinctrl: qcom-wcd934x: Add support to wcd934x pinctrl driver Srinivas Kandagatla
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 10/11] ASoC: dt-bindings: Add compatible for DB845c and Lenovo Yoga Srinivas Kandagatla
2019-10-25 21:01 ` Rob Herring
2019-10-18 0:18 ` [alsa-devel] [PATCH v2 11/11] ASoC: qcom: sdm845: add support to " Srinivas Kandagatla
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=e0049071-7fb7-7f9a-e79f-102c1a9c8d20@intel.com \
--to=cezary.rojewski@intel.com \
--cc=alsa-devel@alsa-project.org \
--cc=bgoswami@codeaurora.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=lee.jones@linaro.org \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh@kernel.org \
--cc=spapothi@codeaurora.org \
--cc=srinivas.kandagatla@linaro.org \
--cc=vinod.koul@linaro.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).