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@xxxxxxxxxxxxxxxx
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
