Re: [PATCH 03/15] ASoC: mediatek: mt8186: support adda in platform driver

On Fri, Feb 11, 2022 at 06:38:06PM +0800, Jiaxin Yu wrote:

This looks pretty good, there's some issues below but they're all very minor, mostly stylistic things rather than anything substantial.

+// SPDX-License-Identifier: GPL-2.0 +/*

• • MediaTek ALSA SoC Audio DAI ADDA Control
• • Copyright (c) 2022 MediaTek Inc.
• • Author: Jiaxin Yu jiaxin.yu@mediatek.com
• */

Please make the entire comment a C++ one so things look more intentional.

+static int mtk_adda_ul_event(struct snd_soc_dapm_widget *w,

• 	     struct snd_kcontrol *kcontrol,
  

• 	     int event)
  

+{

• struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
• struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
• struct mt8186_afe_private *afe_priv = afe->platform_priv;
• int mtkaif_dmic = afe_priv->mtkaif_dmic;
• dev_info(afe->dev, "%s(), name %s, event 0x%x, mtkaif_dmic %d\n",

•  __func__, w->name, event, mtkaif_dmic);
  

This should be dev_dbg() at most, otherwise the logs will get very noisy (but note that there are trace points in the core which cover this). There's a bunch of other dev_info() calls like this on DAPM events.

• if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2_CLK_P2)
  

• 	regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x38);
  

• else if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2)
  

• 	regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x30);
  

• else
  

• 	regmap_write(afe->regmap, AFE_AUD_PAD_TOP, 0x30);
  

This could be more clearly written as a switch statement.

• 	if (strcmp(w->name, "ADDA_MTKAIF_CFG") == 0) {
  

• 		if (afe_priv->mtkaif_chosen_phase[0] < 0 &&
  

• 		    afe_priv->mtkaif_chosen_phase[1] < 0) {
  

• 			dev_info(afe->dev,
  

• 				 "%s(), calib fail mtkaif_chosen_phase[0/1]:%d/%d\n",
  

• 				 __func__,
  

Should this be a dev_err() given that the calibration failed?

+/* mtkaif dmic */ +static const char * const mt8186_adda_off_on_str[] = {

• "Off", "On"

+};

+static const struct soc_enum mt8186_adda_enum[] = {

• SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8186_adda_off_on_str),

• 	    mt8186_adda_off_on_str),
  

+};

This is a simple on/off control so should be a standard numeric control with a name ending in Switch to help UIs handle it properly.

+static int mt8186_adda_dmic_set(struct snd_kcontrol *kcontrol,

• 		struct snd_ctl_elem_value *ucontrol)
  

+{

• struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol);
• struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
• struct mt8186_afe_private *afe_priv = afe->platform_priv;
• struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
• int dmic_on;
• if (ucontrol->value.enumerated.item[0] >= e->items)

• return -EINVAL;
  

• dmic_on = ucontrol->value.integer.value[0];
• dev_info(afe->dev, "%s(), kcontrol name %s, dmic_on %d\n",

•  __func__, kcontrol->id.name, dmic_on);
  

• afe_priv->mtkaif_dmic = dmic_on;
• return 0;

This should return 1 if the value changed so an event is generated for userspace. You might want to run the mixer-test kselftest (ideally the version that's in -next as there were a few bits added very recently), it should detect issues like this.