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

[gabrielcsmo@gmail.com]

Hello Irina,

Please see my comments inline.

Best regards,
Cosmin

On Thu, 2019-07-04 at 13:22 +0000, Viorel Suman wrote:
> 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)

Use defines for constants instead of hardcoded values.

> +               return -1;
> 
> 
> Maybe return -EINVAL here ?
> 
> 
> +       micfil->dc_remover = val;
> +
> +       /* Calculate total value for all channels */
> +       for (i = 0; i < 8; i++)

Same as before. I think you already have MICFIL_NUM_CHANNELS in header.

> +               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);

I don't know if there is an updated manual for MICFIL but it would be
nice to have a public link.
Please make sure that this change in the configuration can be done live
witout side-effects. Modifying some of the settings were affecting
performance or functionality and you must use other mechanism for
updating the DC_CTRL if it is the case.
> 
> 
> 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))
> 
> 
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel