Re: [PATCH v2 05/11] ASoC: wcd934x: add playback dapm widgets

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

 



Thanks Rojewski for taking time to review.

On 20/10/2019 21:05, Cezary Rojewski wrote:
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?
Looks like copy paste error.. no reason for this to be here!!


+    case SND_SOC_DAPM_POST_PMD:
+        kfree(dai->sconfig.chs);
+
+        break;

Comment for kfree depending on _event_ would be advised.

I could probably move this to hw_free callback.

+    }
+
+    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.

we could do that too! I will try to clean this bit up in next version.
+
+    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.


I agree with you! I will fix such instances + other following suggestions in all the patches!


Thanks,
srini
+}
+
+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;hd2_
+    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) {hd2_
+    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@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux