[alsa-devel] [PATCH] MLK-22197 sound: asoc: add micfil dc remover amixer control

[Viorel Suman]

Hi Irina,

Some nitpicks inline.

/Viorel

On Jo, 2019-07-04 at 12:52 +0000, Irina Patru wrote:


<snip>


+static int micfil_put_dc_remover_state(struct snd_kcontrol *kcontrol,
+                                       struct snd_ctl_elem_value *ucontrol)
+{
+       struct snd_soc_component *comp = snd_kcontrol_chip(kcontrol);
+       struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
+       struct fsl_micfil *micfil = snd_soc_component_get_drvdata(comp);
+       unsigned int *item = ucontrol->value.enumerated.item;
+       int val = snd_soc_enum_item_to_val(e, item[0]);
+       int i = 0, ret = 0;
+       u32 reg_val = 0;
+
+       if (val < 0 || val > 3)
+               return -1;


Maybe return -EINVAL here ?


+       micfil->dc_remover = val;
+
+       /* Calculate total value for all channels */
+       for (i = 0; i < 8; i++)
+               reg_val |= MICFIL_DC_MODE(val, i);
+
+       /* Update DC Remover mode for all channels */
+       ret = snd_soc_component_update_bits(comp,
+                                       REG_MICFIL_DC_CTRL,
+                                       MICFIL_DC_CTRL_MASK,
+                                       reg_val);


Please check the description of snd_soc_component_update_bits return value:
==========
 * Return: 1 if the operation was successful and the value of the register
 * changed, 0 if the operation was successful, but the value did not change.
 * Returns a negative error code otherwise.
==========

We may need to return a non-zero value in case of error only, ie:
=============
if (ret < 0)
return ret;

return 0;
=============


+       return ret;
+}
+
+static int micfil_get_dc_remover_state(struct snd_kcontrol *kcontrol,
+                                       struct snd_ctl_elem_value *ucontrol)
+{
+       struct snd_soc_component *comp = snd_kcontrol_chip(kcontrol);
+       struct fsl_micfil *micfil = snd_soc_component_get_drvdata(comp);
+
+       ucontrol->value.enumerated.item[0] = micfil->dc_remover;
+
+       return 0;
+}
+
+
 static int hwvad_put_init_mode(struct snd_kcontrol *kcontrol,
                               struct snd_ctl_elem_value *ucontrol)
 {
@@ -676,6 +731,10 @@ static const struct snd_kcontrol_new fsl_micfil_snd_controls[] = {
        SOC_ENUM_EXT("Clock Source",
                     fsl_micfil_clk_src_enum,
                     micfil_get_clk_src, micfil_put_clk_src),
+       SOC_ENUM_EXT("MICFIL DC Remover Control",
+                       fsl_micfil_dc_remover_enum,
+                       micfil_get_dc_remover_state,
+                       micfil_put_dc_remover_state),
        {
                .iface = SNDRV_CTL_ELEM_IFACE_MIXER,
                .name = "HWVAD Input Gain",
diff --git a/sound/soc/fsl/fsl_micfil.h b/sound/soc/fsl/fsl_micfil.h
index 792187b717f0..e47dba9b90b8 100644
--- a/sound/soc/fsl/fsl_micfil.h
+++ b/sound/soc/fsl/fsl_micfil.h
@@ -278,6 +278,16 @@
 #define MICFIL_HWVAD_HPF_102HZ         3
 #define MICFIL_HWVAD_FRAMET_DEFAULT    10

+/* MICFIL DC Remover Control Register -- REG_MICFIL_DC_CTRL */
+#define MICFIL_DC_CTRL_SHIFT           0
+#define MICFIL_DC_CTRL_MASK                    0xFFFF
+#define MICFIL_DC_CTRL_WIDTH           2
+#define MICFIL_DC_CHX_SHIFT(v)         (2 * (v))
+#define MICFIL_DC_CHX_MASK(v)          ((BIT(MICFIL_DC_CTRL_WIDTH) - 1) \
+                               << MICFIL_DC_CHX_SHIFT(v))
+#define MICFIL_DC_MODE(v1, v2)         (((v1) << MICFIL_DC_CHX_SHIFT(v2)) \
+                               & MICFIL_DC_CHX_MASK(v2))
+
 /* MICFIL Output Control Register */
 #define MICFIL_OUTGAIN_CHX_SHIFT(v)    (4 * (v))