mbox series

[v2,00/17] ASoC: mediatek: Add support for MT8186 SoC

Message ID 20220217134205.15400-1-jiaxin.yu@mediatek.com
Headers show
Series ASoC: mediatek: Add support for MT8186 SoC | expand

Message

Jiaxin Yu (俞家鑫) Feb. 17, 2022, 1:41 p.m. UTC
This series of patches adds support for Mediatek AFE of MT8186 Soc.
Patches are based on broonie tree "for-next" branch.

Changes since v1:
  [v2 01/17]
    - add a new ID to the existing mt6358 codec driver
  [v2 03/17]
    - modify log level in DAPM events
    - use standard numeric control with name ending in Switch
    - return 1 when the value changed in mixer control's .get callback
  [v2 05/17]
    - ending in Switch to the standard on/off controls
    - change to "HW Gain 1 Volume" and "HW Gain 2 Volume"
  [v2 09/17]
    - return an error in the default case rather than just picking one of
      the behaviours when do .set_fmt
    - use the new defines that are _PROVIDER_MASK, _DAIFMT_CBP_CFP and
      _DAIFMT_CBC_CFC
  [v2 10/17]
  [v2 11/17]
    - the clock and gpio are aplit out into separate  patches

  The source file's GPL comment use c++ style, and the header fils's GPL
  comment use c style. We have added "Switch" after the names of all the
  controls that just are simple on/off.

Jiaxin Yu (17):
  ASoC: mediatek: mt6366: add codec driver
  ASoC: mediatek: mt8186: support audsys clock control
  ASoC: mediatek: mt8186: support adda in platform driver
  ASoC: mediatek: mt8186: support hostless in platform driver
  ASoC: mediatek: mt8186: support hw gain in platform driver
  ASoC: mediatek: mt8186: support i2s in platform driver
  ASoC: mediatek: mt8186: support pcm in platform driver
  ASoC: mediatek: mt8186: support src in platform driver
  ASoC: mediatek: mt8186: support tdm in platform driver
  ASoC: mediatek: mt8186: support audio clock control in platform driver
  ASoC: mediatek: mt8186: support gpio control in platform driver
  ASoC: mediatek: mt8186: add platform driver
  dt-bindings: mediatek: mt8186: add audio afe document
  ASoC: mediatek: mt8186: add machine driver with mt6366, da7219 and
    max98357
  dt-bindings: mediatek: mt8186: add mt8186-mt6366-da7219-max98357
    document
  ASoC: mediatek: mt8186: add machine driver with mt6366, rt1019 and
    rt5682s
  dt-bindings: mediatek: mt8186: add mt8186-mt6366-rt1019-rt5682s
    document

 .../bindings/sound/mt8186-afe-pcm.yaml        |  175 +
 .../sound/mt8186-mt6366-da7219-max98357.yaml  |   47 +
 .../sound/mt8186-mt6366-rt1019-rt5682s.yaml   |   47 +
 sound/soc/codecs/Kconfig                      |    8 +
 sound/soc/codecs/Makefile                     |    1 +
 sound/soc/mediatek/Kconfig                    |   44 +
 sound/soc/mediatek/Makefile                   |    1 +
 sound/soc/mediatek/mt8186/Makefile            |   21 +
 sound/soc/mediatek/mt8186/mt8186-afe-clk.c    |  719 ++++
 sound/soc/mediatek/mt8186/mt8186-afe-clk.h    |  210 +
 sound/soc/mediatek/mt8186/mt8186-afe-common.h |  245 ++
 .../soc/mediatek/mt8186/mt8186-afe-control.c  |  261 ++
 sound/soc/mediatek/mt8186/mt8186-afe-gpio.c   |  210 +
 sound/soc/mediatek/mt8186/mt8186-afe-gpio.h   |   19 +
 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c    | 3029 +++++++++++++++
 sound/soc/mediatek/mt8186/mt8186-audsys-clk.c |  150 +
 sound/soc/mediatek/mt8186/mt8186-audsys-clk.h |   15 +
 .../soc/mediatek/mt8186/mt8186-audsys-clkid.h |   45 +
 sound/soc/mediatek/mt8186/mt8186-dai-adda.c   |  891 +++++
 .../soc/mediatek/mt8186/mt8186-dai-hostless.c |  295 ++
 .../soc/mediatek/mt8186/mt8186-dai-hw-gain.c  |  245 ++
 sound/soc/mediatek/mt8186/mt8186-dai-i2s.c    | 1371 +++++++
 sound/soc/mediatek/mt8186/mt8186-dai-pcm.c    |  432 +++
 sound/soc/mediatek/mt8186/mt8186-dai-src.c    |  758 ++++
 sound/soc/mediatek/mt8186/mt8186-dai-tdm.c    |  713 ++++
 .../mediatek/mt8186/mt8186-interconnection.h  |   69 +
 .../soc/mediatek/mt8186/mt8186-misc-control.c | 1728 +++++++++
 .../mt8186/mt8186-mt6366-da7219-max98357.c    |  910 +++++
 .../mt8186/mt8186-mt6366-rt1019-rt5682s.c     |  894 +++++
 sound/soc/mediatek/mt8186/mt8186-reg.h        | 3433 +++++++++++++++++
 30 files changed, 16986 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/mt8186-afe-pcm.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/mt8186-mt6366-da7219-max98357.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/mt8186-mt6366-rt1019-rt5682s.yaml
 create mode 100644 sound/soc/mediatek/mt8186/Makefile
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.h
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-common.h
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-control.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-pcm.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-audsys-clk.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-audsys-clk.h
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-audsys-clkid.h
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-adda.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-hostless.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-hw-gain.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-pcm.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-src.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-tdm.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-interconnection.h
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-misc-control.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c
 create mode 100644 sound/soc/mediatek/mt8186/mt8186-reg.h

Comments

AngeloGioacchino Del Regno Feb. 18, 2022, 2:54 p.m. UTC | #1
Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> This patch adds mt8186 hostless dai driver.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>   .../soc/mediatek/mt8186/mt8186-dai-hostless.c | 295 ++++++++++++++++++
>   1 file changed, 295 insertions(+)
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-hostless.c
> 
> diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-hostless.c b/sound/soc/mediatek/mt8186/mt8186-dai-hostless.c
> new file mode 100644
> index 000000000000..37460a3acc93
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-dai-hostless.c
> @@ -0,0 +1,295 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// MediaTek ALSA SoC Audio DAI Hostless Control
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> +
> +#include "mt8186-afe-common.h"
> +
> +static const struct snd_pcm_hardware mt8186_hostless_hardware = {
> +	.info = (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_INTERLEAVED |
> +		 SNDRV_PCM_INFO_MMAP_VALID),
> +	.period_bytes_min = 256,
> +	.period_bytes_max = 4 * 48 * 1024,
> +	.periods_min = 2,
> +	.periods_max = 256,
> +	.buffer_bytes_max = 4 * 48 * 1024,
> +	.fifo_size = 0,
> +};
> +
> +/* dai component */
> +static const struct snd_soc_dapm_route mtk_dai_hostless_routes[] = {
> +	/* Hostless ADDA Loopback */
> +	{"ADDA_DL_CH1", "ADDA_UL_CH1 Switch", "Hostless LPBK DL"},
> +	{"ADDA_DL_CH1", "ADDA_UL_CH2 Switch", "Hostless LPBK DL"},
> +	{"ADDA_DL_CH2", "ADDA_UL_CH1 Switch", "Hostless LPBK DL"},
> +	{"ADDA_DL_CH2", "ADDA_UL_CH2 Switch", "Hostless LPBK DL"},
> +	{"I2S1_CH1", "ADDA_UL_CH1 Switch", "Hostless LPBK DL"},
> +	{"I2S1_CH2", "ADDA_UL_CH2 Switch", "Hostless LPBK DL"},
> +	{"I2S3_CH1", "ADDA_UL_CH1 Switch", "Hostless LPBK DL"},
> +	{"I2S3_CH1", "ADDA_UL_CH2 Switch", "Hostless LPBK DL"},
> +	{"I2S3_CH2", "ADDA_UL_CH1 Switch", "Hostless LPBK DL"},
> +	{"I2S3_CH2", "ADDA_UL_CH2 Switch", "Hostless LPBK DL"},
> +	{"Hostless LPBK UL", NULL, "ADDA_UL_Mux"},
> +
> +	/* Hostelss FM */
> +	/* connsys_i2s to hw gain 1*/
> +	{"Hostless FM UL", NULL, "Connsys I2S"},
> +
> +	{"HW_GAIN1_IN_CH1", "CONNSYS_I2S_CH1 Switch", "Hostless FM DL"},
> +	{"HW_GAIN1_IN_CH2", "CONNSYS_I2S_CH2 Switch", "Hostless FM DL"},
> +	/* hw gain to adda dl */
> +	{"Hostless FM UL", NULL, "HW Gain 1 Out"},
> +
> +	{"ADDA_DL_CH1", "GAIN1_OUT_CH1 Switch", "Hostless FM DL"},
> +	{"ADDA_DL_CH2", "GAIN1_OUT_CH2 Switch", "Hostless FM DL"},
> +	/* hw gain to i2s3 */
> +	{"I2S3_CH1", "GAIN1_OUT_CH1 Switch", "Hostless FM DL"},
> +	{"I2S3_CH2", "GAIN1_OUT_CH2 Switch", "Hostless FM DL"},
> +	/* hw gain to i2s1 */
> +	{"I2S1_CH1", "GAIN1_OUT_CH1 Switch", "Hostless FM DL"},
> +	{"I2S1_CH2", "GAIN1_OUT_CH2 Switch", "Hostless FM DL"},
> +
> +	/* Hostless_SRC */
> +	{"ADDA_DL_CH1", "SRC_1_OUT_CH1 Switch", "Hostless_SRC_1_DL"},
> +	{"ADDA_DL_CH2", "SRC_1_OUT_CH2 Switch", "Hostless_SRC_1_DL"},
> +	{"I2S1_CH1", "SRC_1_OUT_CH1 Switch", "Hostless_SRC_1_DL"},
> +	{"I2S1_CH2", "SRC_1_OUT_CH2 Switch", "Hostless_SRC_1_DL"},
> +	{"I2S3_CH1", "SRC_1_OUT_CH1 Switch", "Hostless_SRC_1_DL"},
> +	{"I2S3_CH2", "SRC_1_OUT_CH2 Switch", "Hostless_SRC_1_DL"},
> +	{"Hostless_SRC_1_UL", NULL, "HW_SRC_1_Out"},
> +
> +	/* Hostless_SRC_bargein */
> +	{"HW_SRC_1_IN_CH1", "I2S0_CH1 Switch", "Hostless_SRC_Bargein_DL"},
> +	{"HW_SRC_1_IN_CH2", "I2S0_CH2 Switch", "Hostless_SRC_Bargein_DL"},
> +	{"Hostless_SRC_Bargein_UL", NULL, "I2S0"},
> +
> +	/* Hostless AAudio */
> +	{"Hostless HW Gain AAudio In", NULL, "HW Gain 2 In"},
> +	{"Hostless SRC AAudio UL", NULL, "HW Gain 2 Out"},
> +	{"HW_SRC_2_IN_CH1", "HW_GAIN2_OUT_CH1 Switch", "Hostless SRC AAudio DL"},
> +	{"HW_SRC_2_IN_CH2", "HW_GAIN2_OUT_CH2 Switch", "Hostless SRC AAudio DL"},
> +};
> +
> +/* dai ops */
> +static int mtk_dai_hostless_startup(struct snd_pcm_substream *substream,
> +				    struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	int ret;
> +
> +	snd_soc_set_runtime_hwparams(substream, &mt8186_hostless_hardware);
> +
> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +					    SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		dev_info(afe->dev, "snd_pcm_hw_constraint_integer failed\n");

	if (ret < 0) {
		dev_err(afe->dev, "setting constraints failed: %d\n", ret);
		return ret;
	}

	return 0;


Thanks,
Angelo
AngeloGioacchino Del Regno Feb. 18, 2022, 2:54 p.m. UTC | #2
Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> This patch adds mt8186 adda dai driver
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>   sound/soc/mediatek/mt8186/mt8186-dai-adda.c | 891 ++++++++++++++++++++
>   1 file changed, 891 insertions(+)
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> 
> diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-adda.c b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> new file mode 100644
> index 000000000000..6d7dd1533da0
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> @@ -0,0 +1,891 @@
> +// 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>
> +
> +#include <linux/regmap.h>
> +#include <linux/delay.h>
> +#include "mt8186-afe-clk.h"
> +#include "mt8186-afe-common.h"
> +#include "mt8186-afe-gpio.h"
> +#include "mt8186-interconnection.h"
> +

..snip..


> +
> +static int mtk_adda_mtkaif_cfg_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 delay_data;
> +	int delay_cycle;
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2_CLK_P2) {
> +			/* set protocol 2 */
> +			regmap_write(afe->regmap, AFE_ADDA_MTKAIF_CFG0,
> +				     0x00010000);

No leading zeros, please (also, it fits in one line):
			regmap_write(afe->regmap, AFE_ADDA_MTKAIF_CFG0, 0x10000);

> +			/* mtkaif_rxif_clkinv_adc inverse */
> +			regmap_update_bits(afe->regmap, AFE_ADDA_MTKAIF_CFG0,
> +					   MTKAIF_RXIF_CLKINV_ADC_MASK_SFT,
> +					   0x1 << MTKAIF_RXIF_CLKINV_ADC_SFT);

Please use the BIT() macro:
					   BIT(MTKAIF_RXIF_CLKINV_ADC_SFT)

> +
> +			if (strcmp(w->name, "ADDA_MTKAIF_CFG") == 0) {
> +				if (afe_priv->mtkaif_chosen_phase[0] < 0 &&
> +				    afe_priv->mtkaif_chosen_phase[1] < 0) {
> +					dev_err(afe->dev,
> +						"%s(), calib fail mtkaif_chosen_phase[0/1]:%d/%d\n",
> +						__func__,
> +						afe_priv->mtkaif_chosen_phase[0],
> +						afe_priv->mtkaif_chosen_phase[1]);
> +					break;
> +				}
> +
> +				if (afe_priv->mtkaif_chosen_phase[0] < 0 ||
> +				    afe_priv->mtkaif_chosen_phase[1] < 0) {
> +					dev_err(afe->dev,
> +						"%s(), skip dealy setting mtkaif_chosen_phase[0/1]:%d/%d\n",
> +						__func__,
> +						afe_priv->mtkaif_chosen_phase[0],
> +						afe_priv->mtkaif_chosen_phase[1]);
> +					break;
> +				}
> +			}
> +
> +			/* set delay for ch12 */
> +			if (afe_priv->mtkaif_phase_cycle[0] >=
> +			    afe_priv->mtkaif_phase_cycle[1]) {
> +				delay_data = DELAY_DATA_MISO1;
> +				delay_cycle = afe_priv->mtkaif_phase_cycle[0] -
> +					      afe_priv->mtkaif_phase_cycle[1];
> +			} else {
> +				delay_data = DELAY_DATA_MISO2;
> +				delay_cycle = afe_priv->mtkaif_phase_cycle[1] -
> +					      afe_priv->mtkaif_phase_cycle[0];
> +			}
> +
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_MTKAIF_RX_CFG2,
> +					   MTKAIF_RXIF_DELAY_DATA_MASK_SFT,
> +					   delay_data <<
> +					   MTKAIF_RXIF_DELAY_DATA_SFT);
> +
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_MTKAIF_RX_CFG2,
> +					   MTKAIF_RXIF_DELAY_CYCLE_MASK_SFT,
> +					   delay_cycle <<
> +					   MTKAIF_RXIF_DELAY_CYCLE_SFT);
> +
> +		} else if (afe_priv->mtkaif_protocol == MTKAIF_PROTOCOL_2) {
> +			regmap_write(afe->regmap, AFE_ADDA_MTKAIF_CFG0,
> +				     0x00010000);

No leading zeroes, please.

> +		} else {
> +			regmap_write(afe->regmap, AFE_ADDA_MTKAIF_CFG0, 0x0);

0x0 -> 0

> +		}
> +
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_adda_dl_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);
> +
> +	dev_dbg(afe->dev, "%s(), name %s, event 0x%x\n",
> +		__func__, w->name, event);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		mt8186_afe_gpio_request(afe->dev, true, MT8186_DAI_ADDA, 0);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		/* should delayed 1/fs(smallest is 8k) = 125us before afe off */
> +		usleep_range(125, 135);
> +		mt8186_afe_gpio_request(afe->dev, false, MT8186_DAI_ADDA, 0);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt8186_adda_dmic_get(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;
> +
> +	ucontrol->value.integer.value[0] = afe_priv->mtkaif_dmic;
> +
> +	return 0;
> +}
> +
> +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);

Shouldn't this be a dev_dbg() instead?

> +
> +	if (afe_priv->mtkaif_dmic == dmic_on)
> +		return 0;
> +
> +	afe_priv->mtkaif_dmic = dmic_on;
> +
> +	return 1;
> +}
> +

...snip...

> +
> +#define HIRES_THRESHOLD 48000
> +static int mtk_afe_dac_hires_connect(struct snd_soc_dapm_widget *source,
> +				     struct snd_soc_dapm_widget *sink)
> +{
> +	struct snd_soc_dapm_widget *w = source;
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
> +	struct mtk_afe_adda_priv *adda_priv;
> +
> +	adda_priv = get_adda_priv_by_name(afe, w->name);
> +
> +	if (!adda_priv) {
> +		dev_info(afe->dev, "%s(), adda_priv == NULL", __func__);

dev_err()

> +		return 0;
> +	}
> +
> +	return (adda_priv->dl_rate > HIRES_THRESHOLD) ? 1 : 0;
> +}
> +
> +static int mtk_afe_adc_hires_connect(struct snd_soc_dapm_widget *source,
> +				     struct snd_soc_dapm_widget *sink)
> +{
> +	struct snd_soc_dapm_widget *w = source;
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
> +	struct mtk_afe_adda_priv *adda_priv;
> +
> +	adda_priv = get_adda_priv_by_name(afe, w->name);
> +
> +	if (!adda_priv) {
> +		dev_info(afe->dev, "%s(), adda_priv == NULL", __func__);

dev_err()

> +		return 0;
> +	}
> +
> +	return (adda_priv->ul_rate > HIRES_THRESHOLD) ? 1 : 0;
> +}
> +

..snip..

> +};
> +
> +/* dai ops */
> +static int mtk_dai_adda_hw_params(struct snd_pcm_substream *substream,
> +				  struct snd_pcm_hw_params *params,
> +				  struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	unsigned int rate = params_rate(params);
> +	int id = dai->id;
> +	struct mtk_afe_adda_priv *adda_priv = afe_priv->dai_priv[id];
> +
> +	dev_info(afe->dev, "%s(), id %d, stream %d, rate %d\n",
> +		 __func__,
> +		 id,
> +		 substream->stream,
> +		 rate);
> +
> +	if (!adda_priv) {
> +		dev_info(afe->dev, "%s(), adda_priv == NULL", __func__);
> +		return -EINVAL;
> +	}
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		unsigned int dl_src2_con0 = 0;
> +		unsigned int dl_src2_con1 = 0;

This initialization is redundant: you're never using these variables
before initializing them later, so initializing them to zero is not
needed here.

> +
> +		adda_priv->dl_rate = rate;
> +
> +		/* set sampling rate */
> +		dl_src2_con0 = adda_dl_rate_transform(afe, rate) <<
> +			       DL_2_INPUT_MODE_CTL_SFT;
> +
> +		/* set output mode, UP_SAMPLING_RATE_X8 */
> +		dl_src2_con0 |= (0x3 << DL_2_OUTPUT_SEL_CTL_SFT);
> +
> +		/* turn off mute function */
> +		dl_src2_con0 |= (0x01 << DL_2_MUTE_CH2_OFF_CTL_PRE_SFT);

BIT() macro, please

> +		dl_src2_con0 |= (0x01 << DL_2_MUTE_CH1_OFF_CTL_PRE_SFT);
> +
> +		/* set voice input data if input sample rate is 8k or 16k */
> +		if (rate == 8000 || rate == 16000)
> +			dl_src2_con0 |= 0x01 << DL_2_VOICE_MODE_CTL_PRE_SFT;
> +
> +		/* SA suggest apply -0.3db to audio/speech path */
> +		dl_src2_con1 = MTK_AFE_ADDA_DL_GAIN_NORMAL <<
> +			       DL_2_GAIN_CTL_PRE_SFT;
> +
> +		/* turn on down-link gain */
> +		dl_src2_con0 |= (0x01 << DL_2_GAIN_ON_CTL_PRE_SFT);
> +
> +		if (id == MT8186_DAI_ADDA) {
> +			/* clean predistortion */
> +			regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON0, 0);
> +			regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON1, 0);
> +
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_DL_SRC2_CON0, dl_src2_con0);
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_DL_SRC2_CON1, dl_src2_con1);
> +
> +			/* set sdm gain */
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_DL_SDM_DCCOMP_CON,
> +					   ATTGAIN_CTL_MASK_SFT,
> +					   AUDIO_SDM_LEVEL_NORMAL <<
> +					   ATTGAIN_CTL_SFT);
> +
> +			/* Use new 2nd sdm */
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_DL_SDM_DITHER_CON,
> +					   AFE_DL_SDM_DITHER_64TAP_EN_MASK_SFT,
> +					   0x1 << AFE_DL_SDM_DITHER_64TAP_EN_SFT);

BIT(AFE_DL_SDM_DITHER_64TAP_EN_SFT)

> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_DL_SDM_AUTO_RESET_CON,
> +					   AFE_DL_USE_NEW_2ND_SDM_MASK_SFT,
> +					   0x1 << AFE_DL_USE_NEW_2ND_SDM_SFT);

BIT(AFE_DL_USE_NEW_2ND_SDM_SFT)

> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_DL_SDM_DCCOMP_CON,
> +					   USE_3RD_SDM_MASK_SFT,
> +					   AUDIO_SDM_2ND << USE_3RD_SDM_SFT);
> +
> +			/* sdm auto reset */
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_DL_SDM_AUTO_RESET_CON,
> +				     SDM_AUTO_RESET_THRESHOLD);
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_DL_SDM_AUTO_RESET_CON,
> +					   SDM_AUTO_RESET_TEST_ON_MASK_SFT,
> +					   0x1 << SDM_AUTO_RESET_TEST_ON_SFT);

BIT(SDM_AUTO_RESET_TEST_ON_SFT)

> +		}
> +	} else {
> +		unsigned int voice_mode = 0;

what about...
		unsigned int ul_src_con0 = 0; /* default value */
		unsigned int voice_mode =  adda_ul_rate_transform(afe, rate);

> +		unsigned int ul_src_con0 = 0;	/* default value */
> +
> +		adda_priv->ul_rate = rate;
> +
> +		voice_mode = adda_ul_rate_transform(afe, rate);
> +
> +		ul_src_con0 |= (voice_mode << 17) & (0x7 << 17);
> +
> +		/* enable iir */
> +		ul_src_con0 |= (1 << UL_IIR_ON_TMP_CTL_SFT) &
> +			       UL_IIR_ON_TMP_CTL_MASK_SFT;
> +		ul_src_con0 |= (UL_IIR_SW << UL_IIRMODE_CTL_SFT) &
> +			       UL_IIRMODE_CTL_MASK_SFT;
> +		switch (id) {
> +		case MT8186_DAI_ADDA:
> +		case MT8186_DAI_AP_DMIC:
> +			/* 35Hz @ 48k */
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_IIR_COEF_02_01, 0x00000000);

Please drop leading zeroes:

regmap_write(afe->regmap, AFE_ADDA_IIR_COEF_02_01, 0);

> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_IIR_COEF_04_03, 0x00003FB8);

... and also please write hex in lower-case:

regmap_write(afe->regmap,
	     AFE_ADDA_IIR_COEF_04_03, 0x03fb8);

> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_IIR_COEF_06_05, 0x3FB80000);
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_IIR_COEF_08_07, 0x3FB80000);
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_IIR_COEF_10_09, 0x0000C048);
> +
> +			regmap_write(afe->regmap,
> +				     AFE_ADDA_UL_SRC_CON0, ul_src_con0);
> +
> +			/* Using Internal ADC */
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_TOP_CON0,
> +					   0x1 << 0,
> +					   0x0 << 0);

Please use the BIT() macro:

regmap_update_bits(afe->regmap, AFE_ADDA_TOP_CON0, BIT(0), 0);

P.S.: 87 columns is ok

> +
> +			/* mtkaif_rxif_data_mode = 0, amic */
> +			regmap_update_bits(afe->regmap,
> +					   AFE_ADDA_MTKAIF_RX_CFG0,
> +					   0x1 << 0,
> +					   0x0 << 0);

same here.

> +			break;
> +		default:
> +			break;
> +		}
> +
> +		/* ap dmic */
> +		switch (id) {
> +		case MT8186_DAI_AP_DMIC:
> +			mtk_adda_ul_src_dmic(afe, id);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +

Regards,
Angelo
AngeloGioacchino Del Regno Feb. 18, 2022, 2:54 p.m. UTC | #3
Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> This patch adds mt8186 i2s dai driver
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>   sound/soc/mediatek/mt8186/mt8186-dai-i2s.c | 1371 ++++++++++++++++++++
>   1 file changed, 1371 insertions(+)
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> 
> diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-i2s.c b/sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> new file mode 100644
> index 000000000000..d6db5f6a7315
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> @@ -0,0 +1,1371 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// MediaTek ALSA SoC Audio DAI I2S Control
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> +
> +#include <linux/bitops.h>
> +#include <linux/regmap.h>
> +#include <sound/pcm_params.h>
> +#include "mt8186-afe-clk.h"
> +#include "mt8186-afe-common.h"
> +#include "mt8186-afe-gpio.h"
> +#include "mt8186-interconnection.h"
> +
> +enum {
> +	I2S_FMT_EIAJ = 0,
> +	I2S_FMT_I2S = 1,
> +};
> +
> +enum {
> +	I2S_WLEN_16_BIT = 0,
> +	I2S_WLEN_32_BIT = 1,
> +};
> +
> +enum {
> +	I2S_HD_NORMAL = 0,
> +	I2S_HD_LOW_JITTER = 1,
> +};
> +
> +enum {
> +	I2S1_SEL_O28_O29 = 0,
> +	I2S1_SEL_O03_O04 = 1,
> +};
> +
> +enum {
> +	I2S_IN_PAD_CONNSYS = 0,
> +	I2S_IN_PAD_IO_MUX = 1,
> +};
> +
> +struct mtk_afe_i2s_priv {
> +	int id;
> +	int rate; /* for determine which apll to use */
> +	int low_jitter_en;
> +	int master; /* only i2s0 has slave mode*/
> +
> +	const char *share_property_name;
> +	int share_i2s_id;
> +
> +	int mclk_id;
> +	int mclk_rate;
> +	int mclk_apll;
> +};
> +
> +static unsigned int get_i2s_wlen(snd_pcm_format_t format)
> +{
> +	return snd_pcm_format_physical_width(format) <= 16 ?
> +	       I2S_WLEN_16_BIT : I2S_WLEN_32_BIT;
> +}
> +
> +#define MTK_AFE_I2S0_KCONTROL_NAME "I2S0_HD_Mux"
> +#define MTK_AFE_I2S1_KCONTROL_NAME "I2S1_HD_Mux"
> +#define MTK_AFE_I2S2_KCONTROL_NAME "I2S2_HD_Mux"
> +#define MTK_AFE_I2S3_KCONTROL_NAME "I2S3_HD_Mux"
> +#define MTK_AFE_I2S0_SRC_KCONTROL_NAME "I2S0_SRC_Mux"
> +
> +#define I2S0_HD_EN_W_NAME "I2S0_HD_EN"
> +#define I2S1_HD_EN_W_NAME "I2S1_HD_EN"
> +#define I2S2_HD_EN_W_NAME "I2S2_HD_EN"
> +#define I2S3_HD_EN_W_NAME "I2S3_HD_EN"
> +
> +#define I2S0_MCLK_EN_W_NAME "I2S0_MCLK_EN"
> +#define I2S1_MCLK_EN_W_NAME "I2S1_MCLK_EN"
> +#define I2S2_MCLK_EN_W_NAME "I2S2_MCLK_EN"
> +#define I2S3_MCLK_EN_W_NAME "I2S3_MCLK_EN"
> +
> +static int get_i2s_id_by_name(struct mtk_base_afe *afe,
> +			      const char *name)
> +{
> +	if (strncmp(name, "I2S0", 4) == 0)
> +		return MT8186_DAI_I2S_0;
> +	else if (strncmp(name, "I2S1", 4) == 0)
> +		return MT8186_DAI_I2S_1;
> +	else if (strncmp(name, "I2S2", 4) == 0)
> +		return MT8186_DAI_I2S_2;
> +	else if (strncmp(name, "I2S3", 4) == 0)
> +		return MT8186_DAI_I2S_3;

This is just

	return -EINVAL;

(without the "else").

> +	else
> +		return -EINVAL;
> +}
> +
> +static struct mtk_afe_i2s_priv *get_i2s_priv_by_name(struct mtk_base_afe *afe,
> +						     const char *name)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int dai_id = get_i2s_id_by_name(afe, name);
> +
> +	if (dai_id < 0)
> +		return NULL;
> +
> +	return afe_priv->dai_priv[dai_id];
> +}
> +
> +/* low jitter control */
> +static const char * const mt8186_i2s_hd_str[] = {
> +	"Normal", "Low_Jitter"
> +};
> +
> +static const struct soc_enum mt8186_i2s_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8186_i2s_hd_str),
> +			    mt8186_i2s_hd_str),
> +};
> +
> +/* clock source control */
> +static const char * const mt8186_i2s_src_str[] = {
> +	"Master", "Slave"
> +};
> +
> +static const struct soc_enum mt8186_i2s_src_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8186_i2s_src_str),
> +			    mt8186_i2s_src_str),
> +};
> +
> +static int mt8186_i2s_hd_get(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 mtk_afe_i2s_priv *i2s_priv;
> +
> +	i2s_priv = get_i2s_priv_by_name(afe, kcontrol->id.name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	ucontrol->value.integer.value[0] = i2s_priv->low_jitter_en;
> +
> +	return 0;
> +}
> +
> +static int mt8186_i2s_hd_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 mtk_afe_i2s_priv *i2s_priv;
> +	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
> +	int hd_en;
> +
> +	if (ucontrol->value.enumerated.item[0] >= e->items)
> +		return -EINVAL;
> +
> +	hd_en = ucontrol->value.integer.value[0];
> +
> +	dev_info(afe->dev, "%s(), kcontrol name %s, hd_en %d\n",
> +		 __func__, kcontrol->id.name, hd_en);

dev_dbg()

> +
> +	i2s_priv = get_i2s_priv_by_name(afe, kcontrol->id.name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	i2s_priv->low_jitter_en = hd_en;
> +
> +	return 0;
> +}
> +
> +static int mt8186_i2s_src_get(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 mtk_afe_i2s_priv *i2s_priv;
> +
> +	i2s_priv = get_i2s_priv_by_name(afe, kcontrol->id.name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	ucontrol->value.integer.value[0] = i2s_priv->master;
> +
> +	return 0;
> +}
> +
> +static int mt8186_i2s_src_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 mtk_afe_i2s_priv *i2s_priv;
> +	struct soc_enum *e = (struct soc_enum *)kcontrol->private_value;
> +	int clk_src;
> +	int dai_id;
> +
> +	if (ucontrol->value.enumerated.item[0] >= e->items)
> +		return -EINVAL;
> +
> +	clk_src = ucontrol->value.integer.value[0];
> +
> +	dev_info(afe->dev, "%s(), kcontrol name %s, hd_en %d\n",
> +		 __func__, kcontrol->id.name, clk_src);

dev_dbg()

> +
> +	i2s_priv = get_i2s_priv_by_name(afe, kcontrol->id.name);
> +	dai_id = get_i2s_id_by_name(afe, kcontrol->id.name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	switch (dai_id) {
> +	case MT8186_DAI_I2S_0:
> +		regmap_update_bits(afe->regmap, AFE_I2S_CON,
> +				   I2S_SRC_MASK_SFT,
> +				   clk_src << I2S_SRC_SFT);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	i2s_priv->master = clk_src;
> +
> +	return 0;
> +}
> +

..snip..

> +
> +static int mtk_i2s_en_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 mtk_afe_i2s_priv *i2s_priv;
> +
> +	i2s_priv = get_i2s_priv_by_name(afe, w->name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> +		__func__, w->name, event);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		mt8186_afe_gpio_request(afe->dev, true, i2s_priv->id, 0);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		mt8186_afe_gpio_request(afe->dev, false, i2s_priv->id, 0);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_apll_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);
> +
> +	dev_dbg(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> +		__func__, w->name, event);
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		if (strcmp(w->name, APLL1_W_NAME) == 0)
> +			mt8186_apll1_enable(afe);
> +		else
> +			mt8186_apll2_enable(afe);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		if (strcmp(w->name, APLL1_W_NAME) == 0)
> +			mt8186_apll1_disable(afe);
> +		else
> +			mt8186_apll2_disable(afe);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mtk_mclk_en_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 mtk_afe_i2s_priv *i2s_priv;
> +
> +	dev_dbg(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> +		__func__, w->name, event);
> +
> +	i2s_priv = get_i2s_priv_by_name(afe, w->name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_PRE_PMU:
> +		mt8186_mck_enable(afe, i2s_priv->mclk_id, i2s_priv->mclk_rate);
> +		break;
> +	case SND_SOC_DAPM_POST_PMD:
> +		i2s_priv->mclk_rate = 0;
> +		mt8186_mck_disable(afe, i2s_priv->mclk_id);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +

..snip..

> +
> +static int mtk_afe_i2s_share_connect(struct snd_soc_dapm_widget *source,
> +				     struct snd_soc_dapm_widget *sink)
> +{
> +	struct snd_soc_dapm_widget *w = sink;
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
> +	struct mtk_afe_i2s_priv *i2s_priv;
> +
> +	i2s_priv = get_i2s_priv_by_name(afe, sink->name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

Is this an error? => dev_err()
Is this expected? => dev_dbg()

> +		return 0;
> +	}
> +
> +	if (i2s_priv->share_i2s_id < 0)
> +		return 0;
> +
> +	return i2s_priv->share_i2s_id == get_i2s_id_by_name(afe, source->name);
> +}
> +
> +static int mtk_afe_i2s_hd_connect(struct snd_soc_dapm_widget *source,
> +				  struct snd_soc_dapm_widget *sink)
> +{
> +	struct snd_soc_dapm_widget *w = sink;
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +	struct mtk_base_afe *afe = snd_soc_component_get_drvdata(cmpnt);
> +	struct mtk_afe_i2s_priv *i2s_priv;
> +
> +	i2s_priv = get_i2s_priv_by_name(afe, sink->name);
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

Is this an error? => dev_err()
Is this expected? => dev_dbg()

Please fix all of the other instances of this.

> +		return 0;
> +	}
> +
> +	if (get_i2s_id_by_name(afe, sink->name) ==
> +	    get_i2s_id_by_name(afe, source->name))
> +		return i2s_priv->low_jitter_en;
> +
> +	/* check if share i2s need hd en */
> +	if (i2s_priv->share_i2s_id < 0)
> +		return 0;
> +
> +	if (i2s_priv->share_i2s_id == get_i2s_id_by_name(afe, source->name))
> +		return i2s_priv->low_jitter_en;
> +
> +	return 0;
> +}
> +

..snip...

> +
> +/* dai ops */
> +static int mtk_dai_connsys_i2s_hw_params(struct snd_pcm_substream *substream,
> +					 struct snd_pcm_hw_params *params,
> +					 struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	unsigned int rate = params_rate(params);
> +	unsigned int rate_reg = mt8186_rate_transform(afe->dev,
> +						      rate, dai->id);
> +	unsigned int i2s_con = 0;
> +
> +	dev_info(afe->dev, "%s(), id %d, stream %d, rate %d\n",
> +		 __func__,
> +		 dai->id,
> +		 substream->stream,
> +		 rate);
> +
> +	/* non-inverse, i2s mode, slave, 16bits, from connsys */
> +	i2s_con |= 0 << INV_PAD_CTRL_SFT;
> +	i2s_con |= I2S_FMT_I2S << I2S_FMT_SFT;
> +	i2s_con |= 1 << I2S_SRC_SFT;
> +	i2s_con |= get_i2s_wlen(SNDRV_PCM_FORMAT_S16_LE) << I2S_WLEN_SFT;
> +	i2s_con |= 0 << I2SIN_PAD_SEL_SFT;
> +	regmap_write(afe->regmap, AFE_CONNSYS_I2S_CON, i2s_con);
> +
> +	/* use asrc */
> +	regmap_update_bits(afe->regmap,
> +			   AFE_CONNSYS_I2S_CON,
> +			   I2S_BYPSRC_MASK_SFT,
> +			   0x0 << I2S_BYPSRC_SFT);

Zero shifted of a billion bits is still zero.

regmap_update_bits(afe->regmap, AFE_CONNSYS_I2S_CON, I2S_BYPSRC_MASK_SFT, 0);

> +
> +	/* slave mode, set i2s for asrc */
> +	regmap_update_bits(afe->regmap,
> +			   AFE_CONNSYS_I2S_CON,
> +			   I2S_MODE_MASK_SFT,
> +			   rate_reg << I2S_MODE_SFT);

	regmap_update_bits(afe->regmap, AFE_CONNSYS_I2S_CON,

			   I2S_MODE_MASK_SFT, rate_reg << I2S_MODE_SFT);

> +
> +	if (rate == 44100)
> +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001B9000);

lower case hex, please, and no leading zeros.

> +	else if (rate == 32000)
> +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000);
> +	else
> +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001E0000);
> +
> +	/* Calibration setting */
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986);

ditto.

> +
> +	/* 0:Stereo 1:Mono */
> +	regmap_update_bits(afe->regmap,
> +			   AFE_ASRC_2CH_CON2,
> +			   CHSET_IS_MONO_MASK_SFT,
> +			   0x0 << CHSET_IS_MONO_SFT);

0 << SOMETHING = 0

Also, this fits in one line.

> +
> +	return 0;
> +}
> +
> +static int mtk_dai_connsys_i2s_trigger(struct snd_pcm_substream *substream,
> +				       int cmd, struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +
> +	dev_info(afe->dev, "%s(), cmd %d, stream %d\n",
> +		 __func__,
> +		 cmd,
> +		 substream->stream);

dev_dbg(), also fits in two lines.

> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		/* i2s enable */
> +		regmap_update_bits(afe->regmap,
> +				   AFE_CONNSYS_I2S_CON,
> +				   I2S_EN_MASK_SFT,
> +				   0x1 << I2S_EN_SFT);

BIT()

> +
> +		/* calibrator enable */
> +		regmap_update_bits(afe->regmap,
> +				   AFE_ASRC_2CH_CON5,
> +				   CALI_EN_MASK_SFT,
> +				   0x1 << CALI_EN_SFT);

BIT()

> +
> +		/* asrc enable */
> +		regmap_update_bits(afe->regmap,
> +				   AFE_ASRC_2CH_CON0,
> +				   CON0_CHSET_STR_CLR_MASK_SFT,
> +				   0x1 << CON0_CHSET_STR_CLR_SFT);

BIT()

> +		regmap_update_bits(afe->regmap,
> +				   AFE_ASRC_2CH_CON0,
> +				   CON0_ASM_ON_MASK_SFT,
> +				   0x1 << CON0_ASM_ON_SFT);

BIT()

> +
> +		afe_priv->dai_on[dai->id] = true;
> +		return 0;
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +		regmap_update_bits(afe->regmap,
> +				   AFE_ASRC_2CH_CON0,
> +				   CON0_ASM_ON_MASK_SFT,
> +				   0 << CON0_ASM_ON_SFT);

This is zero.

> +		regmap_update_bits(afe->regmap,
> +				   AFE_ASRC_2CH_CON5,
> +				   CALI_EN_MASK_SFT,
> +				   0 << CALI_EN_SFT);

Zero again.

> +
> +		/* i2s disable */
> +		regmap_update_bits(afe->regmap,
> +				   AFE_CONNSYS_I2S_CON,
> +				   I2S_EN_MASK_SFT,
> +				   0x0 << I2S_EN_SFT);

...and again.

> +
> +		/* bypass asrc */
> +		regmap_update_bits(afe->regmap,
> +				   AFE_CONNSYS_I2S_CON,
> +				   I2S_BYPSRC_MASK_SFT,
> +				   0x1 << I2S_BYPSRC_SFT);

BIT()

> +
> +		afe_priv->dai_on[dai->id] = false;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static const struct snd_soc_dai_ops mtk_dai_connsys_i2s_ops = {
> +	.hw_params = mtk_dai_connsys_i2s_hw_params,
> +	.trigger = mtk_dai_connsys_i2s_trigger,
> +};
> +
> +/* i2s */
> +static int mtk_dai_i2s_config(struct mtk_base_afe *afe,
> +			      struct snd_pcm_hw_params *params,
> +			      int i2s_id)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	struct mtk_afe_i2s_priv *i2s_priv = afe_priv->dai_priv[i2s_id];
> +
> +	unsigned int rate = params_rate(params);
> +	unsigned int rate_reg = mt8186_rate_transform(afe->dev,
> +						      rate, i2s_id);

Fits on a single line.

> +	snd_pcm_format_t format = params_format(params);
> +	unsigned int i2s_con = 0;
> +	int ret = 0;
> +
> +	dev_info(afe->dev, "%s(), id %d, rate %d, format %d\n",
> +		 __func__,
> +		 i2s_id,
> +		 rate, format);

dev_dbg(), fits on two lines.

> +
> +	if (i2s_priv)
> +		i2s_priv->rate = rate;
> +	else
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

I'm not sure about this print, maybe this should also be dev_dbg()

> +
> +	switch (i2s_id) {
> +	case MT8186_DAI_I2S_0:
> +		i2s_con = I2S_IN_PAD_IO_MUX << I2SIN_PAD_SEL_SFT;
> +		i2s_con |= rate_reg << I2S_OUT_MODE_SFT;
> +		i2s_con |= I2S_FMT_I2S << I2S_FMT_SFT;
> +		i2s_con |= get_i2s_wlen(format) << I2S_WLEN_SFT;
> +		regmap_update_bits(afe->regmap, AFE_I2S_CON,
> +				   0xffffeffa, i2s_con);
> +		break;
> +	case MT8186_DAI_I2S_1:
> +		i2s_con = I2S1_SEL_O28_O29 << I2S2_SEL_O03_O04_SFT;
> +		i2s_con |= rate_reg << I2S2_OUT_MODE_SFT;
> +		i2s_con |= I2S_FMT_I2S << I2S2_FMT_SFT;
> +		i2s_con |= get_i2s_wlen(format) << I2S2_WLEN_SFT;
> +		regmap_update_bits(afe->regmap, AFE_I2S_CON1,
> +				   0xffffeffa, i2s_con);
> +		break;
> +	case MT8186_DAI_I2S_2:
> +		i2s_con = 8 << I2S3_UPDATE_WORD_SFT;
> +		i2s_con |= rate_reg << I2S3_OUT_MODE_SFT;
> +		i2s_con |= I2S_FMT_I2S << I2S3_FMT_SFT;
> +		i2s_con |= get_i2s_wlen(format) << I2S3_WLEN_SFT;
> +		regmap_update_bits(afe->regmap, AFE_I2S_CON2,
> +				   0xffffeffa, i2s_con);
> +		break;
> +	case MT8186_DAI_I2S_3:
> +		i2s_con = rate_reg << I2S4_OUT_MODE_SFT;
> +		i2s_con |= I2S_FMT_I2S << I2S4_FMT_SFT;
> +		i2s_con |= get_i2s_wlen(format) << I2S4_WLEN_SFT;
> +		regmap_update_bits(afe->regmap, AFE_I2S_CON3,
> +				   0xffffeffa, i2s_con);
> +		break;
> +	default:
> +		dev_info(afe->dev, "%s(), id %d not support\n",
> +			 __func__, i2s_id);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	/* set share i2s */
> +	if (i2s_priv && i2s_priv->share_i2s_id >= 0)
> +		ret = mtk_dai_i2s_config(afe, params, i2s_priv->share_i2s_id);
> +

	if (i2s_priv && i2s_priv->share_i2s_id >= 0) {

		ret = mtk_dai_i2s_config(afe, params, i2s_priv->share_i2s_id);

		if (ret)

			return ret;

	}



	return 0;

> +	return ret;
> +}
> +
> +static int mtk_dai_i2s_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *params,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> +
> +	return mtk_dai_i2s_config(afe, params, dai->id);
> +}
> +
> +static int mtk_dai_i2s_set_sysclk(struct snd_soc_dai *dai,
> +				  int clk_id, unsigned int freq, int dir)
> +{
> +	struct mtk_base_afe *afe = dev_get_drvdata(dai->dev);
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	struct mtk_afe_i2s_priv *i2s_priv = afe_priv->dai_priv[dai->id];
> +	int apll;
> +	int apll_rate;
> +
> +	if (!i2s_priv) {
> +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);

dev_err()

> +		return -EINVAL;
> +	}
> +
> +	if (dir != SND_SOC_CLOCK_OUT) {
> +		dev_info(afe->dev, "%s(), dir != SND_SOC_CLOCK_OUT", __func__);

again...

> +		return -EINVAL;
> +	}
> +
> +	dev_info(afe->dev, "%s(), freq %d\n", __func__, freq);

dev_dbg()

> +
> +	apll = mt8186_get_apll_by_rate(afe, freq);
> +	apll_rate = mt8186_get_apll_rate(afe, apll);
> +
> +	if (freq > apll_rate) {
> +		dev_info(afe->dev, "%s(), freq > apll rate", __func__);

dev_err() .... please fix the rest as well.

> +		return -EINVAL;
> +	}
> +
> +	if (apll_rate % freq != 0) {
> +		dev_info(afe->dev, "%s(), APLL cannot generate freq Hz", __func__);
> +		return -EINVAL;
> +	}
> +
> +	i2s_priv->mclk_rate = freq;
> +	i2s_priv->mclk_apll = apll;
> +
> +	if (i2s_priv->share_i2s_id > 0) {
> +		struct mtk_afe_i2s_priv *share_i2s_priv;
> +
> +		share_i2s_priv = afe_priv->dai_priv[i2s_priv->share_i2s_id];
> +		if (!share_i2s_priv) {
> +			dev_info(afe->dev, "%s(), share_i2s_priv == NULL", __func__);
> +			return -EINVAL;
> +		}
> +
> +		share_i2s_priv->mclk_rate = i2s_priv->mclk_rate;
> +		share_i2s_priv->mclk_apll = i2s_priv->mclk_apll;
> +	}
> +
> +	return 0;
> +}
> +

Regards,
Angelo
AngeloGioacchino Del Regno Feb. 18, 2022, 2:54 p.m. UTC | #4
Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> This patch add audio clock control with CCF interface.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>   sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 719 +++++++++++++++++++++
>   sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 210 ++++++
>   2 files changed, 929 insertions(+)
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.c
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> 
> diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-clk.c b/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> new file mode 100644
> index 000000000000..14f64b935619
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> @@ -0,0 +1,719 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// mt8186-afe-clk.c  --  Mediatek 8186 afe clock ctrl
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> +
> +#include <linux/clk.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#include "mt8186-afe-common.h"
> +#include "mt8186-afe-clk.h"
> +#include "mt8186-audsys-clk.h"
> +
> +static DEFINE_MUTEX(mutex_request_dram);
> +
> +static const char *aud_clks[CLK_NUM] = {
> +	[CLK_AFE] = "aud_afe_clk",
> +	[CLK_DAC] = "aud_dac_clk",
> +	[CLK_DAC_PREDIS] = "aud_dac_predis_clk",
> +	[CLK_ADC] = "aud_adc_clk",
> +	[CLK_TML] = "aud_tml_clk",
> +	[CLK_APLL22M] = "aud_apll22m_clk",
> +	[CLK_APLL24M] = "aud_apll24m_clk",
> +	[CLK_APLL1_TUNER] = "aud_apll_tuner_clk",
> +	[CLK_APLL2_TUNER] = "aud_apll2_tuner_clk",
> +	[CLK_TDM] = "aud_tdm_clk",
> +	[CLK_NLE] = "aud_nle_clk",
> +	[CLK_DAC_HIRES] = "aud_dac_hires_clk",
> +	[CLK_ADC_HIRES] = "aud_adc_hires_clk",
> +	[CLK_I2S1_BCLK] = "aud_i2s1_bclk",
> +	[CLK_I2S2_BCLK] = "aud_i2s2_bclk",
> +	[CLK_I2S3_BCLK] = "aud_i2s3_bclk",
> +	[CLK_I2S4_BCLK] = "aud_i2s4_bclk",
> +	[CLK_CONNSYS_I2S_ASRC] = "aud_connsys_i2s_asrc",
> +	[CLK_GENERAL1_ASRC] = "aud_general1_asrc",
> +	[CLK_GENERAL2_ASRC] = "aud_general2_asrc",
> +	[CLK_ADC_HIRES_TML] = "aud_adc_hires_tml",
> +	[CLK_ADDA6_ADC] = "aud_adda6_adc",
> +	[CLK_ADDA6_ADC_HIRES] = "aud_adda6_adc_hires",
> +	[CLK_3RD_DAC] = "aud_3rd_dac",
> +	[CLK_3RD_DAC_PREDIS] = "aud_3rd_dac_predis",
> +	[CLK_3RD_DAC_TML] = "aud_3rd_dac_tml",
> +	[CLK_3RD_DAC_HIRES] = "aud_3rd_dac_hires",
> +	[CLK_ETDM_IN1_BCLK] = "aud_etdm_in1_bclk",
> +	[CLK_ETDM_OUT1_BCLK] = "aud_etdm_out1_bclk",
> +	[CLK_INFRA_SYS_AUDIO] = "aud_infra_clk",
> +	[CLK_INFRA_AUDIO_26M] = "mtkaif_26m_clk",
> +	[CLK_MUX_AUDIO] = "top_mux_audio",
> +	[CLK_MUX_AUDIOINTBUS] = "top_mux_audio_int",
> +	[CLK_TOP_MAINPLL_D2_D4] = "top_mainpll_d2_d4",
> +	[CLK_TOP_MUX_AUD_1] = "top_mux_aud_1",
> +	[CLK_TOP_APLL1_CK] = "top_apll1_ck",
> +	[CLK_TOP_MUX_AUD_2] = "top_mux_aud_2",
> +	[CLK_TOP_APLL2_CK] = "top_apll2_ck",
> +	[CLK_TOP_MUX_AUD_ENG1] = "top_mux_aud_eng1",
> +	[CLK_TOP_APLL1_D8] = "top_apll1_d8",
> +	[CLK_TOP_MUX_AUD_ENG2] = "top_mux_aud_eng2",
> +	[CLK_TOP_APLL2_D8] = "top_apll2_d8",
> +	[CLK_TOP_MUX_AUDIO_H] = "top_mux_audio_h",
> +	[CLK_TOP_I2S0_M_SEL] = "top_i2s0_m_sel",
> +	[CLK_TOP_I2S1_M_SEL] = "top_i2s1_m_sel",
> +	[CLK_TOP_I2S2_M_SEL] = "top_i2s2_m_sel",
> +	[CLK_TOP_I2S4_M_SEL] = "top_i2s4_m_sel",
> +	[CLK_TOP_TDM_M_SEL] = "top_tdm_m_sel",
> +	[CLK_TOP_APLL12_DIV0] = "top_apll12_div0",
> +	[CLK_TOP_APLL12_DIV1] = "top_apll12_div1",
> +	[CLK_TOP_APLL12_DIV2] = "top_apll12_div2",
> +	[CLK_TOP_APLL12_DIV4] = "top_apll12_div4",
> +	[CLK_TOP_APLL12_DIV_TDM] = "top_apll12_div_tdm",
> +	[CLK_CLK26M] = "top_clk26m_clk",
> +};
> +
> +int mt8186_set_audio_int_bus_parent(struct mtk_base_afe *afe,
> +				    int clk_id)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret;
> +
> +	ret = clk_set_parent(afe_priv->clk[CLK_MUX_AUDIOINTBUS],
> +			     afe_priv->clk[clk_id]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS],
> +			 aud_clks[clk_id], ret);

		dev_err(......)
		return ret;

> +	}
> +

	return 0;

> +	return ret;
> +}
> +
> +static int apll1_mux_setting(struct mtk_base_afe *afe, bool enable)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret = 0;
> +
> +	if (enable) {
> +		ret = clk_prepare_enable(afe_priv->clk[CLK_TOP_MUX_AUD_1]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1], ret);
> +			goto EXIT;
> +		}
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_1],
> +				     afe_priv->clk[CLK_TOP_APLL1_CK]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> +				 aud_clks[CLK_TOP_APLL1_CK], ret);

dev_err()

> +			goto EXIT;
> +		}
> +
> +		/* 180.6336 / 8 = 22.5792MHz */
> +		ret = clk_prepare_enable(afe_priv->clk[CLK_TOP_MUX_AUD_ENG1]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_ENG1], ret);
> +			goto EXIT;
> +		}
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_ENG1],
> +				     afe_priv->clk[CLK_TOP_APLL1_D8]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_ENG1],
> +				 aud_clks[CLK_TOP_APLL1_D8], ret);
> +			goto EXIT;
> +		}
> +	} else {
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_ENG1],
> +				     afe_priv->clk[CLK_CLK26M]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_ENG1],
> +				 aud_clks[CLK_CLK26M], ret);
> +			goto EXIT;
> +		}
> +		clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_ENG1]);
> +
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_1],
> +				     afe_priv->clk[CLK_CLK26M]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> +				 aud_clks[CLK_CLK26M], ret);
> +			goto EXIT;
> +		}
> +		clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_1]);
> +	}
> +EXIT:
> +	return 0;

You're returning 0 even in error cases, this is wrong.

> +}
> +
> +static int apll2_mux_setting(struct mtk_base_afe *afe, bool enable)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret = 0;
> +
> +	if (enable) {
> +		ret = clk_prepare_enable(afe_priv->clk[CLK_TOP_MUX_AUD_2]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2], ret);
> +			goto EXIT;
> +		}
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
> +				     afe_priv->clk[CLK_TOP_APLL2_CK]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> +				 aud_clks[CLK_TOP_APLL2_CK], ret);
> +			goto EXIT;
> +		}
> +
> +		/* 196.608 / 8 = 24.576MHz */
> +		ret = clk_prepare_enable(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_ENG2], ret);
> +			goto EXIT;
> +		}
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2],
> +				     afe_priv->clk[CLK_TOP_APLL2_D8]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_ENG2],
> +				 aud_clks[CLK_TOP_APLL2_D8], ret);
> +			goto EXIT;
> +		}
> +	} else {
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2],
> +				     afe_priv->clk[CLK_CLK26M]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_ENG2],
> +				 aud_clks[CLK_CLK26M], ret);
> +			goto EXIT;
> +		}
> +		clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2]);
> +
> +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
> +				     afe_priv->clk[CLK_CLK26M]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> +				 aud_clks[CLK_CLK26M], ret);
> +			goto EXIT;
> +		}
> +		clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_2]);
> +	}
> +
> +EXIT:
> +	return 0;
> +}
> +
> +int mt8186_afe_enable_cgs(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret = 0;
> +	int i;
> +
> +	for (i = CLK_I2S1_BCLK; i <= CLK_ETDM_OUT1_BCLK; i++) {
> +		ret = clk_prepare_enable(afe_priv->clk[i]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +				 __func__, aud_clks[i], ret);

dev_err()

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +void mt8186_afe_disable_cgs(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int i;
> +
> +	for (i = CLK_I2S1_BCLK; i <= CLK_ETDM_OUT1_BCLK; i++)
> +		clk_disable_unprepare(afe_priv->clk[i]);
> +}
> +
> +int mt8186_afe_enable_clock(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret = 0;
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_INFRA_SYS_AUDIO], ret);

dev_err()

> +		goto CLK_INFRA_SYS_AUDIO_ERR;

also, please use lower-case labels (here and everywhere else).

> +	}
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_INFRA_AUDIO_26M], ret);
> +		goto CLK_INFRA_AUDIO_26M_ERR;
> +	}
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIO]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_MUX_AUDIO], ret);
> +		goto CLK_MUX_AUDIO_ERR;
> +	}
> +	ret = clk_set_parent(afe_priv->clk[CLK_MUX_AUDIO],
> +			     afe_priv->clk[CLK_CLK26M]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +			 __func__, aud_clks[CLK_MUX_AUDIO],
> +			 aud_clks[CLK_CLK26M], ret);
> +		goto CLK_MUX_AUDIO_ERR;
> +	}
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> +	}
> +	ret = mt8186_set_audio_int_bus_parent(afe,
> +					      CLK_TOP_MAINPLL_D2_D4);
> +	if (ret)
> +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> +
> +	ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUDIO_H],
> +			     afe_priv->clk[CLK_TOP_APLL2_CK]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> +			 __func__, aud_clks[CLK_TOP_MUX_AUDIO_H],
> +			 aud_clks[CLK_TOP_APLL2_CK], ret);
> +		goto CLK_MUX_AUDIO_H_PARENT_ERR;
> +	}
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_AFE]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_AFE], ret);
> +		goto CLK_AFE_ERR;
> +	}
> +
> +	return 0;
> +
> +CLK_AFE_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_AFE]);
> +CLK_MUX_AUDIO_H_PARENT_ERR:
> +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> +CLK_MUX_AUDIO_INTBUS_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +CLK_MUX_AUDIO_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIO]);
> +CLK_INFRA_SYS_AUDIO_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> +CLK_INFRA_AUDIO_26M_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> +
> +	return ret;
> +}
> +
> +void mt8186_afe_disable_clock(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +
> +	clk_disable_unprepare(afe_priv->clk[CLK_AFE]);
> +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIO]);
> +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> +}
> +
> +int mt8186_afe_suspend_clock(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret;
> +
> +	/* set audio int bus to 26M */
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);

dev_err() - here and for the other similar instances.

> +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> +	}
> +	ret = mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> +	if (ret)
> +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> +
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +
> +	return 0;
> +
> +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> +	mt8186_set_audio_int_bus_parent(afe, CLK_TOP_MAINPLL_D2_D4);
> +CLK_MUX_AUDIO_INTBUS_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +	return ret;
> +}
> +
> +int mt8186_afe_resume_clock(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret;
> +
> +	/* set audio int bus to normal working clock */
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> +	}
> +	ret = mt8186_set_audio_int_bus_parent(afe,
> +					      CLK_TOP_MAINPLL_D2_D4);
> +	if (ret)
> +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> +
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +
> +	return 0;
> +
> +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> +CLK_MUX_AUDIO_INTBUS_ERR:
> +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> +	return ret;
> +}
> +
> +int mt8186_apll1_enable(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret;
> +
> +	/* setting for APLL */
> +	apll1_mux_setting(afe, true);
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL22M]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_APLL22M], ret);
> +		goto ERR_CLK_APLL22M;
> +	}
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL1_TUNER]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_APLL1_TUNER], ret);
> +		goto ERR_CLK_APLL1_TUNER;
> +	}
> +
> +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
> +			   0x0000FFF7, 0x00000832);

no leading zeroes - and without them, it fits in one line.

> +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG, 0x1, 0x1);
> +
> +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> +			   AFE_22M_ON_MASK_SFT,
> +			   0x1 << AFE_22M_ON_SFT);

BIT() macro please

> +
> +	return 0;
> +
> +ERR_CLK_APLL1_TUNER:
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL1_TUNER]);
> +ERR_CLK_APLL22M:
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
> +
> +	return ret;
> +}
> +
> +void mt8186_apll1_disable(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +
> +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> +			   AFE_22M_ON_MASK_SFT,
> +			   0x0 << AFE_22M_ON_SFT);
> +
> +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG, 0x1, 0x0);
> +
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL1_TUNER]);
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
> +
> +	apll1_mux_setting(afe, false);
> +}
> +
> +int mt8186_apll2_enable(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int ret;
> +
> +	/* setting for APLL */
> +	apll2_mux_setting(afe, true);
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL24M]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_APLL24M], ret);
> +		goto ERR_CLK_APLL24M;
> +	}
> +
> +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL2_TUNER]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[CLK_APLL2_TUNER], ret);
> +		goto ERR_CLK_APLL2_TUNER;
> +	}
> +
> +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
> +			   0x0000FFF7, 0x00000634);
> +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG, 0x1, 0x1);
> +
> +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> +			   AFE_24M_ON_MASK_SFT,
> +			   0x1 << AFE_24M_ON_SFT);
> +
> +	return 0;
> +
> +ERR_CLK_APLL2_TUNER:
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL2_TUNER]);
> +ERR_CLK_APLL24M:
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
> +
> +	return ret;
> +}
> +
> +void mt8186_apll2_disable(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +
> +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> +			   AFE_24M_ON_MASK_SFT,
> +			   0x0 << AFE_24M_ON_SFT);
> +
> +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG, 0x1, 0x0);
> +
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL2_TUNER]);
> +	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
> +
> +	apll2_mux_setting(afe, false);
> +}
> +
> +int mt8186_get_apll_rate(struct mtk_base_afe *afe, int apll)
> +{
> +	return (apll == MT8186_APLL1) ? 180633600 : 196608000;
> +}
> +
> +int mt8186_get_apll_by_rate(struct mtk_base_afe *afe, int rate)
> +{
> +	return ((rate % 8000) == 0) ? MT8186_APLL2 : MT8186_APLL1;
> +}
> +
> +int mt8186_get_apll_by_name(struct mtk_base_afe *afe, const char *name)
> +{
> +	if (strcmp(name, APLL1_W_NAME) == 0)
> +		return MT8186_APLL1;
> +	else
> +		return MT8186_APLL2;
> +}
> +
> +/* mck */
> +struct mt8186_mck_div {
> +	int m_sel_id;
> +	int div_clk_id;
> +	/* below will be deprecated */
> +	int div_pdn_reg;
> +	int div_pdn_mask_sft;
> +	int div_reg;
> +	int div_mask_sft;
> +	int div_mask;
> +	int div_sft;
> +	int div_msb_reg;
> +	int div_msb_mask_sft;
> +	int div_msb_mask;
> +	int div_msb_sft;
> +	int div_apll_sel_reg;
> +	int div_apll_sel_mask_sft;
> +	int div_apll_sel_sft;
> +	int div_inv_reg;
> +	int div_inv_mask_sft;



*_reg fits in u16.
*_mask_sft, *_sel_sft can be u32.

This would be nice to avoid wasting memory for variables that are larger than
needed; besides, you're also using a signed type for a number that may not
ever be less than zero.


> +};
> +
> +static const struct mt8186_mck_div mck_div[MT8186_MCK_NUM] = {
> +	[MT8186_I2S0_MCK] = {
> +		.m_sel_id = CLK_TOP_I2S0_M_SEL,
> +		.div_clk_id = CLK_TOP_APLL12_DIV0,
> +		.div_pdn_reg = CLK_AUDDIV_0,
> +		.div_pdn_mask_sft = APLL12_DIV0_PDN_MASK_SFT,
> +		.div_reg = CLK_AUDDIV_2,
> +		.div_mask_sft = APLL12_CK_DIV0_MASK_SFT,
> +		.div_mask = APLL12_CK_DIV0_MASK,
> +		.div_sft = APLL12_CK_DIV0_SFT,
> +		.div_apll_sel_reg = CLK_AUDDIV_0,
> +		.div_apll_sel_mask_sft = APLL_I2S0_MCK_SEL_MASK_SFT,
> +		.div_apll_sel_sft = APLL_I2S0_MCK_SEL_SFT,
> +	},
> +	[MT8186_I2S1_MCK] = {
> +		.m_sel_id = CLK_TOP_I2S1_M_SEL,
> +		.div_clk_id = CLK_TOP_APLL12_DIV1,
> +		.div_pdn_reg = CLK_AUDDIV_0,
> +		.div_pdn_mask_sft = APLL12_DIV1_PDN_MASK_SFT,
> +		.div_reg = CLK_AUDDIV_2,
> +		.div_mask_sft = APLL12_CK_DIV1_MASK_SFT,
> +		.div_mask = APLL12_CK_DIV1_MASK,
> +		.div_sft = APLL12_CK_DIV1_SFT,
> +		.div_apll_sel_reg = CLK_AUDDIV_0,
> +		.div_apll_sel_mask_sft = APLL_I2S1_MCK_SEL_MASK_SFT,
> +		.div_apll_sel_sft = APLL_I2S1_MCK_SEL_SFT,
> +	},
> +	[MT8186_I2S2_MCK] = {
> +		.m_sel_id = CLK_TOP_I2S2_M_SEL,
> +		.div_clk_id = CLK_TOP_APLL12_DIV2,
> +		.div_pdn_reg = CLK_AUDDIV_0,
> +		.div_pdn_mask_sft = APLL12_DIV2_PDN_MASK_SFT,
> +		.div_reg = CLK_AUDDIV_2,
> +		.div_mask_sft = APLL12_CK_DIV2_MASK_SFT,
> +		.div_mask = APLL12_CK_DIV2_MASK,
> +		.div_sft = APLL12_CK_DIV2_SFT,
> +		.div_apll_sel_reg = CLK_AUDDIV_0,
> +		.div_apll_sel_mask_sft = APLL_I2S2_MCK_SEL_MASK_SFT,
> +		.div_apll_sel_sft = APLL_I2S2_MCK_SEL_SFT,
> +	},
> +	[MT8186_I2S4_MCK] = {
> +		.m_sel_id = CLK_TOP_I2S4_M_SEL,
> +		.div_clk_id = CLK_TOP_APLL12_DIV4,
> +		.div_pdn_reg = CLK_AUDDIV_0,
> +		.div_pdn_mask_sft = APLL12_DIV4_PDN_MASK_SFT,
> +		.div_reg = CLK_AUDDIV_2,
> +		.div_mask_sft = APLL12_CK_DIV4_MASK_SFT,
> +		.div_mask = APLL12_CK_DIV4_MASK,
> +		.div_sft = APLL12_CK_DIV4_SFT,
> +		.div_apll_sel_reg = CLK_AUDDIV_0,
> +		.div_apll_sel_mask_sft = APLL_I2S4_MCK_SEL_MASK_SFT,
> +		.div_apll_sel_sft = APLL_I2S4_MCK_SEL_SFT,
> +	},
> +	[MT8186_TDM_MCK] = {
> +		.m_sel_id = CLK_TOP_TDM_M_SEL,
> +		.div_clk_id = CLK_TOP_APLL12_DIV_TDM,
> +		.div_pdn_reg = CLK_AUDDIV_0,
> +		.div_pdn_mask_sft = APLL12_DIV_TDM_PDN_MASK_SFT,
> +		.div_reg = CLK_AUDDIV_3,
> +		.div_mask_sft = APLL12_CK_DIV_TDM_MASK_SFT,
> +		.div_mask = APLL12_CK_DIV_TDM_MASK,
> +		.div_sft = APLL12_CK_DIV_TDM_SFT,
> +		.div_apll_sel_reg = CLK_AUDDIV_0,
> +		.div_apll_sel_mask_sft = APLL_TDM_MCK_SEL_MASK_SFT,
> +		.div_apll_sel_sft = APLL_TDM_MCK_SEL_SFT,
> +	},
> +};
> +
> +int mt8186_mck_enable(struct mtk_base_afe *afe, int mck_id, int rate)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int apll = mt8186_get_apll_by_rate(afe, rate);
> +	int apll_clk_id = apll == MT8186_APLL1 ?
> +			  CLK_TOP_MUX_AUD_1 : CLK_TOP_MUX_AUD_2;
> +	int m_sel_id = mck_div[mck_id].m_sel_id;
> +	int div_clk_id = mck_div[mck_id].div_clk_id;
> +	int ret;
> +
> +	/* select apll */
> +	if (m_sel_id >= 0) {
> +		ret = clk_prepare_enable(afe_priv->clk[m_sel_id]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s(), clk_prepare_enable %s fail %d\n",
> +				 __func__, aud_clks[m_sel_id], ret);

dev_err()

> +			return ret;
> +		}
> +		ret = clk_set_parent(afe_priv->clk[m_sel_id],
> +				     afe_priv->clk[apll_clk_id]);
> +		if (ret) {
> +			dev_info(afe->dev, "%s(), clk_set_parent %s-%s fail %d\n",
> +				 __func__, aud_clks[m_sel_id],
> +				aud_clks[apll_clk_id], ret);

again

> +			return ret;
> +		}
> +	}
> +
> +	/* enable div, set rate */
> +	ret = clk_prepare_enable(afe_priv->clk[div_clk_id]);
> +	if (ret) {
> +		dev_info(afe->dev, "%s(), clk_prepare_enable %s fail %d\n",
> +			 __func__, aud_clks[div_clk_id], ret);

again

> +		return ret;
> +	}
> +	ret = clk_set_rate(afe_priv->clk[div_clk_id], rate);
> +	if (ret) {
> +		dev_info(afe->dev, "%s(), clk_set_rate %s, rate %d, fail %d\n",
> +			 __func__, aud_clks[div_clk_id],
> +			 rate, ret);

again - and this fits in two lines.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +void mt8186_mck_disable(struct mtk_base_afe *afe, int mck_id)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	int m_sel_id = mck_div[mck_id].m_sel_id;
> +	int div_clk_id = mck_div[mck_id].div_clk_id;
> +
> +	clk_disable_unprepare(afe_priv->clk[div_clk_id]);
> +	if (m_sel_id >= 0)
> +		clk_disable_unprepare(afe_priv->clk[m_sel_id]);
> +}
> +
> +int mt8186_init_clock(struct mtk_base_afe *afe)
> +{
> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> +	struct device_node *of_node = afe->dev->of_node;
> +	int i = 0;
> +
> +	 mt8186_audsys_clk_register(afe);

Fix indentation please

> +
> +	afe_priv->clk = devm_kcalloc(afe->dev, CLK_NUM, sizeof(*afe_priv->clk),
> +				     GFP_KERNEL);
> +	if (!afe_priv->clk)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < CLK_NUM; i++) {
> +		afe_priv->clk[i] = devm_clk_get(afe->dev, aud_clks[i]);
> +		if (IS_ERR(afe_priv->clk[i])) {
> +			dev_info(afe->dev, "%s devm_clk_get %s fail, ret %ld\n",
> +				 __func__,
> +				 aud_clks[i], PTR_ERR(afe_priv->clk[i]));
> +			afe_priv->clk[i] = NULL;
> +		}
> +	}
> +
> +	afe_priv->apmixedsys = syscon_regmap_lookup_by_phandle(of_node,
> +							       "mediatek,apmixedsys");
> +	if (IS_ERR(afe_priv->apmixedsys)) {
> +		dev_err(afe->dev, "%s() Cannot find apmixedsys controller: %ld\n",
> +			__func__, PTR_ERR(afe_priv->apmixedsys));
> +		return PTR_ERR(afe_priv->apmixedsys);
> +	}
> +
> +	afe_priv->topckgen = syscon_regmap_lookup_by_phandle(of_node,
> +							     "mediatek,topckgen");
> +	if (IS_ERR(afe_priv->topckgen)) {
> +		dev_err(afe->dev, "%s() Cannot find topckgen controller: %ld\n",
> +			__func__, PTR_ERR(afe_priv->topckgen));
> +		return PTR_ERR(afe_priv->topckgen);
> +	}
> +
> +	afe_priv->infracfg = syscon_regmap_lookup_by_phandle(of_node,
> +							     "mediatek,infracfg");
> +	if (IS_ERR(afe_priv->infracfg)) {
> +		dev_err(afe->dev, "%s() Cannot find infracfg: %ld\n",
> +			__func__, PTR_ERR(afe_priv->infracfg));
> +		return PTR_ERR(afe_priv->infracfg);
> +	}
> +
> +	return 0;
> +}
> +
> +void mt8186_deinit_clock(struct mtk_base_afe *afe)
> +{
> +	mt8186_audsys_clk_unregister(afe);
> +}
> diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-clk.h b/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> new file mode 100644
> index 000000000000..3ce7a9a24d4a
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> @@ -0,0 +1,210 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * mt8186-afe-clk.h  --  Mediatek 8186 afe clock ctrl definition
> + *
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> + */
> +
> +#ifndef _MT8186_AFE_CLOCK_CTRL_H_
> +#define _MT8186_AFE_CLOCK_CTRL_H_
> +
> +#define PERI_BUS_DCM_CTRL 0x0074
> +#define MODULE_SW_CG_1_STA 0x0094
> +#define MODULE_SW_CG_2_STA 0x00ac
> +#define CLK_CFG_7 0x0080
> +#define CLK_CFG_8 0x0090
> +#define CLK_CFG_11 0x00c0
> +#define CLK_CFG_12 0x00d0
> +#define CLK_CFG_13 0x00e0
> +#define CLK_CFG_15 0x0100
> +#define AP_PLL_CON3 0x0014
> +#define APLL1_CON4 0x0328
> +#define APLL1_TUNER_CON0 0x0040
> +#define APLL2_CON4 0x033c
> +#define APLL2_TUNER_CON0 0x0044
> +
> +#define AP_PLL_CON5 0x0014
> +#define APLL1_CON0 0x02c0
> +#define APLL1_CON1 0x02c4
> +#define APLL1_CON2 0x02c8
> +#define APLL1_CON3 0x02cc
> +#define APLL1_PWR_CON0 0x02d0
> +
> +#define APLL2_CON0 0x02d4
> +#define APLL2_CON1 0x02d8
> +#define APLL2_CON2 0x02dc
> +#define APLL2_CON3 0x02e0
> +#define APLL2_PWR_CON0 0x02e4
> +
> +#define APMIXEDSYS_MAX_LENGTH APLL2_PWR_CON0
> +
> +#define CLK_CFG_6 0x0080
> +#define CLK_AUDDIV_0 0x0320
> +#define CLK_AUDDIV_1 0x0324
> +#define CLK_AUDDIV_2 0x0328
> +#define CKSYS_AUD_TOP_CFG 0x032c
> +#define CKSYS_AUD_TOP_MON 0x0330
> +#define CLK_AUDDIV_3 0x0334
> +
> +#define CLK_MAX_LENGTH CLK_AUDDIV_3
> +
> +/* CLK_CFG_6 */
> +#define CLK_AUD_INTBUS_SEL_SFT              16
> +#define CLK_AUD_INTBUS_SEL_MASK             0x3
> +#define CLK_AUD_INTBUS_SEL_MASK_SFT         (0x3 << 16)

#define CLK_AUD_INTBUS_SEL_MASK_SFT		GENMASK(23, 22)

> +
> +/* CLK_AUDDIV_0 */
> +#define APLL12_DIV0_PDN_SFT                0
> +#define APLL12_DIV0_PDN_MASK               0x1
> +#define APLL12_DIV0_PDN_MASK_SFT           (0x1 << 0)

BIT() macro please

#define APLL12_DIV0_PDN_MASK_SFT		BIT(0)

also, using tabulations instead of spaces is nicer.

> +#define APLL12_DIV1_PDN_SFT                1
> +#define APLL12_DIV1_PDN_MASK               0x1
> +#define APLL12_DIV1_PDN_MASK_SFT           (0x1 << 1)

Of course, BIT() macro again, here and everywhere else applicable.

> +#define APLL12_DIV2_PDN_SFT                2
> +#define APLL12_DIV2_PDN_MASK               0x1
> +#define APLL12_DIV2_PDN_MASK_SFT           (0x1 << 2)
> +#define APLL12_DIV4_PDN_SFT                3
> +#define APLL12_DIV4_PDN_MASK               0x1
> +#define APLL12_DIV4_PDN_MASK_SFT           (0x1 << 3)
> +#define APLL12_DIV_TDM_PDN_SFT             4
> +#define APLL12_DIV_TDM_PDN_MASK            0x1
> +#define APLL12_DIV_TDM_PDN_MASK_SFT        (0x1 << 4)
> +#define APLL_I2S0_MCK_SEL_SFT              16
> +#define APLL_I2S0_MCK_SEL_MASK             0x1
> +#define APLL_I2S0_MCK_SEL_MASK_SFT         (0x1 << 16)
> +#define APLL_I2S1_MCK_SEL_SFT              17
> +#define APLL_I2S1_MCK_SEL_MASK             0x1
> +#define APLL_I2S1_MCK_SEL_MASK_SFT         (0x1 << 17)
> +#define APLL_I2S2_MCK_SEL_SFT              18
> +#define APLL_I2S2_MCK_SEL_MASK             0x1
> +#define APLL_I2S2_MCK_SEL_MASK_SFT         (0x1 << 17)
> +#define APLL_I2S4_MCK_SEL_SFT              19
> +#define APLL_I2S4_MCK_SEL_MASK             0x1
> +#define APLL_I2S4_MCK_SEL_MASK_SFT         (0x1 << 19)
> +#define APLL_TDM_MCK_SEL_SFT               20
> +#define APLL_TDM_MCK_SEL_MASK              0x1
> +#define APLL_TDM_MCK_SEL_MASK_SFT          (0x1 << 20)
> +
> +/* CLK_AUDDIV_2 */
> +#define APLL12_CK_DIV0_SFT                 0
> +#define APLL12_CK_DIV0_MASK                0xff
> +#define APLL12_CK_DIV0_MASK_SFT            (0xff << 0)

GENMASK(7, 0)

> +#define APLL12_CK_DIV1_SFT                 8
> +#define APLL12_CK_DIV1_MASK                0xff
> +#define APLL12_CK_DIV1_MASK_SFT            (0xff << 8)

GENMASK(15, 8)

> +#define APLL12_CK_DIV2_SFT                 16
> +#define APLL12_CK_DIV2_MASK                0xff
> +#define APLL12_CK_DIV2_MASK_SFT            (0xff << 16)

Fix all the others too :))

> +#define APLL12_CK_DIV4_SFT                 24
> +#define APLL12_CK_DIV4_MASK                0xff
> +#define APLL12_CK_DIV4_MASK_SFT            (0xff << 24)
> +
> +/* CLK_AUDDIV_3 */
> +#define APLL12_CK_DIV_TDM_SFT              0
> +#define APLL12_CK_DIV_TDM_MASK             0xff
> +#define APLL12_CK_DIV_TDM_MASK_SFT         (0xff << 0)
> +
> +/* AUD_TOP_CFG */
> +#define AUD_TOP_CFG_SFT                    0
> +#define AUD_TOP_CFG_MASK                   0xffffffff
> +#define AUD_TOP_CFG_MASK_SFT               (0xffffffff << 0)

This is GENMASK(31, 0) for both MASK and MASK_SFT

> +
> +/* AUD_TOP_MON */
> +#define AUD_TOP_MON_SFT                    0
> +#define AUD_TOP_MON_MASK                   0xffffffff
> +#define AUD_TOP_MON_MASK_SFT               (0xffffffff << 0)

same here



Regards,
Angelo
AngeloGioacchino Del Regno Feb. 18, 2022, 2:54 p.m. UTC | #5
Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> This patch add gpio control for all audio interface separately.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>   sound/soc/mediatek/mt8186/mt8186-afe-gpio.c | 210 ++++++++++++++++++++
>   sound/soc/mediatek/mt8186/mt8186-afe-gpio.h |  19 ++
>   2 files changed, 229 insertions(+)
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> 
> diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-gpio.c b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
> new file mode 100644
> index 000000000000..6faec5c95bf3
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// mt8186-afe-gpio.c  --  Mediatek 8186 afe gpio ctrl
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> +
> +#include <linux/gpio.h>
> +#include <linux/pinctrl/consumer.h>
> +
> +#include "mt8186-afe-common.h"
> +#include "mt8186-afe-gpio.h"
> +
> +struct pinctrl *aud_pinctrl;
> +
> +enum mt8186_afe_gpio {
> +	MT8186_AFE_GPIO_CLK_MOSI_OFF,
> +	MT8186_AFE_GPIO_CLK_MOSI_ON,
> +	MT8186_AFE_GPIO_CLK_MISO_OFF,
> +	MT8186_AFE_GPIO_CLK_MISO_ON,
> +	MT8186_AFE_GPIO_DAT_MISO_OFF,
> +	MT8186_AFE_GPIO_DAT_MISO_ON,
> +	MT8186_AFE_GPIO_DAT_MOSI_OFF,
> +	MT8186_AFE_GPIO_DAT_MOSI_ON,
> +	MT8186_AFE_GPIO_I2S0_OFF,
> +	MT8186_AFE_GPIO_I2S0_ON,
> +	MT8186_AFE_GPIO_I2S1_OFF,
> +	MT8186_AFE_GPIO_I2S1_ON,
> +	MT8186_AFE_GPIO_I2S2_OFF,
> +	MT8186_AFE_GPIO_I2S2_ON,
> +	MT8186_AFE_GPIO_I2S3_OFF,
> +	MT8186_AFE_GPIO_I2S3_ON,
> +	MT8186_AFE_GPIO_TDM_OFF,
> +	MT8186_AFE_GPIO_TDM_ON,
> +	MT8186_AFE_GPIO_PCM_OFF,
> +	MT8186_AFE_GPIO_PCM_ON,
> +	MT8186_AFE_GPIO_GPIO_NUM
> +};
> +
> +struct audio_gpio_attr {
> +	const char *name;
> +	bool gpio_prepare;
> +	struct pinctrl_state *gpioctrl;
> +};
> +
> +static struct audio_gpio_attr aud_gpios[MT8186_AFE_GPIO_GPIO_NUM] = {
> +	[MT8186_AFE_GPIO_CLK_MOSI_OFF] = {"aud_clk_mosi_off", false, NULL},
> +	[MT8186_AFE_GPIO_CLK_MOSI_ON] = {"aud_clk_mosi_on", false, NULL},
> +	[MT8186_AFE_GPIO_CLK_MISO_OFF] = {"aud_clk_miso_off", false, NULL},
> +	[MT8186_AFE_GPIO_CLK_MISO_ON] = {"aud_clk_miso_on", false, NULL},
> +	[MT8186_AFE_GPIO_DAT_MISO_OFF] = {"aud_dat_miso_off", false, NULL},
> +	[MT8186_AFE_GPIO_DAT_MISO_ON] = {"aud_dat_miso_on", false, NULL},
> +	[MT8186_AFE_GPIO_DAT_MOSI_OFF] = {"aud_dat_mosi_off", false, NULL},
> +	[MT8186_AFE_GPIO_DAT_MOSI_ON] = {"aud_dat_mosi_on", false, NULL},
> +	[MT8186_AFE_GPIO_I2S0_OFF] = {"aud_gpio_i2s0_off", false, NULL},
> +	[MT8186_AFE_GPIO_I2S0_ON] = {"aud_gpio_i2s0_on", false, NULL},
> +	[MT8186_AFE_GPIO_I2S1_OFF] = {"aud_gpio_i2s1_off", false, NULL},
> +	[MT8186_AFE_GPIO_I2S1_ON] = {"aud_gpio_i2s1_on", false, NULL},
> +	[MT8186_AFE_GPIO_I2S2_OFF] = {"aud_gpio_i2s2_off", false, NULL},
> +	[MT8186_AFE_GPIO_I2S2_ON] = {"aud_gpio_i2s2_on", false, NULL},
> +	[MT8186_AFE_GPIO_I2S3_OFF] = {"aud_gpio_i2s3_off", false, NULL},
> +	[MT8186_AFE_GPIO_I2S3_ON] = {"aud_gpio_i2s3_on", false, NULL},
> +	[MT8186_AFE_GPIO_TDM_OFF] = {"aud_gpio_tdm_off", false, NULL},
> +	[MT8186_AFE_GPIO_TDM_ON] = {"aud_gpio_tdm_on", false, NULL},
> +	[MT8186_AFE_GPIO_PCM_OFF] = {"aud_gpio_pcm_off", false, NULL},
> +	[MT8186_AFE_GPIO_PCM_ON] = {"aud_gpio_pcm_on", false, NULL},
> +};
> +
> +static DEFINE_MUTEX(gpio_request_mutex);
> +
> +int mt8186_afe_gpio_init(struct device *dev)
> +{
> +	int ret;
> +	int i = 0;
> +
> +	aud_pinctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(aud_pinctrl)) {
> +		ret = PTR_ERR(aud_pinctrl);
> +		dev_info(dev, "%s(), ret %d, cannot get aud_pinctrl!\n",
> +			 __func__, ret);

dev_err()
... and return ret.

> +		return -ENODEV;
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(aud_gpios); i++) {
> +		aud_gpios[i].gpioctrl = pinctrl_lookup_state(aud_pinctrl,
> +							     aud_gpios[i].name);
> +		if (IS_ERR(aud_gpios[i].gpioctrl)) {
> +			ret = PTR_ERR(aud_gpios[i].gpioctrl);
> +			dev_info(dev, "%s(), pinctrl_lookup_state %s fail, ret %d\n",
> +				 __func__, aud_gpios[i].name, ret);

dev_err()

P.S.: I think that this function should return ret, at this point, to avoid
       unexpected behavior.


> +		} else {
> +			aud_gpios[i].gpio_prepare = true;
> +		}
> +	}
> +
> +	/* gpio status init */
> +	mt8186_afe_gpio_request(dev, false, MT8186_DAI_ADDA, 0);
> +	mt8186_afe_gpio_request(dev, false, MT8186_DAI_ADDA, 1);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(mt8186_afe_gpio_init);
> +
> +static int mt8186_afe_gpio_select(struct device *dev,
> +				  enum mt8186_afe_gpio type)
> +{
> +	int ret = 0;
> +
> +	if (type < 0 || type >= MT8186_AFE_GPIO_GPIO_NUM) {
> +		dev_info(dev, "%s(), error, invalid gpio type %d\n",
> +			 __func__, type);
> +		return -EINVAL;
> +	}
> +
> +	if (!aud_gpios[type].gpio_prepare) {
> +		dev_info(dev, "%s(), error, gpio type %d not prepared\n",
> +			 __func__, type);
> +		return -EIO;
> +	}
> +
> +	ret = pinctrl_select_state(aud_pinctrl,
> +				   aud_gpios[type].gpioctrl);
> +	if (ret)
> +		dev_info(dev, "%s(), error, can not set gpio type %d\n",
> +			 __func__, type);
> +
> +	return ret;

Please change it like so:

	if (ret) {
		dev_err(dev, "Failed to select picntrl state for type %d\n", type);
		return ret;
	}

	return 0;

> +}
> +
> +static int mt8186_afe_gpio_adda_dl(struct device *dev, bool enable)
> +{
> +	if (enable) {
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_CLK_MOSI_ON);
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_DAT_MOSI_ON);
> +	} else {
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_DAT_MOSI_OFF);
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_CLK_MOSI_OFF);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt8186_afe_gpio_adda_ul(struct device *dev, bool enable)
> +{
> +	if (enable) {
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_CLK_MISO_ON);
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_DAT_MISO_ON);
> +	} else {
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_DAT_MISO_OFF);
> +		mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_CLK_MISO_OFF);
> +	}
> +
> +	return 0;
> +}
> +
> +int mt8186_afe_gpio_request(struct device *dev, bool enable,
> +			    int dai, int uplink)
> +{

I think that it's more readable and even shorter if you do:

	enum mt8186_afe_gpio sel;

	int ret = -EINVAL;



	mutex_lock(&gpio_request_mutex);



	switch (dai) {

	case MT8186_DAI_ADDA:
		if (uplink)
			ret = mt8186_afe_gpio_adda_ul(dev, enable);
		else
			ret = mt8186_afe_gpio_adda_dl(dev, enable);
		goto unlock;
	case MT8186_DAI_I2S_0:

		sel = enable ? MT8186_AFE_GPIO_I2S0_ON : MT8186_AFE_GPIO_I2S0_OFF;

		break;

	case MT8186_DAI_I2S_1:

		sel = enable ? MT8186_AFE_GPIO_I2S1_ON : MT8186_AFE_GPIO_I2S1_OFF;

		break;



	.................... all other cases ................

	default:

		dev_err(dev, "invalid dai %d\n", dai);

		goto unlock;

	}


	ret = mt8186_afe_gpio_select(dev, sel);


unlock:

	mutex_unlock(&gpio_request_mutex);

	return ret;
}

> +	mutex_lock(&gpio_request_mutex);
> +	switch (dai) {
> +	case MT8186_DAI_ADDA:
> +		if (uplink)
> +			mt8186_afe_gpio_adda_ul(dev, enable);
> +		else
> +			mt8186_afe_gpio_adda_dl(dev, enable);
> +		break;
> +	case MT8186_DAI_I2S_0:
> +		if (enable)
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S0_ON);
> +		else
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S0_OFF);
> +		break;
> +	case MT8186_DAI_I2S_1:
> +		if (enable)
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S1_ON);
> +		else
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S1_OFF);
> +		break;
> +	case MT8186_DAI_I2S_2:
> +		if (enable)
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S2_ON);
> +		else
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S2_OFF);
> +		break;
> +	case MT8186_DAI_I2S_3:
> +		if (enable)
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S3_ON);
> +		else
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_I2S3_OFF);
> +		break;
> +	case MT8186_DAI_TDM_IN:
> +		if (enable)
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_TDM_ON);
> +		else
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_TDM_OFF);
> +		break;
> +	case MT8186_DAI_PCM:
> +		if (enable)
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_PCM_ON);
> +		else
> +			mt8186_afe_gpio_select(dev, MT8186_AFE_GPIO_PCM_OFF);
> +		break;
> +	default:
> +		mutex_unlock(&gpio_request_mutex);
> +		dev_info(dev, "%s(), invalid dai %d\n", __func__, dai);
> +		return -EINVAL;
> +	}
> +	mutex_unlock(&gpio_request_mutex);
> +	return 0;
> +}
> diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-gpio.h b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> new file mode 100644
> index 000000000000..1ddc27838eb1
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * mt6833-afe-gpio.h  --  Mediatek 6833 afe gpio ctrl definition
> + *
> + * Copyright (c) 2022 MediaTek Inc.
> + * Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> + */
> +
> +#ifndef _MT8186_AFE_GPIO_H_
> +#define _MT8186_AFE_GPIO_H_
> +
> +struct mtk_base_afe;
> +
> +int mt8186_afe_gpio_init(struct device *dev);
> +
> +int mt8186_afe_gpio_request(struct device *dev, bool enable,
> +			    int dai, int uplink);
> +
> +#endif
Jiaxin Yu (俞家鑫) March 3, 2022, 3:16 p.m. UTC | #6
On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> > This patch add audio clock control with CCF interface.
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 719
> > +++++++++++++++++++++
> >   sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 210 ++++++
> >   2 files changed, 929 insertions(+)
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > 
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> > b/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> > new file mode 100644
> > index 000000000000..14f64b935619
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> > @@ -0,0 +1,719 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// mt8186-afe-clk.c  --  Mediatek 8186 afe clock ctrl
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-clk.h"
> > +#include "mt8186-audsys-clk.h"
> > +
> > +static DEFINE_MUTEX(mutex_request_dram);
> > +
> > +static const char *aud_clks[CLK_NUM] = {
> > +	[CLK_AFE] = "aud_afe_clk",
> > +	[CLK_DAC] = "aud_dac_clk",
> > +	[CLK_DAC_PREDIS] = "aud_dac_predis_clk",
> > +	[CLK_ADC] = "aud_adc_clk",
> > +	[CLK_TML] = "aud_tml_clk",
> > +	[CLK_APLL22M] = "aud_apll22m_clk",
> > +	[CLK_APLL24M] = "aud_apll24m_clk",
> > +	[CLK_APLL1_TUNER] = "aud_apll_tuner_clk",
> > +	[CLK_APLL2_TUNER] = "aud_apll2_tuner_clk",
> > +	[CLK_TDM] = "aud_tdm_clk",
> > +	[CLK_NLE] = "aud_nle_clk",
> > +	[CLK_DAC_HIRES] = "aud_dac_hires_clk",
> > +	[CLK_ADC_HIRES] = "aud_adc_hires_clk",
> > +	[CLK_I2S1_BCLK] = "aud_i2s1_bclk",
> > +	[CLK_I2S2_BCLK] = "aud_i2s2_bclk",
> > +	[CLK_I2S3_BCLK] = "aud_i2s3_bclk",
> > +	[CLK_I2S4_BCLK] = "aud_i2s4_bclk",
> > +	[CLK_CONNSYS_I2S_ASRC] = "aud_connsys_i2s_asrc",
> > +	[CLK_GENERAL1_ASRC] = "aud_general1_asrc",
> > +	[CLK_GENERAL2_ASRC] = "aud_general2_asrc",
> > +	[CLK_ADC_HIRES_TML] = "aud_adc_hires_tml",
> > +	[CLK_ADDA6_ADC] = "aud_adda6_adc",
> > +	[CLK_ADDA6_ADC_HIRES] = "aud_adda6_adc_hires",
> > +	[CLK_3RD_DAC] = "aud_3rd_dac",
> > +	[CLK_3RD_DAC_PREDIS] = "aud_3rd_dac_predis",
> > +	[CLK_3RD_DAC_TML] = "aud_3rd_dac_tml",
> > +	[CLK_3RD_DAC_HIRES] = "aud_3rd_dac_hires",
> > +	[CLK_ETDM_IN1_BCLK] = "aud_etdm_in1_bclk",
> > +	[CLK_ETDM_OUT1_BCLK] = "aud_etdm_out1_bclk",
> > +	[CLK_INFRA_SYS_AUDIO] = "aud_infra_clk",
> > +	[CLK_INFRA_AUDIO_26M] = "mtkaif_26m_clk",
> > +	[CLK_MUX_AUDIO] = "top_mux_audio",
> > +	[CLK_MUX_AUDIOINTBUS] = "top_mux_audio_int",
> > +	[CLK_TOP_MAINPLL_D2_D4] = "top_mainpll_d2_d4",
> > +	[CLK_TOP_MUX_AUD_1] = "top_mux_aud_1",
> > +	[CLK_TOP_APLL1_CK] = "top_apll1_ck",
> > +	[CLK_TOP_MUX_AUD_2] = "top_mux_aud_2",
> > +	[CLK_TOP_APLL2_CK] = "top_apll2_ck",
> > +	[CLK_TOP_MUX_AUD_ENG1] = "top_mux_aud_eng1",
> > +	[CLK_TOP_APLL1_D8] = "top_apll1_d8",
> > +	[CLK_TOP_MUX_AUD_ENG2] = "top_mux_aud_eng2",
> > +	[CLK_TOP_APLL2_D8] = "top_apll2_d8",
> > +	[CLK_TOP_MUX_AUDIO_H] = "top_mux_audio_h",
> > +	[CLK_TOP_I2S0_M_SEL] = "top_i2s0_m_sel",
> > +	[CLK_TOP_I2S1_M_SEL] = "top_i2s1_m_sel",
> > +	[CLK_TOP_I2S2_M_SEL] = "top_i2s2_m_sel",
> > +	[CLK_TOP_I2S4_M_SEL] = "top_i2s4_m_sel",
> > +	[CLK_TOP_TDM_M_SEL] = "top_tdm_m_sel",
> > +	[CLK_TOP_APLL12_DIV0] = "top_apll12_div0",
> > +	[CLK_TOP_APLL12_DIV1] = "top_apll12_div1",
> > +	[CLK_TOP_APLL12_DIV2] = "top_apll12_div2",
> > +	[CLK_TOP_APLL12_DIV4] = "top_apll12_div4",
> > +	[CLK_TOP_APLL12_DIV_TDM] = "top_apll12_div_tdm",
> > +	[CLK_CLK26M] = "top_clk26m_clk",
> > +};
> > +
> > +int mt8186_set_audio_int_bus_parent(struct mtk_base_afe *afe,
> > +				    int clk_id)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	ret = clk_set_parent(afe_priv->clk[CLK_MUX_AUDIOINTBUS],
> > +			     afe_priv->clk[clk_id]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS],
> > +			 aud_clks[clk_id], ret);
> 
> 		dev_err(......)
> 		return ret;
> 
> > +	}
> > +
> 
> 	return 0;
> 
> > +	return ret;
> > +}
> > +
> > +static int apll1_mux_setting(struct mtk_base_afe *afe, bool
> > enable)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_1]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> > ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_1],
> > +				     afe_priv->clk[CLK_TOP_APLL1_CK]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> > +				 aud_clks[CLK_TOP_APLL1_CK], ret);
> 
> dev_err()
> 
> > +			goto EXIT;
> > +		}
> > +
> > +		/* 180.6336 / 8 = 22.5792MHz */
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG1], ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1],
> > +				     afe_priv->clk[CLK_TOP_APLL1_D8]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG1],
> > +				 aud_clks[CLK_TOP_APLL1_D8], ret);
> > +			goto EXIT;
> > +		}
> > +	} else {
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG1],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1]);
> > +
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_1],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_1]);
> > +	}
> > +EXIT:
> > +	return 0;
> 
> You're returning 0 even in error cases, this is wrong.
Thanka for your careful review. I will fix this issue in next version.

> 
> > +}
> > +
> > +static int apll2_mux_setting(struct mtk_base_afe *afe, bool
> > enable)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_2]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> > ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
> > +				     afe_priv->clk[CLK_TOP_APLL2_CK]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> > +				 aud_clks[CLK_TOP_APLL2_CK], ret);
> > +			goto EXIT;
> > +		}
> > +
> > +		/* 196.608 / 8 = 24.576MHz */
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG2], ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2],
> > +				     afe_priv->clk[CLK_TOP_APLL2_D8]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG2],
> > +				 aud_clks[CLK_TOP_APLL2_D8], ret);
> > +			goto EXIT;
> > +		}
> > +	} else {
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG2],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2]);
> > +
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_2]);
> > +	}
> > +
> > +EXIT:
> > +	return 0;
> > +}
> > +
> > +int mt8186_afe_enable_cgs(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	for (i = CLK_I2S1_BCLK; i <= CLK_ETDM_OUT1_BCLK; i++) {
> > +		ret = clk_prepare_enable(afe_priv->clk[i]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[i], ret);
> 
> dev_err()
> 
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void mt8186_afe_disable_cgs(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int i;
> > +
> > +	for (i = CLK_I2S1_BCLK; i <= CLK_ETDM_OUT1_BCLK; i++)
> > +		clk_disable_unprepare(afe_priv->clk[i]);
> > +}
> > +
> > +int mt8186_afe_enable_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_INFRA_SYS_AUDIO], ret);
> 
> dev_err()
> 
> > +		goto CLK_INFRA_SYS_AUDIO_ERR;
> 
> also, please use lower-case labels (here and everywhere else).
> 
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_INFRA_AUDIO_26M], ret);
> > +		goto CLK_INFRA_AUDIO_26M_ERR;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIO]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIO], ret);
> > +		goto CLK_MUX_AUDIO_ERR;
> > +	}
> > +	ret = clk_set_parent(afe_priv->clk[CLK_MUX_AUDIO],
> > +			     afe_priv->clk[CLK_CLK26M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIO],
> > +			 aud_clks[CLK_CLK26M], ret);
> > +		goto CLK_MUX_AUDIO_ERR;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> > +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> > +	}
> > +	ret = mt8186_set_audio_int_bus_parent(afe,
> > +					      CLK_TOP_MAINPLL_D2_D4);
> > +	if (ret)
> > +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> > +
> > +	ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUDIO_H],
> > +			     afe_priv->clk[CLK_TOP_APLL2_CK]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> > +			 __func__, aud_clks[CLK_TOP_MUX_AUDIO_H],
> > +			 aud_clks[CLK_TOP_APLL2_CK], ret);
> > +		goto CLK_MUX_AUDIO_H_PARENT_ERR;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_AFE]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_AFE], ret);
> > +		goto CLK_AFE_ERR;
> > +	}
> > +
> > +	return 0;
> > +
> > +CLK_AFE_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_AFE]);
> > +CLK_MUX_AUDIO_H_PARENT_ERR:
> > +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +CLK_MUX_AUDIO_INTBUS_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +CLK_MUX_AUDIO_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIO]);
> > +CLK_INFRA_SYS_AUDIO_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> > +CLK_INFRA_AUDIO_26M_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mt8186_afe_disable_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_AFE]);
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIO]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> > +}
> > +
> > +int mt8186_afe_suspend_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* set audio int bus to 26M */
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> 
> dev_err() - here and for the other similar instances.
> 
> > +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> > +	}
> > +	ret = mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +	if (ret)
> > +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +
> > +	return 0;
> > +
> > +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_TOP_MAINPLL_D2_D4);
> > +CLK_MUX_AUDIO_INTBUS_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	return ret;
> > +}
> > +
> > +int mt8186_afe_resume_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* set audio int bus to normal working clock */
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> > +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> > +	}
> > +	ret = mt8186_set_audio_int_bus_parent(afe,
> > +					      CLK_TOP_MAINPLL_D2_D4);
> > +	if (ret)
> > +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +
> > +	return 0;
> > +
> > +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +CLK_MUX_AUDIO_INTBUS_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	return ret;
> > +}
> > +
> > +int mt8186_apll1_enable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* setting for APLL */
> > +	apll1_mux_setting(afe, true);
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL22M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL22M], ret);
> > +		goto ERR_CLK_APLL22M;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL1_TUNER]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL1_TUNER], ret);
> > +		goto ERR_CLK_APLL1_TUNER;
> > +	}
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
> > +			   0x0000FFF7, 0x00000832);
> 
> no leading zeroes - and without them, it fits in one line.
> 
> > +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG, 0x1, 0x1);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_22M_ON_MASK_SFT,
> > +			   0x1 << AFE_22M_ON_SFT);
> 
> BIT() macro please
> 
> > +
> > +	return 0;
> > +
> > +ERR_CLK_APLL1_TUNER:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL1_TUNER]);
> > +ERR_CLK_APLL22M:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mt8186_apll1_disable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_22M_ON_MASK_SFT,
> > +			   0x0 << AFE_22M_ON_SFT);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG, 0x1, 0x0);
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL1_TUNER]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
> > +
> > +	apll1_mux_setting(afe, false);
> > +}
> > +
> > +int mt8186_apll2_enable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* setting for APLL */
> > +	apll2_mux_setting(afe, true);
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL24M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL24M], ret);
> > +		goto ERR_CLK_APLL24M;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL2_TUNER]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL2_TUNER], ret);
> > +		goto ERR_CLK_APLL2_TUNER;
> > +	}
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
> > +			   0x0000FFF7, 0x00000634);
> > +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG, 0x1, 0x1);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_24M_ON_MASK_SFT,
> > +			   0x1 << AFE_24M_ON_SFT);
> > +
> > +	return 0;
> > +
> > +ERR_CLK_APLL2_TUNER:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL2_TUNER]);
> > +ERR_CLK_APLL24M:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mt8186_apll2_disable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_24M_ON_MASK_SFT,
> > +			   0x0 << AFE_24M_ON_SFT);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG, 0x1, 0x0);
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL2_TUNER]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
> > +
> > +	apll2_mux_setting(afe, false);
> > +}
> > +
> > +int mt8186_get_apll_rate(struct mtk_base_afe *afe, int apll)
> > +{
> > +	return (apll == MT8186_APLL1) ? 180633600 : 196608000;
> > +}
> > +
> > +int mt8186_get_apll_by_rate(struct mtk_base_afe *afe, int rate)
> > +{
> > +	return ((rate % 8000) == 0) ? MT8186_APLL2 : MT8186_APLL1;
> > +}
> > +
> > +int mt8186_get_apll_by_name(struct mtk_base_afe *afe, const char
> > *name)
> > +{
> > +	if (strcmp(name, APLL1_W_NAME) == 0)
> > +		return MT8186_APLL1;
> > +	else
> > +		return MT8186_APLL2;
> > +}
> > +
> > +/* mck */
> > +struct mt8186_mck_div {
> > +	int m_sel_id;
> > +	int div_clk_id;
> > +	/* below will be deprecated */
> > +	int div_pdn_reg;
> > +	int div_pdn_mask_sft;
> > +	int div_reg;
> > +	int div_mask_sft;
> > +	int div_mask;
> > +	int div_sft;
> > +	int div_msb_reg;
> > +	int div_msb_mask_sft;
> > +	int div_msb_mask;
> > +	int div_msb_sft;
> > +	int div_apll_sel_reg;
> > +	int div_apll_sel_mask_sft;
> > +	int div_apll_sel_sft;
> > +	int div_inv_reg;
> > +	int div_inv_mask_sft;
> 
> 
> 
> *_reg fits in u16.
> *_mask_sft, *_sel_sft can be u32.
> 
> This would be nice to avoid wasting memory for variables that are
> larger than
> needed; besides, you're also using a signed type for a number that
> may not
> ever be less than zero.
> 
Yes, your suggestion is very correct. But I will remove some member fo
this strcut because they are not needed in audio driver.

> 
> > +};
> > +
> > +static const struct mt8186_mck_div mck_div[MT8186_MCK_NUM] = {
> > +	[MT8186_I2S0_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S0_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV0,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV0_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV0_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV0_MASK,
> > +		.div_sft = APLL12_CK_DIV0_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S0_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S0_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_I2S1_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S1_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV1,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV1_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV1_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV1_MASK,
> > +		.div_sft = APLL12_CK_DIV1_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S1_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S1_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_I2S2_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S2_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV2,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV2_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV2_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV2_MASK,
> > +		.div_sft = APLL12_CK_DIV2_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S2_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S2_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_I2S4_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S4_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV4,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV4_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV4_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV4_MASK,
> > +		.div_sft = APLL12_CK_DIV4_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S4_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S4_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_TDM_MCK] = {
> > +		.m_sel_id = CLK_TOP_TDM_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV_TDM,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV_TDM_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_3,
> > +		.div_mask_sft = APLL12_CK_DIV_TDM_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV_TDM_MASK,
> > +		.div_sft = APLL12_CK_DIV_TDM_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_TDM_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_TDM_MCK_SEL_SFT,
> > +	},
> > +};
> > +
> > +int mt8186_mck_enable(struct mtk_base_afe *afe, int mck_id, int
> > rate)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int apll = mt8186_get_apll_by_rate(afe, rate);
> > +	int apll_clk_id = apll == MT8186_APLL1 ?
> > +			  CLK_TOP_MUX_AUD_1 : CLK_TOP_MUX_AUD_2;
> > +	int m_sel_id = mck_div[mck_id].m_sel_id;
> > +	int div_clk_id = mck_div[mck_id].div_clk_id;
> > +	int ret;
> > +
> > +	/* select apll */
> > +	if (m_sel_id >= 0) {
> > +		ret = clk_prepare_enable(afe_priv->clk[m_sel_id]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s(), clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[m_sel_id], ret);
> 
> dev_err()
> 
> > +			return ret;
> > +		}
> > +		ret = clk_set_parent(afe_priv->clk[m_sel_id],
> > +				     afe_priv->clk[apll_clk_id]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s(), clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[m_sel_id],
> > +				aud_clks[apll_clk_id], ret);
> 
> again
> 
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* enable div, set rate */
> > +	ret = clk_prepare_enable(afe_priv->clk[div_clk_id]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s(), clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[div_clk_id], ret);
> 
> again
> 
> > +		return ret;
> > +	}
> > +	ret = clk_set_rate(afe_priv->clk[div_clk_id], rate);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s(), clk_set_rate %s, rate %d,
> > fail %d\n",
> > +			 __func__, aud_clks[div_clk_id],
> > +			 rate, ret);
> 
> again - and this fits in two lines.
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void mt8186_mck_disable(struct mtk_base_afe *afe, int mck_id)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int m_sel_id = mck_div[mck_id].m_sel_id;
> > +	int div_clk_id = mck_div[mck_id].div_clk_id;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[div_clk_id]);
> > +	if (m_sel_id >= 0)
> > +		clk_disable_unprepare(afe_priv->clk[m_sel_id]);
> > +}
> > +
> > +int mt8186_init_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	struct device_node *of_node = afe->dev->of_node;
> > +	int i = 0;
> > +
> > +	 mt8186_audsys_clk_register(afe);
> 
> Fix indentation please
> 
> > +
> > +	afe_priv->clk = devm_kcalloc(afe->dev, CLK_NUM,
> > sizeof(*afe_priv->clk),
> > +				     GFP_KERNEL);
> > +	if (!afe_priv->clk)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < CLK_NUM; i++) {
> > +		afe_priv->clk[i] = devm_clk_get(afe->dev, aud_clks[i]);
> > +		if (IS_ERR(afe_priv->clk[i])) {
> > +			dev_info(afe->dev, "%s devm_clk_get %s fail,
> > ret %ld\n",
> > +				 __func__,
> > +				 aud_clks[i], PTR_ERR(afe_priv-
> > >clk[i]));
> > +			afe_priv->clk[i] = NULL;
> > +		}
> > +	}
> > +
> > +	afe_priv->apmixedsys = syscon_regmap_lookup_by_phandle(of_node,
> > +							       "mediate
> > k,apmixedsys");
> > +	if (IS_ERR(afe_priv->apmixedsys)) {
> > +		dev_err(afe->dev, "%s() Cannot find apmixedsys
> > controller: %ld\n",
> > +			__func__, PTR_ERR(afe_priv->apmixedsys));
> > +		return PTR_ERR(afe_priv->apmixedsys);
> > +	}
> > +
> > +	afe_priv->topckgen = syscon_regmap_lookup_by_phandle(of_node,
> > +							     "mediatek,
> > topckgen");
> > +	if (IS_ERR(afe_priv->topckgen)) {
> > +		dev_err(afe->dev, "%s() Cannot find topckgen
> > controller: %ld\n",
> > +			__func__, PTR_ERR(afe_priv->topckgen));
> > +		return PTR_ERR(afe_priv->topckgen);
> > +	}
> > +
> > +	afe_priv->infracfg = syscon_regmap_lookup_by_phandle(of_node,
> > +							     "mediatek,
> > infracfg");
> > +	if (IS_ERR(afe_priv->infracfg)) {
> > +		dev_err(afe->dev, "%s() Cannot find infracfg: %ld\n",
> > +			__func__, PTR_ERR(afe_priv->infracfg));
> > +		return PTR_ERR(afe_priv->infracfg);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void mt8186_deinit_clock(struct mtk_base_afe *afe)
> > +{
> > +	mt8186_audsys_clk_unregister(afe);
> > +}
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > b/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > new file mode 100644
> > index 000000000000..3ce7a9a24d4a
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > @@ -0,0 +1,210 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * mt8186-afe-clk.h  --  Mediatek 8186 afe clock ctrl definition
> > + *
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > + */
> > +
> > +#ifndef _MT8186_AFE_CLOCK_CTRL_H_
> > +#define _MT8186_AFE_CLOCK_CTRL_H_
> > +
> > +#define PERI_BUS_DCM_CTRL 0x0074
> > +#define MODULE_SW_CG_1_STA 0x0094
> > +#define MODULE_SW_CG_2_STA 0x00ac
> > +#define CLK_CFG_7 0x0080
> > +#define CLK_CFG_8 0x0090
> > +#define CLK_CFG_11 0x00c0
> > +#define CLK_CFG_12 0x00d0
> > +#define CLK_CFG_13 0x00e0
> > +#define CLK_CFG_15 0x0100
> > +#define AP_PLL_CON3 0x0014
> > +#define APLL1_CON4 0x0328
> > +#define APLL1_TUNER_CON0 0x0040
> > +#define APLL2_CON4 0x033c
> > +#define APLL2_TUNER_CON0 0x0044
> > +
> > +#define AP_PLL_CON5 0x0014
> > +#define APLL1_CON0 0x02c0
> > +#define APLL1_CON1 0x02c4
> > +#define APLL1_CON2 0x02c8
> > +#define APLL1_CON3 0x02cc
> > +#define APLL1_PWR_CON0 0x02d0
> > +
> > +#define APLL2_CON0 0x02d4
> > +#define APLL2_CON1 0x02d8
> > +#define APLL2_CON2 0x02dc
> > +#define APLL2_CON3 0x02e0
> > +#define APLL2_PWR_CON0 0x02e4
> > +
> > +#define APMIXEDSYS_MAX_LENGTH APLL2_PWR_CON0
> > +
> > +#define CLK_CFG_6 0x0080
> > +#define CLK_AUDDIV_0 0x0320
> > +#define CLK_AUDDIV_1 0x0324
> > +#define CLK_AUDDIV_2 0x0328
> > +#define CKSYS_AUD_TOP_CFG 0x032c
> > +#define CKSYS_AUD_TOP_MON 0x0330
> > +#define CLK_AUDDIV_3 0x0334
> > +
> > +#define CLK_MAX_LENGTH CLK_AUDDIV_3
> > +
> > +/* CLK_CFG_6 */
> > +#define CLK_AUD_INTBUS_SEL_SFT              16
> > +#define CLK_AUD_INTBUS_SEL_MASK             0x3
> > +#define CLK_AUD_INTBUS_SEL_MASK_SFT         (0x3 << 16)
> 
> #define CLK_AUD_INTBUS_SEL_MASK_SFT		GENMASK(23, 22)
> 
I will remove these define because they are not used in audio driver.

> > +
> > +/* CLK_AUDDIV_0 */
> > +#define APLL12_DIV0_PDN_SFT                0
> > +#define APLL12_DIV0_PDN_MASK               0x1
> > +#define APLL12_DIV0_PDN_MASK_SFT           (0x1 << 0)
> 
> BIT() macro please
same

> 
> #define APLL12_DIV0_PDN_MASK_SFT		BIT(0)
> 
> also, using tabulations instead of spaces is nicer.
> 
> > +#define APLL12_DIV1_PDN_SFT                1
> > +#define APLL12_DIV1_PDN_MASK               0x1
> > +#define APLL12_DIV1_PDN_MASK_SFT           (0x1 << 1)
> 
> Of course, BIT() macro again, here and everywhere else applicable.
Yes, you are right.

> 
> > +#define APLL12_DIV2_PDN_SFT                2
> > +#define APLL12_DIV2_PDN_MASK               0x1
> > +#define APLL12_DIV2_PDN_MASK_SFT           (0x1 << 2)
> > +#define APLL12_DIV4_PDN_SFT                3
> > +#define APLL12_DIV4_PDN_MASK               0x1
> > +#define APLL12_DIV4_PDN_MASK_SFT           (0x1 << 3)
> > +#define APLL12_DIV_TDM_PDN_SFT             4
> > +#define APLL12_DIV_TDM_PDN_MASK            0x1
> > +#define APLL12_DIV_TDM_PDN_MASK_SFT        (0x1 << 4)
> > +#define APLL_I2S0_MCK_SEL_SFT              16
> > +#define APLL_I2S0_MCK_SEL_MASK             0x1
> > +#define APLL_I2S0_MCK_SEL_MASK_SFT         (0x1 << 16)
> > +#define APLL_I2S1_MCK_SEL_SFT              17
> > +#define APLL_I2S1_MCK_SEL_MASK             0x1
> > +#define APLL_I2S1_MCK_SEL_MASK_SFT         (0x1 << 17)
> > +#define APLL_I2S2_MCK_SEL_SFT              18
> > +#define APLL_I2S2_MCK_SEL_MASK             0x1
> > +#define APLL_I2S2_MCK_SEL_MASK_SFT         (0x1 << 17)
> > +#define APLL_I2S4_MCK_SEL_SFT              19
> > +#define APLL_I2S4_MCK_SEL_MASK             0x1
> > +#define APLL_I2S4_MCK_SEL_MASK_SFT         (0x1 << 19)
> > +#define APLL_TDM_MCK_SEL_SFT               20
> > +#define APLL_TDM_MCK_SEL_MASK              0x1
> > +#define APLL_TDM_MCK_SEL_MASK_SFT          (0x1 << 20)
> > +
> > +/* CLK_AUDDIV_2 */
> > +#define APLL12_CK_DIV0_SFT                 0
> > +#define APLL12_CK_DIV0_MASK                0xff
> > +#define APLL12_CK_DIV0_MASK_SFT            (0xff << 0)
> 
> GENMASK(7, 0)
> 
> > +#define APLL12_CK_DIV1_SFT                 8
> > +#define APLL12_CK_DIV1_MASK                0xff
> > +#define APLL12_CK_DIV1_MASK_SFT            (0xff << 8)
> 
> GENMASK(15, 8)
> 
> > +#define APLL12_CK_DIV2_SFT                 16
> > +#define APLL12_CK_DIV2_MASK                0xff
> > +#define APLL12_CK_DIV2_MASK_SFT            (0xff << 16)
> 
> Fix all the others too :))
> 
> > +#define APLL12_CK_DIV4_SFT                 24
> > +#define APLL12_CK_DIV4_MASK                0xff
> > +#define APLL12_CK_DIV4_MASK_SFT            (0xff << 24)
> > +
> > +/* CLK_AUDDIV_3 */
> > +#define APLL12_CK_DIV_TDM_SFT              0
> > +#define APLL12_CK_DIV_TDM_MASK             0xff
> > +#define APLL12_CK_DIV_TDM_MASK_SFT         (0xff << 0)
> > +
> > +/* AUD_TOP_CFG */
> > +#define AUD_TOP_CFG_SFT                    0
> > +#define AUD_TOP_CFG_MASK                   0xffffffff
> > +#define AUD_TOP_CFG_MASK_SFT               (0xffffffff << 0)
> 
> This is GENMASK(31, 0) for both MASK and MASK_SFT
> 
> > +
> > +/* AUD_TOP_MON */
> > +#define AUD_TOP_MON_SFT                    0
> > +#define AUD_TOP_MON_MASK                   0xffffffff
> > +#define AUD_TOP_MON_MASK_SFT               (0xffffffff << 0)
> 
> same here
> 
> 
> 
> Regards,
> Angelo
Jiaxin Yu (俞家鑫) March 3, 2022, 3:17 p.m. UTC | #7
On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> > This patch add audio clock control with CCF interface.
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt8186/mt8186-afe-clk.c | 719
> > +++++++++++++++++++++
> >   sound/soc/mediatek/mt8186/mt8186-afe-clk.h | 210 ++++++
> >   2 files changed, 929 insertions(+)
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > 
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> > b/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> > new file mode 100644
> > index 000000000000..14f64b935619
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-clk.c
> > @@ -0,0 +1,719 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// mt8186-afe-clk.c  --  Mediatek 8186 afe clock ctrl
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > +
> > +#include <linux/clk.h>
> > +#include <linux/regmap.h>
> > +#include <linux/mfd/syscon.h>
> > +
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-clk.h"
> > +#include "mt8186-audsys-clk.h"
> > +
> > +static DEFINE_MUTEX(mutex_request_dram);
> > +
> > +static const char *aud_clks[CLK_NUM] = {
> > +	[CLK_AFE] = "aud_afe_clk",
> > +	[CLK_DAC] = "aud_dac_clk",
> > +	[CLK_DAC_PREDIS] = "aud_dac_predis_clk",
> > +	[CLK_ADC] = "aud_adc_clk",
> > +	[CLK_TML] = "aud_tml_clk",
> > +	[CLK_APLL22M] = "aud_apll22m_clk",
> > +	[CLK_APLL24M] = "aud_apll24m_clk",
> > +	[CLK_APLL1_TUNER] = "aud_apll_tuner_clk",
> > +	[CLK_APLL2_TUNER] = "aud_apll2_tuner_clk",
> > +	[CLK_TDM] = "aud_tdm_clk",
> > +	[CLK_NLE] = "aud_nle_clk",
> > +	[CLK_DAC_HIRES] = "aud_dac_hires_clk",
> > +	[CLK_ADC_HIRES] = "aud_adc_hires_clk",
> > +	[CLK_I2S1_BCLK] = "aud_i2s1_bclk",
> > +	[CLK_I2S2_BCLK] = "aud_i2s2_bclk",
> > +	[CLK_I2S3_BCLK] = "aud_i2s3_bclk",
> > +	[CLK_I2S4_BCLK] = "aud_i2s4_bclk",
> > +	[CLK_CONNSYS_I2S_ASRC] = "aud_connsys_i2s_asrc",
> > +	[CLK_GENERAL1_ASRC] = "aud_general1_asrc",
> > +	[CLK_GENERAL2_ASRC] = "aud_general2_asrc",
> > +	[CLK_ADC_HIRES_TML] = "aud_adc_hires_tml",
> > +	[CLK_ADDA6_ADC] = "aud_adda6_adc",
> > +	[CLK_ADDA6_ADC_HIRES] = "aud_adda6_adc_hires",
> > +	[CLK_3RD_DAC] = "aud_3rd_dac",
> > +	[CLK_3RD_DAC_PREDIS] = "aud_3rd_dac_predis",
> > +	[CLK_3RD_DAC_TML] = "aud_3rd_dac_tml",
> > +	[CLK_3RD_DAC_HIRES] = "aud_3rd_dac_hires",
> > +	[CLK_ETDM_IN1_BCLK] = "aud_etdm_in1_bclk",
> > +	[CLK_ETDM_OUT1_BCLK] = "aud_etdm_out1_bclk",
> > +	[CLK_INFRA_SYS_AUDIO] = "aud_infra_clk",
> > +	[CLK_INFRA_AUDIO_26M] = "mtkaif_26m_clk",
> > +	[CLK_MUX_AUDIO] = "top_mux_audio",
> > +	[CLK_MUX_AUDIOINTBUS] = "top_mux_audio_int",
> > +	[CLK_TOP_MAINPLL_D2_D4] = "top_mainpll_d2_d4",
> > +	[CLK_TOP_MUX_AUD_1] = "top_mux_aud_1",
> > +	[CLK_TOP_APLL1_CK] = "top_apll1_ck",
> > +	[CLK_TOP_MUX_AUD_2] = "top_mux_aud_2",
> > +	[CLK_TOP_APLL2_CK] = "top_apll2_ck",
> > +	[CLK_TOP_MUX_AUD_ENG1] = "top_mux_aud_eng1",
> > +	[CLK_TOP_APLL1_D8] = "top_apll1_d8",
> > +	[CLK_TOP_MUX_AUD_ENG2] = "top_mux_aud_eng2",
> > +	[CLK_TOP_APLL2_D8] = "top_apll2_d8",
> > +	[CLK_TOP_MUX_AUDIO_H] = "top_mux_audio_h",
> > +	[CLK_TOP_I2S0_M_SEL] = "top_i2s0_m_sel",
> > +	[CLK_TOP_I2S1_M_SEL] = "top_i2s1_m_sel",
> > +	[CLK_TOP_I2S2_M_SEL] = "top_i2s2_m_sel",
> > +	[CLK_TOP_I2S4_M_SEL] = "top_i2s4_m_sel",
> > +	[CLK_TOP_TDM_M_SEL] = "top_tdm_m_sel",
> > +	[CLK_TOP_APLL12_DIV0] = "top_apll12_div0",
> > +	[CLK_TOP_APLL12_DIV1] = "top_apll12_div1",
> > +	[CLK_TOP_APLL12_DIV2] = "top_apll12_div2",
> > +	[CLK_TOP_APLL12_DIV4] = "top_apll12_div4",
> > +	[CLK_TOP_APLL12_DIV_TDM] = "top_apll12_div_tdm",
> > +	[CLK_CLK26M] = "top_clk26m_clk",
> > +};
> > +
> > +int mt8186_set_audio_int_bus_parent(struct mtk_base_afe *afe,
> > +				    int clk_id)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	ret = clk_set_parent(afe_priv->clk[CLK_MUX_AUDIOINTBUS],
> > +			     afe_priv->clk[clk_id]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS],
> > +			 aud_clks[clk_id], ret);
> 
> 		dev_err(......)
> 		return ret;
> 
> > +	}
> > +
> 
> 	return 0;
> 
> > +	return ret;
> > +}
> > +
> > +static int apll1_mux_setting(struct mtk_base_afe *afe, bool
> > enable)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_1]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> > ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_1],
> > +				     afe_priv->clk[CLK_TOP_APLL1_CK]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> > +				 aud_clks[CLK_TOP_APLL1_CK], ret);
> 
> dev_err()
> 
> > +			goto EXIT;
> > +		}
> > +
> > +		/* 180.6336 / 8 = 22.5792MHz */
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG1], ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1],
> > +				     afe_priv->clk[CLK_TOP_APLL1_D8]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG1],
> > +				 aud_clks[CLK_TOP_APLL1_D8], ret);
> > +			goto EXIT;
> > +		}
> > +	} else {
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG1],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG1]);
> > +
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_1],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_1],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_1]);
> > +	}
> > +EXIT:
> > +	return 0;
> 
> You're returning 0 even in error cases, this is wrong.
> 
> > +}
> > +
> > +static int apll2_mux_setting(struct mtk_base_afe *afe, bool
> > enable)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +
> > +	if (enable) {
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_2]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> > ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
> > +				     afe_priv->clk[CLK_TOP_APLL2_CK]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> > +				 aud_clks[CLK_TOP_APLL2_CK], ret);
> > +			goto EXIT;
> > +		}
> > +
> > +		/* 196.608 / 8 = 24.576MHz */
> > +		ret = clk_prepare_enable(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG2], ret);
> > +			goto EXIT;
> > +		}
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2],
> > +				     afe_priv->clk[CLK_TOP_APLL2_D8]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG2],
> > +				 aud_clks[CLK_TOP_APLL2_D8], ret);
> > +			goto EXIT;
> > +		}
> > +	} else {
> > +		ret = clk_set_parent(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__,
> > aud_clks[CLK_TOP_MUX_AUD_ENG2],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_ENG2]);
> > +
> > +		ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
> > +				     afe_priv->clk[CLK_CLK26M]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[CLK_TOP_MUX_AUD_2],
> > +				 aud_clks[CLK_CLK26M], ret);
> > +			goto EXIT;
> > +		}
> > +		clk_disable_unprepare(afe_priv-
> > >clk[CLK_TOP_MUX_AUD_2]);
> > +	}
> > +
> > +EXIT:
> > +	return 0;
> > +}
> > +
> > +int mt8186_afe_enable_cgs(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	for (i = CLK_I2S1_BCLK; i <= CLK_ETDM_OUT1_BCLK; i++) {
> > +		ret = clk_prepare_enable(afe_priv->clk[i]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[i], ret);
> 
> dev_err()
> 
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void mt8186_afe_disable_cgs(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int i;
> > +
> > +	for (i = CLK_I2S1_BCLK; i <= CLK_ETDM_OUT1_BCLK; i++)
> > +		clk_disable_unprepare(afe_priv->clk[i]);
> > +}
> > +
> > +int mt8186_afe_enable_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret = 0;
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_INFRA_SYS_AUDIO], ret);
> 
> dev_err()
> 
> > +		goto CLK_INFRA_SYS_AUDIO_ERR;
> 
> also, please use lower-case labels (here and everywhere else).
> 
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_INFRA_AUDIO_26M], ret);
> > +		goto CLK_INFRA_AUDIO_26M_ERR;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIO]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIO], ret);
> > +		goto CLK_MUX_AUDIO_ERR;
> > +	}
> > +	ret = clk_set_parent(afe_priv->clk[CLK_MUX_AUDIO],
> > +			     afe_priv->clk[CLK_CLK26M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIO],
> > +			 aud_clks[CLK_CLK26M], ret);
> > +		goto CLK_MUX_AUDIO_ERR;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> > +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> > +	}
> > +	ret = mt8186_set_audio_int_bus_parent(afe,
> > +					      CLK_TOP_MAINPLL_D2_D4);
> > +	if (ret)
> > +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> > +
> > +	ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUDIO_H],
> > +			     afe_priv->clk[CLK_TOP_APLL2_CK]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_set_parent %s-%s fail %d\n",
> > +			 __func__, aud_clks[CLK_TOP_MUX_AUDIO_H],
> > +			 aud_clks[CLK_TOP_APLL2_CK], ret);
> > +		goto CLK_MUX_AUDIO_H_PARENT_ERR;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_AFE]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_AFE], ret);
> > +		goto CLK_AFE_ERR;
> > +	}
> > +
> > +	return 0;
> > +
> > +CLK_AFE_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_AFE]);
> > +CLK_MUX_AUDIO_H_PARENT_ERR:
> > +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +CLK_MUX_AUDIO_INTBUS_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +CLK_MUX_AUDIO_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIO]);
> > +CLK_INFRA_SYS_AUDIO_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> > +CLK_INFRA_AUDIO_26M_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mt8186_afe_disable_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_AFE]);
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIO]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_AUDIO_26M]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_INFRA_SYS_AUDIO]);
> > +}
> > +
> > +int mt8186_afe_suspend_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* set audio int bus to 26M */
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> 
> dev_err() - here and for the other similar instances.
> 
> > +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> > +	}
> > +	ret = mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +	if (ret)
> > +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +
> > +	return 0;
> > +
> > +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_TOP_MAINPLL_D2_D4);
> > +CLK_MUX_AUDIO_INTBUS_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	return ret;
> > +}
> > +
> > +int mt8186_afe_resume_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* set audio int bus to normal working clock */
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_MUX_AUDIOINTBUS], ret);
> > +		goto CLK_MUX_AUDIO_INTBUS_ERR;
> > +	}
> > +	ret = mt8186_set_audio_int_bus_parent(afe,
> > +					      CLK_TOP_MAINPLL_D2_D4);
> > +	if (ret)
> > +		goto CLK_MUX_AUDIO_INTBUS_PARENT_ERR;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +
> > +	return 0;
> > +
> > +CLK_MUX_AUDIO_INTBUS_PARENT_ERR:
> > +	mt8186_set_audio_int_bus_parent(afe, CLK_CLK26M);
> > +CLK_MUX_AUDIO_INTBUS_ERR:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_MUX_AUDIOINTBUS]);
> > +	return ret;
> > +}
> > +
> > +int mt8186_apll1_enable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* setting for APLL */
> > +	apll1_mux_setting(afe, true);
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL22M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL22M], ret);
> > +		goto ERR_CLK_APLL22M;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL1_TUNER]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL1_TUNER], ret);
> > +		goto ERR_CLK_APLL1_TUNER;
> > +	}
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG,
> > +			   0x0000FFF7, 0x00000832);
> 
> no leading zeroes - and without them, it fits in one line.
> 
> > +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG, 0x1, 0x1);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_22M_ON_MASK_SFT,
> > +			   0x1 << AFE_22M_ON_SFT);
> 
> BIT() macro please
> 
> > +
> > +	return 0;
> > +
> > +ERR_CLK_APLL1_TUNER:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL1_TUNER]);
> > +ERR_CLK_APLL22M:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mt8186_apll1_disable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_22M_ON_MASK_SFT,
> > +			   0x0 << AFE_22M_ON_SFT);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL1_TUNER_CFG, 0x1, 0x0);
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL1_TUNER]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL22M]);
> > +
> > +	apll1_mux_setting(afe, false);
> > +}
> > +
> > +int mt8186_apll2_enable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int ret;
> > +
> > +	/* setting for APLL */
> > +	apll2_mux_setting(afe, true);
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL24M]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL24M], ret);
> > +		goto ERR_CLK_APLL24M;
> > +	}
> > +
> > +	ret = clk_prepare_enable(afe_priv->clk[CLK_APLL2_TUNER]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[CLK_APLL2_TUNER], ret);
> > +		goto ERR_CLK_APLL2_TUNER;
> > +	}
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG,
> > +			   0x0000FFF7, 0x00000634);
> > +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG, 0x1, 0x1);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_24M_ON_MASK_SFT,
> > +			   0x1 << AFE_24M_ON_SFT);
> > +
> > +	return 0;
> > +
> > +ERR_CLK_APLL2_TUNER:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL2_TUNER]);
> > +ERR_CLK_APLL24M:
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
> > +
> > +	return ret;
> > +}
> > +
> > +void mt8186_apll2_disable(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +
> > +	regmap_update_bits(afe->regmap, AFE_HD_ENGEN_ENABLE,
> > +			   AFE_24M_ON_MASK_SFT,
> > +			   0x0 << AFE_24M_ON_SFT);
> > +
> > +	regmap_update_bits(afe->regmap, AFE_APLL2_TUNER_CFG, 0x1, 0x0);
> > +
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL2_TUNER]);
> > +	clk_disable_unprepare(afe_priv->clk[CLK_APLL24M]);
> > +
> > +	apll2_mux_setting(afe, false);
> > +}
> > +
> > +int mt8186_get_apll_rate(struct mtk_base_afe *afe, int apll)
> > +{
> > +	return (apll == MT8186_APLL1) ? 180633600 : 196608000;
> > +}
> > +
> > +int mt8186_get_apll_by_rate(struct mtk_base_afe *afe, int rate)
> > +{
> > +	return ((rate % 8000) == 0) ? MT8186_APLL2 : MT8186_APLL1;
> > +}
> > +
> > +int mt8186_get_apll_by_name(struct mtk_base_afe *afe, const char
> > *name)
> > +{
> > +	if (strcmp(name, APLL1_W_NAME) == 0)
> > +		return MT8186_APLL1;
> > +	else
> > +		return MT8186_APLL2;
> > +}
> > +
> > +/* mck */
> > +struct mt8186_mck_div {
> > +	int m_sel_id;
> > +	int div_clk_id;
> > +	/* below will be deprecated */
> > +	int div_pdn_reg;
> > +	int div_pdn_mask_sft;
> > +	int div_reg;
> > +	int div_mask_sft;
> > +	int div_mask;
> > +	int div_sft;
> > +	int div_msb_reg;
> > +	int div_msb_mask_sft;
> > +	int div_msb_mask;
> > +	int div_msb_sft;
> > +	int div_apll_sel_reg;
> > +	int div_apll_sel_mask_sft;
> > +	int div_apll_sel_sft;
> > +	int div_inv_reg;
> > +	int div_inv_mask_sft;
> 
> 
> 
> *_reg fits in u16.
> *_mask_sft, *_sel_sft can be u32.
> 
> This would be nice to avoid wasting memory for variables that are
> larger than
> needed; besides, you're also using a signed type for a number that
> may not
> ever be less than zero.
> 
> 
> > +};
> > +
> > +static const struct mt8186_mck_div mck_div[MT8186_MCK_NUM] = {
> > +	[MT8186_I2S0_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S0_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV0,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV0_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV0_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV0_MASK,
> > +		.div_sft = APLL12_CK_DIV0_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S0_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S0_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_I2S1_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S1_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV1,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV1_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV1_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV1_MASK,
> > +		.div_sft = APLL12_CK_DIV1_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S1_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S1_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_I2S2_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S2_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV2,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV2_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV2_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV2_MASK,
> > +		.div_sft = APLL12_CK_DIV2_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S2_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S2_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_I2S4_MCK] = {
> > +		.m_sel_id = CLK_TOP_I2S4_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV4,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV4_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_2,
> > +		.div_mask_sft = APLL12_CK_DIV4_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV4_MASK,
> > +		.div_sft = APLL12_CK_DIV4_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_I2S4_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_I2S4_MCK_SEL_SFT,
> > +	},
> > +	[MT8186_TDM_MCK] = {
> > +		.m_sel_id = CLK_TOP_TDM_M_SEL,
> > +		.div_clk_id = CLK_TOP_APLL12_DIV_TDM,
> > +		.div_pdn_reg = CLK_AUDDIV_0,
> > +		.div_pdn_mask_sft = APLL12_DIV_TDM_PDN_MASK_SFT,
> > +		.div_reg = CLK_AUDDIV_3,
> > +		.div_mask_sft = APLL12_CK_DIV_TDM_MASK_SFT,
> > +		.div_mask = APLL12_CK_DIV_TDM_MASK,
> > +		.div_sft = APLL12_CK_DIV_TDM_SFT,
> > +		.div_apll_sel_reg = CLK_AUDDIV_0,
> > +		.div_apll_sel_mask_sft = APLL_TDM_MCK_SEL_MASK_SFT,
> > +		.div_apll_sel_sft = APLL_TDM_MCK_SEL_SFT,
> > +	},
> > +};
> > +
> > +int mt8186_mck_enable(struct mtk_base_afe *afe, int mck_id, int
> > rate)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int apll = mt8186_get_apll_by_rate(afe, rate);
> > +	int apll_clk_id = apll == MT8186_APLL1 ?
> > +			  CLK_TOP_MUX_AUD_1 : CLK_TOP_MUX_AUD_2;
> > +	int m_sel_id = mck_div[mck_id].m_sel_id;
> > +	int div_clk_id = mck_div[mck_id].div_clk_id;
> > +	int ret;
> > +
> > +	/* select apll */
> > +	if (m_sel_id >= 0) {
> > +		ret = clk_prepare_enable(afe_priv->clk[m_sel_id]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s(), clk_prepare_enable %s
> > fail %d\n",
> > +				 __func__, aud_clks[m_sel_id], ret);
> 
> dev_err()
> 
> > +			return ret;
> > +		}
> > +		ret = clk_set_parent(afe_priv->clk[m_sel_id],
> > +				     afe_priv->clk[apll_clk_id]);
> > +		if (ret) {
> > +			dev_info(afe->dev, "%s(), clk_set_parent %s-%s
> > fail %d\n",
> > +				 __func__, aud_clks[m_sel_id],
> > +				aud_clks[apll_clk_id], ret);
> 
> again
> 
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	/* enable div, set rate */
> > +	ret = clk_prepare_enable(afe_priv->clk[div_clk_id]);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s(), clk_prepare_enable %s fail
> > %d\n",
> > +			 __func__, aud_clks[div_clk_id], ret);
> 
> again
> 
> > +		return ret;
> > +	}
> > +	ret = clk_set_rate(afe_priv->clk[div_clk_id], rate);
> > +	if (ret) {
> > +		dev_info(afe->dev, "%s(), clk_set_rate %s, rate %d,
> > fail %d\n",
> > +			 __func__, aud_clks[div_clk_id],
> > +			 rate, ret);
> 
> again - and this fits in two lines.
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void mt8186_mck_disable(struct mtk_base_afe *afe, int mck_id)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	int m_sel_id = mck_div[mck_id].m_sel_id;
> > +	int div_clk_id = mck_div[mck_id].div_clk_id;
> > +
> > +	clk_disable_unprepare(afe_priv->clk[div_clk_id]);
> > +	if (m_sel_id >= 0)
> > +		clk_disable_unprepare(afe_priv->clk[m_sel_id]);
> > +}
> > +
> > +int mt8186_init_clock(struct mtk_base_afe *afe)
> > +{
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	struct device_node *of_node = afe->dev->of_node;
> > +	int i = 0;
> > +
> > +	 mt8186_audsys_clk_register(afe);
> 
> Fix indentation please
> 
> > +
> > +	afe_priv->clk = devm_kcalloc(afe->dev, CLK_NUM,
> > sizeof(*afe_priv->clk),
> > +				     GFP_KERNEL);
> > +	if (!afe_priv->clk)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < CLK_NUM; i++) {
> > +		afe_priv->clk[i] = devm_clk_get(afe->dev, aud_clks[i]);
> > +		if (IS_ERR(afe_priv->clk[i])) {
> > +			dev_info(afe->dev, "%s devm_clk_get %s fail,
> > ret %ld\n",
> > +				 __func__,
> > +				 aud_clks[i], PTR_ERR(afe_priv-
> > >clk[i]));
> > +			afe_priv->clk[i] = NULL;
> > +		}
> > +	}
> > +
> > +	afe_priv->apmixedsys = syscon_regmap_lookup_by_phandle(of_node,
> > +							       "mediate
> > k,apmixedsys");
> > +	if (IS_ERR(afe_priv->apmixedsys)) {
> > +		dev_err(afe->dev, "%s() Cannot find apmixedsys
> > controller: %ld\n",
> > +			__func__, PTR_ERR(afe_priv->apmixedsys));
> > +		return PTR_ERR(afe_priv->apmixedsys);
> > +	}
> > +
> > +	afe_priv->topckgen = syscon_regmap_lookup_by_phandle(of_node,
> > +							     "mediatek,
> > topckgen");
> > +	if (IS_ERR(afe_priv->topckgen)) {
> > +		dev_err(afe->dev, "%s() Cannot find topckgen
> > controller: %ld\n",
> > +			__func__, PTR_ERR(afe_priv->topckgen));
> > +		return PTR_ERR(afe_priv->topckgen);
> > +	}
> > +
> > +	afe_priv->infracfg = syscon_regmap_lookup_by_phandle(of_node,
> > +							     "mediatek,
> > infracfg");
> > +	if (IS_ERR(afe_priv->infracfg)) {
> > +		dev_err(afe->dev, "%s() Cannot find infracfg: %ld\n",
> > +			__func__, PTR_ERR(afe_priv->infracfg));
> > +		return PTR_ERR(afe_priv->infracfg);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +void mt8186_deinit_clock(struct mtk_base_afe *afe)
> > +{
> > +	mt8186_audsys_clk_unregister(afe);
> > +}
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > b/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > new file mode 100644
> > index 000000000000..3ce7a9a24d4a
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-clk.h
> > @@ -0,0 +1,210 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * mt8186-afe-clk.h  --  Mediatek 8186 afe clock ctrl definition
> > + *
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > + */
> > +
> > +#ifndef _MT8186_AFE_CLOCK_CTRL_H_
> > +#define _MT8186_AFE_CLOCK_CTRL_H_
> > +
> > +#define PERI_BUS_DCM_CTRL 0x0074
> > +#define MODULE_SW_CG_1_STA 0x0094
> > +#define MODULE_SW_CG_2_STA 0x00ac
> > +#define CLK_CFG_7 0x0080
> > +#define CLK_CFG_8 0x0090
> > +#define CLK_CFG_11 0x00c0
> > +#define CLK_CFG_12 0x00d0
> > +#define CLK_CFG_13 0x00e0
> > +#define CLK_CFG_15 0x0100
> > +#define AP_PLL_CON3 0x0014
> > +#define APLL1_CON4 0x0328
> > +#define APLL1_TUNER_CON0 0x0040
> > +#define APLL2_CON4 0x033c
> > +#define APLL2_TUNER_CON0 0x0044
> > +
> > +#define AP_PLL_CON5 0x0014
> > +#define APLL1_CON0 0x02c0
> > +#define APLL1_CON1 0x02c4
> > +#define APLL1_CON2 0x02c8
> > +#define APLL1_CON3 0x02cc
> > +#define APLL1_PWR_CON0 0x02d0
> > +
> > +#define APLL2_CON0 0x02d4
> > +#define APLL2_CON1 0x02d8
> > +#define APLL2_CON2 0x02dc
> > +#define APLL2_CON3 0x02e0
> > +#define APLL2_PWR_CON0 0x02e4
> > +
> > +#define APMIXEDSYS_MAX_LENGTH APLL2_PWR_CON0
> > +
> > +#define CLK_CFG_6 0x0080
> > +#define CLK_AUDDIV_0 0x0320
> > +#define CLK_AUDDIV_1 0x0324
> > +#define CLK_AUDDIV_2 0x0328
> > +#define CKSYS_AUD_TOP_CFG 0x032c
> > +#define CKSYS_AUD_TOP_MON 0x0330
> > +#define CLK_AUDDIV_3 0x0334
> > +
> > +#define CLK_MAX_LENGTH CLK_AUDDIV_3
> > +
> > +/* CLK_CFG_6 */
> > +#define CLK_AUD_INTBUS_SEL_SFT              16
> > +#define CLK_AUD_INTBUS_SEL_MASK             0x3
> > +#define CLK_AUD_INTBUS_SEL_MASK_SFT         (0x3 << 16)
> 
> #define CLK_AUD_INTBUS_SEL_MASK_SFT		GENMASK(23, 22)
> 
> > +
> > +/* CLK_AUDDIV_0 */
> > +#define APLL12_DIV0_PDN_SFT                0
> > +#define APLL12_DIV0_PDN_MASK               0x1
> > +#define APLL12_DIV0_PDN_MASK_SFT           (0x1 << 0)
> 
> BIT() macro please
> 
> #define APLL12_DIV0_PDN_MASK_SFT		BIT(0)
> 
> also, using tabulations instead of spaces is nicer.
> 
> > +#define APLL12_DIV1_PDN_SFT                1
> > +#define APLL12_DIV1_PDN_MASK               0x1
> > +#define APLL12_DIV1_PDN_MASK_SFT           (0x1 << 1)
> 
> Of course, BIT() macro again, here and everywhere else applicable.
> 
> > +#define APLL12_DIV2_PDN_SFT                2
> > +#define APLL12_DIV2_PDN_MASK               0x1
> > +#define APLL12_DIV2_PDN_MASK_SFT           (0x1 << 2)
> > +#define APLL12_DIV4_PDN_SFT                3
> > +#define APLL12_DIV4_PDN_MASK               0x1
> > +#define APLL12_DIV4_PDN_MASK_SFT           (0x1 << 3)
> > +#define APLL12_DIV_TDM_PDN_SFT             4
> > +#define APLL12_DIV_TDM_PDN_MASK            0x1
> > +#define APLL12_DIV_TDM_PDN_MASK_SFT        (0x1 << 4)
> > +#define APLL_I2S0_MCK_SEL_SFT              16
> > +#define APLL_I2S0_MCK_SEL_MASK             0x1
> > +#define APLL_I2S0_MCK_SEL_MASK_SFT         (0x1 << 16)
> > +#define APLL_I2S1_MCK_SEL_SFT              17
> > +#define APLL_I2S1_MCK_SEL_MASK             0x1
> > +#define APLL_I2S1_MCK_SEL_MASK_SFT         (0x1 << 17)
> > +#define APLL_I2S2_MCK_SEL_SFT              18
> > +#define APLL_I2S2_MCK_SEL_MASK             0x1
> > +#define APLL_I2S2_MCK_SEL_MASK_SFT         (0x1 << 17)
> > +#define APLL_I2S4_MCK_SEL_SFT              19
> > +#define APLL_I2S4_MCK_SEL_MASK             0x1
> > +#define APLL_I2S4_MCK_SEL_MASK_SFT         (0x1 << 19)
> > +#define APLL_TDM_MCK_SEL_SFT               20
> > +#define APLL_TDM_MCK_SEL_MASK              0x1
> > +#define APLL_TDM_MCK_SEL_MASK_SFT          (0x1 << 20)
> > +
> > +/* CLK_AUDDIV_2 */
> > +#define APLL12_CK_DIV0_SFT                 0
> > +#define APLL12_CK_DIV0_MASK                0xff
> > +#define APLL12_CK_DIV0_MASK_SFT            (0xff << 0)
> 
> GENMASK(7, 0)
> 
> > +#define APLL12_CK_DIV1_SFT                 8
> > +#define APLL12_CK_DIV1_MASK                0xff
> > +#define APLL12_CK_DIV1_MASK_SFT            (0xff << 8)
> 
> GENMASK(15, 8)
> 
> > +#define APLL12_CK_DIV2_SFT                 16
> > +#define APLL12_CK_DIV2_MASK                0xff
> > +#define APLL12_CK_DIV2_MASK_SFT            (0xff << 16)
> 
> Fix all the others too :))
> 
> > +#define APLL12_CK_DIV4_SFT                 24
> > +#define APLL12_CK_DIV4_MASK                0xff
> > +#define APLL12_CK_DIV4_MASK_SFT            (0xff << 24)
> > +
> > +/* CLK_AUDDIV_3 */
> > +#define APLL12_CK_DIV_TDM_SFT              0
> > +#define APLL12_CK_DIV_TDM_MASK             0xff
> > +#define APLL12_CK_DIV_TDM_MASK_SFT         (0xff << 0)
> > +
> > +/* AUD_TOP_CFG */
> > +#define AUD_TOP_CFG_SFT                    0
> > +#define AUD_TOP_CFG_MASK                   0xffffffff
> > +#define AUD_TOP_CFG_MASK_SFT               (0xffffffff << 0)
> 
> This is GENMASK(31, 0) for both MASK and MASK_SFT
> 
> > +
> > +/* AUD_TOP_MON */
> > +#define AUD_TOP_MON_SFT                    0
> > +#define AUD_TOP_MON_MASK                   0xffffffff
> > +#define AUD_TOP_MON_MASK_SFT               (0xffffffff << 0)
> 
> same here
> 
> 
> 
> Regards,
> Angelo
Jiaxin Yu (俞家鑫) March 3, 2022, 3:30 p.m. UTC | #8
On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> > This patch add gpio control for all audio interface separately.
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt8186/mt8186-afe-gpio.c | 210
> > ++++++++++++++++++++
> >   sound/soc/mediatek/mt8186/mt8186-afe-gpio.h |  19 ++
> >   2 files changed, 229 insertions(+)
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> > 
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
> > b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
> > new file mode 100644
> > index 000000000000..6faec5c95bf3
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.c
> > @@ -0,0 +1,210 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// mt8186-afe-gpio.c  --  Mediatek 8186 afe gpio ctrl
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > +
> > +#include <linux/gpio.h>
> > +#include <linux/pinctrl/consumer.h>
> > +
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-gpio.h"
> > +
> > +struct pinctrl *aud_pinctrl;
> > +
> > +enum mt8186_afe_gpio {
> > +	MT8186_AFE_GPIO_CLK_MOSI_OFF,
> > +	MT8186_AFE_GPIO_CLK_MOSI_ON,
> > +	MT8186_AFE_GPIO_CLK_MISO_OFF,
> > +	MT8186_AFE_GPIO_CLK_MISO_ON,
> > +	MT8186_AFE_GPIO_DAT_MISO_OFF,
> > +	MT8186_AFE_GPIO_DAT_MISO_ON,
> > +	MT8186_AFE_GPIO_DAT_MOSI_OFF,
> > +	MT8186_AFE_GPIO_DAT_MOSI_ON,
> > +	MT8186_AFE_GPIO_I2S0_OFF,
> > +	MT8186_AFE_GPIO_I2S0_ON,
> > +	MT8186_AFE_GPIO_I2S1_OFF,
> > +	MT8186_AFE_GPIO_I2S1_ON,
> > +	MT8186_AFE_GPIO_I2S2_OFF,
> > +	MT8186_AFE_GPIO_I2S2_ON,
> > +	MT8186_AFE_GPIO_I2S3_OFF,
> > +	MT8186_AFE_GPIO_I2S3_ON,
> > +	MT8186_AFE_GPIO_TDM_OFF,
> > +	MT8186_AFE_GPIO_TDM_ON,
> > +	MT8186_AFE_GPIO_PCM_OFF,
> > +	MT8186_AFE_GPIO_PCM_ON,
> > +	MT8186_AFE_GPIO_GPIO_NUM
> > +};
> > +
> > +struct audio_gpio_attr {
> > +	const char *name;
> > +	bool gpio_prepare;
> > +	struct pinctrl_state *gpioctrl;
> > +};
> > +
> > +static struct audio_gpio_attr aud_gpios[MT8186_AFE_GPIO_GPIO_NUM]
> > = {
> > +	[MT8186_AFE_GPIO_CLK_MOSI_OFF] = {"aud_clk_mosi_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_CLK_MOSI_ON] = {"aud_clk_mosi_on", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_CLK_MISO_OFF] = {"aud_clk_miso_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_CLK_MISO_ON] = {"aud_clk_miso_on", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_DAT_MISO_OFF] = {"aud_dat_miso_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_DAT_MISO_ON] = {"aud_dat_miso_on", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_DAT_MOSI_OFF] = {"aud_dat_mosi_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_DAT_MOSI_ON] = {"aud_dat_mosi_on", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_I2S0_OFF] = {"aud_gpio_i2s0_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_I2S0_ON] = {"aud_gpio_i2s0_on", false, NULL},
> > +	[MT8186_AFE_GPIO_I2S1_OFF] = {"aud_gpio_i2s1_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_I2S1_ON] = {"aud_gpio_i2s1_on", false, NULL},
> > +	[MT8186_AFE_GPIO_I2S2_OFF] = {"aud_gpio_i2s2_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_I2S2_ON] = {"aud_gpio_i2s2_on", false, NULL},
> > +	[MT8186_AFE_GPIO_I2S3_OFF] = {"aud_gpio_i2s3_off", false,
> > NULL},
> > +	[MT8186_AFE_GPIO_I2S3_ON] = {"aud_gpio_i2s3_on", false, NULL},
> > +	[MT8186_AFE_GPIO_TDM_OFF] = {"aud_gpio_tdm_off", false, NULL},
> > +	[MT8186_AFE_GPIO_TDM_ON] = {"aud_gpio_tdm_on", false, NULL},
> > +	[MT8186_AFE_GPIO_PCM_OFF] = {"aud_gpio_pcm_off", false, NULL},
> > +	[MT8186_AFE_GPIO_PCM_ON] = {"aud_gpio_pcm_on", false, NULL},
> > +};
> > +
> > +static DEFINE_MUTEX(gpio_request_mutex);
> > +
> > +int mt8186_afe_gpio_init(struct device *dev)
> > +{
> > +	int ret;
> > +	int i = 0;
> > +
> > +	aud_pinctrl = devm_pinctrl_get(dev);
> > +	if (IS_ERR(aud_pinctrl)) {
> > +		ret = PTR_ERR(aud_pinctrl);
> > +		dev_info(dev, "%s(), ret %d, cannot get
> > aud_pinctrl!\n",
> > +			 __func__, ret);
> 
> dev_err()
> ... and return ret.
> 
> > +		return -ENODEV;
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(aud_gpios); i++) {
> > +		aud_gpios[i].gpioctrl =
> > pinctrl_lookup_state(aud_pinctrl,
> > +							     aud_gpios[
> > i].name);
> > +		if (IS_ERR(aud_gpios[i].gpioctrl)) {
> > +			ret = PTR_ERR(aud_gpios[i].gpioctrl);
> > +			dev_info(dev, "%s(), pinctrl_lookup_state %s
> > fail, ret %d\n",
> > +				 __func__, aud_gpios[i].name, ret);
> 
> dev_err()
> 
> P.S.: I think that this function should return ret, at this point, to
> avoid
>        unexpected behavior.

Because we maybe don't need to config all of audio interface gpio in
dtsi. for example, we may only use I2S0/I2S1 instead of I2S3/I2S4, so
there only config I2S0/I2S1's pinctrl in dtsi. So I think there only
need dev_info() as a reminder. How do you suggest for it?

> 
> 
> > +		} else {
> > +			aud_gpios[i].gpio_prepare = true;
> > +		}
> > +	}
> > +
> > +	/* gpio status init */
> > +	mt8186_afe_gpio_request(dev, false, MT8186_DAI_ADDA, 0);
> > +	mt8186_afe_gpio_request(dev, false, MT8186_DAI_ADDA, 1);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mt8186_afe_gpio_init);
> > +
> > +static int mt8186_afe_gpio_select(struct device *dev,
> > +				  enum mt8186_afe_gpio type)
> > +{
> > +	int ret = 0;
> > +
> > +	if (type < 0 || type >= MT8186_AFE_GPIO_GPIO_NUM) {
> > +		dev_info(dev, "%s(), error, invalid gpio type %d\n",
> > +			 __func__, type);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (!aud_gpios[type].gpio_prepare) {
> > +		dev_info(dev, "%s(), error, gpio type %d not
> > prepared\n",
> > +			 __func__, type);
> > +		return -EIO;
> > +	}
> > +
> > +	ret = pinctrl_select_state(aud_pinctrl,
> > +				   aud_gpios[type].gpioctrl);
> > +	if (ret)
> > +		dev_info(dev, "%s(), error, can not set gpio type
> > %d\n",
> > +			 __func__, type);
> > +
> > +	return ret;
> 
> Please change it like so:
> 
> 	if (ret) {
> 		dev_err(dev, "Failed to select picntrl state for type
> %d\n", type);
> 		return ret;
> 	}
> 
> 	return 0;
> 
> > +}
> > +
> > +static int mt8186_afe_gpio_adda_dl(struct device *dev, bool
> > enable)
> > +{
> > +	if (enable) {
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_CLK_MOSI_ON);
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_DAT_MOSI_ON);
> > +	} else {
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_DAT_MOSI_OFF);
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_CLK_MOSI_OFF);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mt8186_afe_gpio_adda_ul(struct device *dev, bool
> > enable)
> > +{
> > +	if (enable) {
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_CLK_MISO_ON);
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_DAT_MISO_ON);
> > +	} else {
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_DAT_MISO_OFF);
> > +		mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_CLK_MISO_OFF);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int mt8186_afe_gpio_request(struct device *dev, bool enable,
> > +			    int dai, int uplink)
> > +{
> 
> I think that it's more readable and even shorter if you do:
> 
> 	enum mt8186_afe_gpio sel;
> 
> 	int ret = -EINVAL;
> 
> 
> 
> 	mutex_lock(&gpio_request_mutex);
> 
> 
> 
> 	switch (dai) {
> 
> 	case MT8186_DAI_ADDA:
> 		if (uplink)
> 			ret = mt8186_afe_gpio_adda_ul(dev, enable);
> 		else
> 			ret = mt8186_afe_gpio_adda_dl(dev, enable);
> 		goto unlock;
> 	case MT8186_DAI_I2S_0:
> 
> 		sel = enable ? MT8186_AFE_GPIO_I2S0_ON :
> MT8186_AFE_GPIO_I2S0_OFF;
> 
> 		break;
> 
> 	case MT8186_DAI_I2S_1:
> 
> 		sel = enable ? MT8186_AFE_GPIO_I2S1_ON :
> MT8186_AFE_GPIO_I2S1_OFF;
> 
> 		break;
> 
> 
> 
> 	.................... all other cases ................
> 
> 	default:
> 
> 		dev_err(dev, "invalid dai %d\n", dai);
> 
> 		goto unlock;
> 
> 	}
> 
> 
> 	ret = mt8186_afe_gpio_select(dev, sel);
> 
> 
> unlock:
> 
> 	mutex_unlock(&gpio_request_mutex);
> 
> 	return ret;
> }
> 
> > +	mutex_lock(&gpio_request_mutex);
> > +	switch (dai) {
> > +	case MT8186_DAI_ADDA:
> > +		if (uplink)
> > +			mt8186_afe_gpio_adda_ul(dev, enable);
> > +		else
> > +			mt8186_afe_gpio_adda_dl(dev, enable);
> > +		break;
> > +	case MT8186_DAI_I2S_0:
> > +		if (enable)
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S0_ON);
> > +		else
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S0_OFF);
> > +		break;
> > +	case MT8186_DAI_I2S_1:
> > +		if (enable)
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S1_ON);
> > +		else
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S1_OFF);
> > +		break;
> > +	case MT8186_DAI_I2S_2:
> > +		if (enable)
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S2_ON);
> > +		else
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S2_OFF);
> > +		break;
> > +	case MT8186_DAI_I2S_3:
> > +		if (enable)
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S3_ON);
> > +		else
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_I2S3_OFF);
> > +		break;
> > +	case MT8186_DAI_TDM_IN:
> > +		if (enable)
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_TDM_ON);
> > +		else
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_TDM_OFF);
> > +		break;
> > +	case MT8186_DAI_PCM:
> > +		if (enable)
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_PCM_ON);
> > +		else
> > +			mt8186_afe_gpio_select(dev,
> > MT8186_AFE_GPIO_PCM_OFF);
> > +		break;
> > +	default:
> > +		mutex_unlock(&gpio_request_mutex);
> > +		dev_info(dev, "%s(), invalid dai %d\n", __func__, dai);
> > +		return -EINVAL;
> > +	}
> > +	mutex_unlock(&gpio_request_mutex);
> > +	return 0;
> > +}
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> > b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> > new file mode 100644
> > index 000000000000..1ddc27838eb1
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-afe-gpio.h
> > @@ -0,0 +1,19 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * mt6833-afe-gpio.h  --  Mediatek 6833 afe gpio ctrl definition
> > + *
> > + * Copyright (c) 2022 MediaTek Inc.
> > + * Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > + */
> > +
> > +#ifndef _MT8186_AFE_GPIO_H_
> > +#define _MT8186_AFE_GPIO_H_
> > +
> > +struct mtk_base_afe;
> > +
> > +int mt8186_afe_gpio_init(struct device *dev);
> > +
> > +int mt8186_afe_gpio_request(struct device *dev, bool enable,
> > +			    int dai, int uplink);
> > +
> > +#endif
> 
>
Jiaxin Yu (俞家鑫) March 5, 2022, 10:49 a.m. UTC | #9
On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> > This patch adds mt8186 adda dai driver
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt8186/mt8186-dai-adda.c | 891
> > ++++++++++++++++++++
> >   1 file changed, 891 insertions(+)
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > 
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > new file mode 100644
> > index 000000000000..6d7dd1533da0
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
> > @@ -0,0 +1,891 @@
> > +// 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>
> > +
> > +#include <linux/regmap.h>
> > +#include <linux/delay.h>
> > +#include "mt8186-afe-clk.h"
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-gpio.h"
> > +#include "mt8186-interconnection.h"
> > +
...snip...
> > 
> > +/* dai ops */
> > +static int mtk_dai_adda_hw_params(struct snd_pcm_substream
> > *substream,
> > +				  struct snd_pcm_hw_params *params,
> > +				  struct snd_soc_dai *dai)
> > +{
> > +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	unsigned int rate = params_rate(params);
> > +	int id = dai->id;
> > +	struct mtk_afe_adda_priv *adda_priv = afe_priv->dai_priv[id];
> > +
> > +	dev_info(afe->dev, "%s(), id %d, stream %d, rate %d\n",
> > +		 __func__,
> > +		 id,
> > +		 substream->stream,
> > +		 rate);
> > +
> > +	if (!adda_priv) {
> > +		dev_info(afe->dev, "%s(), adda_priv == NULL",
> > __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> > +		unsigned int dl_src2_con0 = 0;
> > +		unsigned int dl_src2_con1 = 0;
> 
> This initialization is redundant: you're never using these variables
> before initializing them later, so initializing them to zero is not
> needed here.
Yes, got it. Thank you.
> 
> > +
> > +		adda_priv->dl_rate = rate;
> > +
> > +		/* set sampling rate */
> > +		dl_src2_con0 = adda_dl_rate_transform(afe, rate) <<
> > +			       DL_2_INPUT_MODE_CTL_SFT;
> > +
> > +		/* set output mode, UP_SAMPLING_RATE_X8 */
> > +		dl_src2_con0 |= (0x3 << DL_2_OUTPUT_SEL_CTL_SFT);
> > +
> > +		/* turn off mute function */
> > +		dl_src2_con0 |= (0x01 <<
> > DL_2_MUTE_CH2_OFF_CTL_PRE_SFT);
> 
> BIT() macro, please
> 
> > +		dl_src2_con0 |= (0x01 <<
> > DL_2_MUTE_CH1_OFF_CTL_PRE_SFT);
> > +
> > +		/* set voice input data if input sample rate is 8k or
> > 16k */
> > +		if (rate == 8000 || rate == 16000)
> > +			dl_src2_con0 |= 0x01 <<
> > DL_2_VOICE_MODE_CTL_PRE_SFT;
> > +
> > +		/* SA suggest apply -0.3db to audio/speech path */
> > +		dl_src2_con1 = MTK_AFE_ADDA_DL_GAIN_NORMAL <<
> > +			       DL_2_GAIN_CTL_PRE_SFT;
> > +
> > +		/* turn on down-link gain */
> > +		dl_src2_con0 |= (0x01 << DL_2_GAIN_ON_CTL_PRE_SFT);
> > +
> > +		if (id == MT8186_DAI_ADDA) {
> > +			/* clean predistortion */
> > +			regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON0,
> > 0);
> > +			regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON1,
> > 0);
> > +
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_DL_SRC2_CON0,
> > dl_src2_con0);
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_DL_SRC2_CON1,
> > dl_src2_con1);
> > +
> > +			/* set sdm gain */
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_DL_SDM_DCCOMP_CON,
> > +					   ATTGAIN_CTL_MASK_SFT,
> > +					   AUDIO_SDM_LEVEL_NORMAL <<
> > +					   ATTGAIN_CTL_SFT);
> > +
> > +			/* Use new 2nd sdm */
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_DL_SDM_DITHER_CON,
> > +					   AFE_DL_SDM_DITHER_64TAP_EN_M
> > ASK_SFT,
> > +					   0x1 <<
> > AFE_DL_SDM_DITHER_64TAP_EN_SFT);
> 
> BIT(AFE_DL_SDM_DITHER_64TAP_EN_SFT)
> 
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_DL_SDM_AUTO_RESET_C
> > ON,
> > +					   AFE_DL_USE_NEW_2ND_SDM_MASK_
> > SFT,
> > +					   0x1 <<
> > AFE_DL_USE_NEW_2ND_SDM_SFT);
> 
> BIT(AFE_DL_USE_NEW_2ND_SDM_SFT)
> 
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_DL_SDM_DCCOMP_CON,
> > +					   USE_3RD_SDM_MASK_SFT,
> > +					   AUDIO_SDM_2ND <<
> > USE_3RD_SDM_SFT);
> > +
> > +			/* sdm auto reset */
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_DL_SDM_AUTO_RESET_CON,
> > +				     SDM_AUTO_RESET_THRESHOLD);
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_DL_SDM_AUTO_RESET_C
> > ON,
> > +					   SDM_AUTO_RESET_TEST_ON_MASK_
> > SFT,
> > +					   0x1 <<
> > SDM_AUTO_RESET_TEST_ON_SFT);
> 
> BIT(SDM_AUTO_RESET_TEST_ON_SFT)
> 
> > +		}
> > +	} else {
> > +		unsigned int voice_mode = 0;
> 
> what about...
> 		unsigned int ul_src_con0 = 0; /* default value */
> 		unsigned int voice_mode =  adda_ul_rate_transform(afe,
> rate);
Agree with you.

> > +		unsigned int ul_src_con0 = 0;	/* default value */
> > +
> > +		adda_priv->ul_rate = rate;
> > +
> > +		voice_mode = adda_ul_rate_transform(afe, rate);
> > +
> > +		ul_src_con0 |= (voice_mode << 17) & (0x7 << 17);
> > +
> > +		/* enable iir */
> > +		ul_src_con0 |= (1 << UL_IIR_ON_TMP_CTL_SFT) &
> > +			       UL_IIR_ON_TMP_CTL_MASK_SFT;
> > +		ul_src_con0 |= (UL_IIR_SW << UL_IIRMODE_CTL_SFT) &
> > +			       UL_IIRMODE_CTL_MASK_SFT;
> > +		switch (id) {
> > +		case MT8186_DAI_ADDA:
> > +		case MT8186_DAI_AP_DMIC:
> > +			/* 35Hz @ 48k */
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_IIR_COEF_02_01,
> > 0x00000000);
> 
> Please drop leading zeroes:
> 
> regmap_write(afe->regmap, AFE_ADDA_IIR_COEF_02_01, 0);
> 
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_IIR_COEF_04_03,
> > 0x00003FB8);
> 
> ... and also please write hex in lower-case:
> 
Got it.
> regmap_write(afe->regmap,
> 	     AFE_ADDA_IIR_COEF_04_03, 0x03fb8);
> 
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_IIR_COEF_06_05,
> > 0x3FB80000);
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_IIR_COEF_08_07,
> > 0x3FB80000);
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_IIR_COEF_10_09,
> > 0x0000C048);
> > +
> > +			regmap_write(afe->regmap,
> > +				     AFE_ADDA_UL_SRC_CON0,
> > ul_src_con0);
> > +
> > +			/* Using Internal ADC */
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_TOP_CON0,
> > +					   0x1 << 0,
> > +					   0x0 << 0);
> 
> Please use the BIT() macro:
> 
> regmap_update_bits(afe->regmap, AFE_ADDA_TOP_CON0, BIT(0), 0);
> 
> P.S.: 87 columns is ok

How can I judge whether it can exceed 80 lines?
> 
> > +
> > +			/* mtkaif_rxif_data_mode = 0, amic */
> > +			regmap_update_bits(afe->regmap,
> > +					   AFE_ADDA_MTKAIF_RX_CFG0,
> > +					   0x1 << 0,
> > +					   0x0 << 0);
> 
> same here.
> 
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +
> > +		/* ap dmic */
> > +		switch (id) {
> > +		case MT8186_DAI_AP_DMIC:
> > +			mtk_adda_ul_src_dmic(afe, id);
> > +			break;
> > +		default:
> > +			break;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> Regards,
> Angelo
>
Jiaxin Yu (俞家鑫) March 5, 2022, 11:07 a.m. UTC | #10
On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno wrote:
> Il 17/02/22 14:41, Jiaxin Yu ha scritto:
> > This patch adds mt8186 i2s dai driver
> > 
> > Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > ---
> >   sound/soc/mediatek/mt8186/mt8186-dai-i2s.c | 1371
> > ++++++++++++++++++++
> >   1 file changed, 1371 insertions(+)
> >   create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> > 
> > diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> > b/sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> > new file mode 100644
> > index 000000000000..d6db5f6a7315
> > --- /dev/null
> > +++ b/sound/soc/mediatek/mt8186/mt8186-dai-i2s.c
> > @@ -0,0 +1,1371 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// MediaTek ALSA SoC Audio DAI I2S Control
> > +//
> > +// Copyright (c) 2022 MediaTek Inc.
> > +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> > +
> > +#include <linux/bitops.h>
> > +#include <linux/regmap.h>
> > +#include <sound/pcm_params.h>
> > +#include "mt8186-afe-clk.h"
> > +#include "mt8186-afe-common.h"
> > +#include "mt8186-afe-gpio.h"
> > +#include "mt8186-interconnection.h"
> > +
> > 
> > +static int mtk_afe_i2s_share_connect(struct snd_soc_dapm_widget
> > *source,
> > +				     struct snd_soc_dapm_widget *sink)
> > +{
> > +	struct snd_soc_dapm_widget *w = sink;
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w-
> > >dapm);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mtk_afe_i2s_priv *i2s_priv;
> > +
> > +	i2s_priv = get_i2s_priv_by_name(afe, sink->name);
> > +
> > +	if (!i2s_priv) {
> > +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);
> 
> Is this an error? => dev_err()
> Is this expected? => dev_dbg()
> 
It should be an error here and use dev_err().
I will fix the rest of the similar log level issues.

> > +		return 0;
> > +	}
> > +
> > +	if (i2s_priv->share_i2s_id < 0)
> > +		return 0;
> > +
> > +	return i2s_priv->share_i2s_id == get_i2s_id_by_name(afe,
> > source->name);
> > +}
> > +
> > +static int mtk_afe_i2s_hd_connect(struct snd_soc_dapm_widget
> > *source,
> > +				  struct snd_soc_dapm_widget *sink)
> > +{
> > +	struct snd_soc_dapm_widget *w = sink;
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w-
> > >dapm);
> > +	struct mtk_base_afe *afe =
> > snd_soc_component_get_drvdata(cmpnt);
> > +	struct mtk_afe_i2s_priv *i2s_priv;
> > +
> > +	i2s_priv = get_i2s_priv_by_name(afe, sink->name);
> > +
> > +	if (!i2s_priv) {
> > +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);
> 
> Is this an error? => dev_err()
> Is this expected? => dev_dbg()
> 
> Please fix all of the other instances of this.
> 
Yes, I know.

> > +		return 0;
> > +	}
> > +
> > +	if (get_i2s_id_by_name(afe, sink->name) ==
> > +	    get_i2s_id_by_name(afe, source->name))
> > +		return i2s_priv->low_jitter_en;
> > +
> > +	/* check if share i2s need hd en */
> > +	if (i2s_priv->share_i2s_id < 0)
> > +		return 0;
> > +
> > +	if (i2s_priv->share_i2s_id == get_i2s_id_by_name(afe, source-
> > >name))
> > +		return i2s_priv->low_jitter_en;
> > +
> > +	return 0;
> > +}
> > +
> 
> ..snip...
> 
> > +
> > +/* dai ops */
> > +static int mtk_dai_connsys_i2s_hw_params(struct snd_pcm_substream
> > *substream,
> > +					 struct snd_pcm_hw_params
> > *params,
> > +					 struct snd_soc_dai *dai)
> > +{
> > +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> > +	unsigned int rate = params_rate(params);
> > +	unsigned int rate_reg = mt8186_rate_transform(afe->dev,
> > +						      rate, dai->id);
> > +	unsigned int i2s_con = 0;
> > +
> > +	dev_info(afe->dev, "%s(), id %d, stream %d, rate %d\n",
> > +		 __func__,
> > +		 dai->id,
> > +		 substream->stream,
> > +		 rate);
> > +
> > +	/* non-inverse, i2s mode, slave, 16bits, from connsys */
> > +	i2s_con |= 0 << INV_PAD_CTRL_SFT;
> > +	i2s_con |= I2S_FMT_I2S << I2S_FMT_SFT;
> > +	i2s_con |= 1 << I2S_SRC_SFT;
> > +	i2s_con |= get_i2s_wlen(SNDRV_PCM_FORMAT_S16_LE) <<
> > I2S_WLEN_SFT;
> > +	i2s_con |= 0 << I2SIN_PAD_SEL_SFT;
> > +	regmap_write(afe->regmap, AFE_CONNSYS_I2S_CON, i2s_con);
> > +
> > +	/* use asrc */
> > +	regmap_update_bits(afe->regmap,
> > +			   AFE_CONNSYS_I2S_CON,
> > +			   I2S_BYPSRC_MASK_SFT,
> > +			   0x0 << I2S_BYPSRC_SFT);
> 
> Zero shifted of a billion bits is still zero.
> 
Got it.

> regmap_update_bits(afe->regmap, AFE_CONNSYS_I2S_CON,
> I2S_BYPSRC_MASK_SFT, 0);
> 
> > +
> > +	/* slave mode, set i2s for asrc */
> > +	regmap_update_bits(afe->regmap,
> > +			   AFE_CONNSYS_I2S_CON,
> > +			   I2S_MODE_MASK_SFT,
> > +			   rate_reg << I2S_MODE_SFT);
> 
> 	regmap_update_bits(afe->regmap, AFE_CONNSYS_I2S_CON,
> 
> 			   I2S_MODE_MASK_SFT, rate_reg <<
> I2S_MODE_SFT);
> 
> > +
> > +	if (rate == 44100)
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3,
> > 0x001B9000);
> 
> lower case hex, please, and no leading zeros.
> 
Got it.
> > +	else if (rate == 32000)
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000);
> > +	else
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3,
> > 0x001E0000);
> > +
> > +	/* Calibration setting */
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986);
> 

snip...
> > +
> > +	if (i2s_priv)
> > +		i2s_priv->rate = rate;
> > +	else
> > +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);
> 
> I'm not sure about this print, maybe this should also be dev_dbg()
> 
It should be return error.
> > +
> > +	switch (i2s_id) {
> > +	case MT8186_DAI_I2S_0:
> > +		i2s_con = I2S_IN_PAD_IO_MUX << I2SIN_PAD_SEL_SFT;
> > +		i2s_con |= rate_reg << I2S_OUT_MODE_SFT;
> > +		i2s_con |= I2S_FMT_I2S << I2S_FMT_SFT;
> > +		i2s_con |= get_i2s_wlen(format) << I2S_WLEN_SFT;
> > +		regmap_update_bits(afe->regmap, AFE_I2S_CON,
> > +				   0xffffeffa, i2s_con);
> > +		break;
> > +	case MT8186_DAI_I2S_1:
> > +		i2s_con = I2S1_SEL_O28_O29 << I2S2_SEL_O03_O04_SFT;
> > +		i2s_con |= rate_reg << I2S2_OUT_MODE_SFT;
> > +		i2s_con |= I2S_FMT_I2S << I2S2_FMT_SFT;
> > +		i2s_con |= get_i2s_wlen(format) << I2S2_WLEN_SFT;
> > +		regmap_update_bits(afe->regmap, AFE_I2S_CON1,
> > +				   0xffffeffa, i2s_con);
> > +		break;
> > +	case MT8186_DAI_I2S_2:
> > +		i2s_con = 8 << I2S3_UPDATE_WORD_SFT;
> > +		i2s_con |= rate_reg << I2S3_OUT_MODE_SFT;
> > +		i2s_con |= I2S_FMT_I2S << I2S3_FMT_SFT;
> > +		i2s_con |= get_i2s_wlen(format) << I2S3_WLEN_SFT;
> > +		regmap_update_bits(afe->regmap, AFE_I2S_CON2,
> > +				   0xffffeffa, i2s_con);
> > +		break;
> > +	case MT8186_DAI_I2S_3:
> > +		i2s_con = rate_reg << I2S4_OUT_MODE_SFT;
> > +		i2s_con |= I2S_FMT_I2S << I2S4_FMT_SFT;
> > +		i2s_con |= get_i2s_wlen(format) << I2S4_WLEN_SFT;
> > +		regmap_update_bits(afe->regmap, AFE_I2S_CON3,
> > +				   0xffffeffa, i2s_con);
> > +		break;
> > +	default:
> > +		dev_info(afe->dev, "%s(), id %d not support\n",
> > +			 __func__, i2s_id);
> 
> dev_err()
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* set share i2s */
> > +	if (i2s_priv && i2s_priv->share_i2s_id >= 0)
> > +		ret = mtk_dai_i2s_config(afe, params, i2s_priv-
> > >share_i2s_id);
> > +
> 
> 	if (i2s_priv && i2s_priv->share_i2s_id >= 0) {
> 
> 		ret = mtk_dai_i2s_config(afe, params, i2s_priv-
> >share_i2s_id);
> 
> 		if (ret)
> 
> 			return ret;
> 
> 	}
> 
> 
> 
> 	return 0;
> 
> > +	return ret;
> > +}
> > +
> > +static int mtk_dai_i2s_hw_params(struct snd_pcm_substream
> > *substream,
> > +				 struct snd_pcm_hw_params *params,
> > +				 struct snd_soc_dai *dai)
> > +{
> > +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
> > +
> > +	return mtk_dai_i2s_config(afe, params, dai->id);
> > +}
> > +
> > +static int mtk_dai_i2s_set_sysclk(struct snd_soc_dai *dai,
> > +				  int clk_id, unsigned int freq, int
> > dir)
> > +{
> > +	struct mtk_base_afe *afe = dev_get_drvdata(dai->dev);
> > +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
> > +	struct mtk_afe_i2s_priv *i2s_priv = afe_priv->dai_priv[dai-
> > >id];
> > +	int apll;
> > +	int apll_rate;
> > +
> > +	if (!i2s_priv) {
> > +		dev_info(afe->dev, "%s(), i2s_priv == NULL", __func__);
> 
> dev_err()
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (dir != SND_SOC_CLOCK_OUT) {
> > +		dev_info(afe->dev, "%s(), dir != SND_SOC_CLOCK_OUT",
> > __func__);
> 
> again...
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	dev_info(afe->dev, "%s(), freq %d\n", __func__, freq);
> 
> dev_dbg()
> 
> > +
> > +	apll = mt8186_get_apll_by_rate(afe, freq);
> > +	apll_rate = mt8186_get_apll_rate(afe, apll);
> > +
> > +	if (freq > apll_rate) {
> > +		dev_info(afe->dev, "%s(), freq > apll rate", __func__);
> 
> dev_err() .... please fix the rest as well.
> 
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (apll_rate % freq != 0) {
> > +		dev_info(afe->dev, "%s(), APLL cannot generate freq
> > Hz", __func__);
> > +		return -EINVAL;
> > +	}
> > +
> > +	i2s_priv->mclk_rate = freq;
> > +	i2s_priv->mclk_apll = apll;
> > +
> > +	if (i2s_priv->share_i2s_id > 0) {
> > +		struct mtk_afe_i2s_priv *share_i2s_priv;
> > +
> > +		share_i2s_priv = afe_priv->dai_priv[i2s_priv-
> > >share_i2s_id];
> > +		if (!share_i2s_priv) {
> > +			dev_info(afe->dev, "%s(), share_i2s_priv ==
> > NULL", __func__);
> > +			return -EINVAL;
> > +		}
> > +
> > +		share_i2s_priv->mclk_rate = i2s_priv->mclk_rate;
> > +		share_i2s_priv->mclk_apll = i2s_priv->mclk_apll;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> Regards,
> Angelo
>
AngeloGioacchino Del Regno March 7, 2022, 9:25 a.m. UTC | #11
Il 05/03/22 11:49, Jiaxin Yu ha scritto:
> On Fri, 2022-02-18 at 15:54 +0100, AngeloGioacchino Del Regno wrote:
>> Il 17/02/22 14:41, Jiaxin Yu ha scritto:
>>> This patch adds mt8186 adda dai driver
>>>
>>> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
>>> ---
>>>    sound/soc/mediatek/mt8186/mt8186-dai-adda.c | 891
>>> ++++++++++++++++++++
>>>    1 file changed, 891 insertions(+)
>>>    create mode 100644 sound/soc/mediatek/mt8186/mt8186-dai-adda.c
>>>
>>> diff --git a/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
>>> b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
>>> new file mode 100644
>>> index 000000000000..6d7dd1533da0
>>> --- /dev/null
>>> +++ b/sound/soc/mediatek/mt8186/mt8186-dai-adda.c
>>> @@ -0,0 +1,891 @@
>>> +// 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>
>>> +
>>> +#include <linux/regmap.h>
>>> +#include <linux/delay.h>
>>> +#include "mt8186-afe-clk.h"
>>> +#include "mt8186-afe-common.h"
>>> +#include "mt8186-afe-gpio.h"
>>> +#include "mt8186-interconnection.h"
>>> +
> ...snip...
>>>
>>> +/* dai ops */
>>> +static int mtk_dai_adda_hw_params(struct snd_pcm_substream
>>> *substream,
>>> +				  struct snd_pcm_hw_params *params,
>>> +				  struct snd_soc_dai *dai)
>>> +{
>>> +	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
>>> +	struct mt8186_afe_private *afe_priv = afe->platform_priv;
>>> +	unsigned int rate = params_rate(params);
>>> +	int id = dai->id;
>>> +	struct mtk_afe_adda_priv *adda_priv = afe_priv->dai_priv[id];
>>> +
>>> +	dev_info(afe->dev, "%s(), id %d, stream %d, rate %d\n",
>>> +		 __func__,
>>> +		 id,
>>> +		 substream->stream,
>>> +		 rate);
>>> +
>>> +	if (!adda_priv) {
>>> +		dev_info(afe->dev, "%s(), adda_priv == NULL",
>>> __func__);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
>>> +		unsigned int dl_src2_con0 = 0;
>>> +		unsigned int dl_src2_con1 = 0;
>>
>> This initialization is redundant: you're never using these variables
>> before initializing them later, so initializing them to zero is not
>> needed here.
> Yes, got it. Thank you.
>>
>>> +
>>> +		adda_priv->dl_rate = rate;
>>> +
>>> +		/* set sampling rate */
>>> +		dl_src2_con0 = adda_dl_rate_transform(afe, rate) <<
>>> +			       DL_2_INPUT_MODE_CTL_SFT;
>>> +
>>> +		/* set output mode, UP_SAMPLING_RATE_X8 */
>>> +		dl_src2_con0 |= (0x3 << DL_2_OUTPUT_SEL_CTL_SFT);
>>> +
>>> +		/* turn off mute function */
>>> +		dl_src2_con0 |= (0x01 <<
>>> DL_2_MUTE_CH2_OFF_CTL_PRE_SFT);
>>
>> BIT() macro, please
>>
>>> +		dl_src2_con0 |= (0x01 <<
>>> DL_2_MUTE_CH1_OFF_CTL_PRE_SFT);
>>> +
>>> +		/* set voice input data if input sample rate is 8k or
>>> 16k */
>>> +		if (rate == 8000 || rate == 16000)
>>> +			dl_src2_con0 |= 0x01 <<
>>> DL_2_VOICE_MODE_CTL_PRE_SFT;
>>> +
>>> +		/* SA suggest apply -0.3db to audio/speech path */
>>> +		dl_src2_con1 = MTK_AFE_ADDA_DL_GAIN_NORMAL <<
>>> +			       DL_2_GAIN_CTL_PRE_SFT;
>>> +
>>> +		/* turn on down-link gain */
>>> +		dl_src2_con0 |= (0x01 << DL_2_GAIN_ON_CTL_PRE_SFT);
>>> +
>>> +		if (id == MT8186_DAI_ADDA) {
>>> +			/* clean predistortion */
>>> +			regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON0,
>>> 0);
>>> +			regmap_write(afe->regmap, AFE_ADDA_PREDIS_CON1,
>>> 0);
>>> +
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_DL_SRC2_CON0,
>>> dl_src2_con0);
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_DL_SRC2_CON1,
>>> dl_src2_con1);
>>> +
>>> +			/* set sdm gain */
>>> +			regmap_update_bits(afe->regmap,
>>> +					   AFE_ADDA_DL_SDM_DCCOMP_CON,
>>> +					   ATTGAIN_CTL_MASK_SFT,
>>> +					   AUDIO_SDM_LEVEL_NORMAL <<
>>> +					   ATTGAIN_CTL_SFT);
>>> +
>>> +			/* Use new 2nd sdm */
>>> +			regmap_update_bits(afe->regmap,
>>> +					   AFE_ADDA_DL_SDM_DITHER_CON,
>>> +					   AFE_DL_SDM_DITHER_64TAP_EN_M
>>> ASK_SFT,
>>> +					   0x1 <<
>>> AFE_DL_SDM_DITHER_64TAP_EN_SFT);
>>
>> BIT(AFE_DL_SDM_DITHER_64TAP_EN_SFT)
>>
>>> +			regmap_update_bits(afe->regmap,
>>> +					   AFE_ADDA_DL_SDM_AUTO_RESET_C
>>> ON,
>>> +					   AFE_DL_USE_NEW_2ND_SDM_MASK_
>>> SFT,
>>> +					   0x1 <<
>>> AFE_DL_USE_NEW_2ND_SDM_SFT);
>>
>> BIT(AFE_DL_USE_NEW_2ND_SDM_SFT)
>>
>>> +			regmap_update_bits(afe->regmap,
>>> +					   AFE_ADDA_DL_SDM_DCCOMP_CON,
>>> +					   USE_3RD_SDM_MASK_SFT,
>>> +					   AUDIO_SDM_2ND <<
>>> USE_3RD_SDM_SFT);
>>> +
>>> +			/* sdm auto reset */
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_DL_SDM_AUTO_RESET_CON,
>>> +				     SDM_AUTO_RESET_THRESHOLD);
>>> +			regmap_update_bits(afe->regmap,
>>> +					   AFE_ADDA_DL_SDM_AUTO_RESET_C
>>> ON,
>>> +					   SDM_AUTO_RESET_TEST_ON_MASK_
>>> SFT,
>>> +					   0x1 <<
>>> SDM_AUTO_RESET_TEST_ON_SFT);
>>
>> BIT(SDM_AUTO_RESET_TEST_ON_SFT)
>>
>>> +		}
>>> +	} else {
>>> +		unsigned int voice_mode = 0;
>>
>> what about...
>> 		unsigned int ul_src_con0 = 0; /* default value */
>> 		unsigned int voice_mode =  adda_ul_rate_transform(afe,
>> rate);
> Agree with you.
> 
>>> +		unsigned int ul_src_con0 = 0;	/* default value */
>>> +
>>> +		adda_priv->ul_rate = rate;
>>> +
>>> +		voice_mode = adda_ul_rate_transform(afe, rate);
>>> +
>>> +		ul_src_con0 |= (voice_mode << 17) & (0x7 << 17);
>>> +
>>> +		/* enable iir */
>>> +		ul_src_con0 |= (1 << UL_IIR_ON_TMP_CTL_SFT) &
>>> +			       UL_IIR_ON_TMP_CTL_MASK_SFT;
>>> +		ul_src_con0 |= (UL_IIR_SW << UL_IIRMODE_CTL_SFT) &
>>> +			       UL_IIRMODE_CTL_MASK_SFT;
>>> +		switch (id) {
>>> +		case MT8186_DAI_ADDA:
>>> +		case MT8186_DAI_AP_DMIC:
>>> +			/* 35Hz @ 48k */
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_IIR_COEF_02_01,
>>> 0x00000000);
>>
>> Please drop leading zeroes:
>>
>> regmap_write(afe->regmap, AFE_ADDA_IIR_COEF_02_01, 0);
>>
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_IIR_COEF_04_03,
>>> 0x00003FB8);
>>
>> ... and also please write hex in lower-case:
>>
> Got it.
>> regmap_write(afe->regmap,
>> 	     AFE_ADDA_IIR_COEF_04_03, 0x03fb8);
>>
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_IIR_COEF_06_05,
>>> 0x3FB80000);
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_IIR_COEF_08_07,
>>> 0x3FB80000);
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_IIR_COEF_10_09,
>>> 0x0000C048);
>>> +
>>> +			regmap_write(afe->regmap,
>>> +				     AFE_ADDA_UL_SRC_CON0,
>>> ul_src_con0);
>>> +
>>> +			/* Using Internal ADC */
>>> +			regmap_update_bits(afe->regmap,
>>> +					   AFE_ADDA_TOP_CON0,
>>> +					   0x1 << 0,
>>> +					   0x0 << 0);
>>
>> Please use the BIT() macro:
>>
>> regmap_update_bits(afe->regmap, AFE_ADDA_TOP_CON0, BIT(0), 0);
>>
>> P.S.: 87 columns is ok
> 
> How can I judge whether it can exceed 80 lines?

https://www.kernel.org/doc/html/latest/process/coding-style.html

Besides, the 80 columns rule was made for old terminals (like VT100 and such)
that supported displaying a maximum of 80 cols... in the modern era, that's
not the case anymore.
It's somewhat preferred if something can fit in 80 columns, but if it does
impact on readability, then you can go over that, up to 100 columns.

scripts/checkpatch.pl was also changed to complain about 100 columns and not
80 anymore...

my $max_line_length = 100;

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?h=v5.17-rc7#n59

Cheers,
Angelo