Re: [PATCH v7 5/8] ASoC: mediatek: mt8186: add machine driver with mt6366, da7219 and max98357

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 10/06/22 10:27, Jiaxin Yu ha scritto:
> Add support for mt8186 board with mt6366, da7219 and max98357.
> 
> Signed-off-by: Jiaxin Yu <jiaxin.yu@mediatek.com>
> ---
>   sound/soc/mediatek/Kconfig                    |   16 +
>   sound/soc/mediatek/mt8186/Makefile            |    1 +
>   .../mt8186/mt8186-mt6366-da7219-max98357.c    | 1002 +++++++++++++++++
>   3 files changed, 1019 insertions(+)
>   create mode 100644 sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
> 
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index f3c3b93226e4..cc93a0d42fe1 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -164,6 +164,22 @@ config SND_SOC_MT8186
>   	  Select Y if you have such device.
>   	  If unsure select "N".
>   
> +config SND_SOC_MT8186_MT6366_DA7219_MAX98357
> +	tristate "ASoC Audio driver for MT8186 with DA7219 MAX98357A codec"
> +	depends on I2C && GPIOLIB
> +	depends on SND_SOC_MT8186 && MTK_PMIC_WRAP
> +	select SND_SOC_MT6358
> +	select SND_SOC_MAX98357A
> +	select SND_SOC_DA7219
> +	select SND_SOC_BT_SCO
> +	select SND_SOC_DMIC
> +	select SND_SOC_HDMI_CODEC
> +	help
> +	  This adds ASoC driver for Mediatek MT8186 boards
> +	  with the MT6366(MT6358) DA7219 MAX98357A codecs.
> +	  Select Y if you have such device.
> +	  If unsure select "N".
> +
>   config SND_SOC_MTK_BTCVSD
>   	tristate "ALSA BT SCO CVSD/MSBC Driver"
>   	help
> diff --git a/sound/soc/mediatek/mt8186/Makefile b/sound/soc/mediatek/mt8186/Makefile
> index bdca1a3b3ff0..e7ddbe74d9d5 100644
> --- a/sound/soc/mediatek/mt8186/Makefile
> +++ b/sound/soc/mediatek/mt8186/Makefile
> @@ -18,3 +18,4 @@ snd-soc-mt8186-afe-objs := \
>   	mt8186-mt6366-common.o
>   
>   obj-$(CONFIG_SND_SOC_MT8186) += snd-soc-mt8186-afe.o
> +obj-$(CONFIG_SND_SOC_MT8186_MT6366_DA7219_MAX98357) += mt8186-mt6366-da7219-max98357.o
> diff --git a/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c b/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
> new file mode 100644
> index 000000000000..5123754374b2
> --- /dev/null
> +++ b/sound/soc/mediatek/mt8186/mt8186-mt6366-da7219-max98357.c
> @@ -0,0 +1,1002 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// mt8186-mt6366-da7219-max98357.c
> +//	--  MT8186-MT6366-DA7219-MAX98357 ALSA SoC machine driver
> +//
> +// Copyright (c) 2022 MediaTek Inc.
> +// Author: Jiaxin Yu <jiaxin.yu@mediatek.com>
> +//
> +


...snip...

> +}
> +
> +static int mt8186_da7219_i2s_hw_params(struct snd_pcm_substream *substream,
> +				       struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +	struct snd_soc_dai *codec_dai;
> +	unsigned int rate = params_rate(params);
> +	unsigned int mclk_fs_ratio = 256;
> +	unsigned int mclk_fs = rate * mclk_fs_ratio;
> +	unsigned int freq;
> +	int ret = 0, j;

Why are you assigning 0 to ret here? You're reassigning just one line under here...

> +
> +	ret = snd_soc_dai_set_sysclk(asoc_rtd_to_cpu(rtd, 0), 0,
> +				     mclk_fs, SND_SOC_CLOCK_OUT);
> +	if (ret < 0) {
> +		dev_err(rtd->dev, "failed to set cpu dai sysclk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	for_each_rtd_codec_dais(rtd, j, codec_dai) {
> +		if (!strcmp(codec_dai->component->name, DA7219_DEV_NAME)) {
> +			ret = snd_soc_dai_set_sysclk(codec_dai,
> +						     DA7219_CLKSRC_MCLK,
> +						     mclk_fs,
> +						     SND_SOC_CLOCK_IN);
> +			if (ret < 0) {
> +				dev_err(rtd->dev, "failed to set sysclk: %d\n",
> +					ret);
> +				return ret;
> +			}
> +
> +			if ((rate % 8000) == 0)
> +				freq = DA7219_PLL_FREQ_OUT_98304;
> +			else
> +				freq = DA7219_PLL_FREQ_OUT_90316;
> +
> +			ret = snd_soc_dai_set_pll(codec_dai, 0,
> +						  DA7219_SYSCLK_PLL_SRM,
> +						  0, freq);
> +			if (ret) {
> +				dev_err(rtd->dev, "failed to start PLL: %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int mt8186_da7219_i2s_hw_free(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
> +	struct snd_soc_dai *codec_dai;
> +	int ret = 0, j;
> +
> +	for_each_rtd_codec_dais(rtd, j, codec_dai) {
> +		if (!strcmp(codec_dai->component->name, DA7219_DEV_NAME)) {
> +			ret = snd_soc_dai_set_pll(codec_dai,
> +						  0, DA7219_SYSCLK_MCLK, 0, 0);
> +			if (ret < 0) {
> +				dev_err(rtd->dev, "failed to stop PLL: %d\n",
> +					ret);

				return ret;

> +				break;
> +			}
> +		}
> +	}
> +

... and return 0 here.

> +	return ret;
> +}
> +

..snip..

> +
> +static const struct snd_soc_dapm_widget
> +mt8186_mt6366_da7219_max98357_widgets[] = {
> +	SND_SOC_DAPM_SPK("SPK Out", NULL),

In mt8183-da7219-max98357, this widget is called "Speakers": please use the
same name, so that we can (at least partially) reuse ALSA UCM2 configurations.

> +	SND_SOC_DAPM_OUTPUT("HDMI Out"),

...and some Intel boards (bxt_da7219_max98357a and others) are calling these
like "HDMI1", "HDMI2", etc.

We have only one HDMI output, yes, but if we change this to "HDMI1" we can again
increase UCM2 conf reusability.
By the way, these comments are actually valid also for the other card driver
mt6366-rt1019-rt5682s, so please model the names to commonize as much as possible.

Look at https://github.com/alsa-project/alsa-ucm-conf/tree/master/ucm2 to
understand what should be the final mixer names to achieve configuration
commonization.


> +};
> +
> +static const struct snd_soc_dapm_route
> +mt8186_mt6366_da7219_max98357_routes[] = {
> +	/* SPK */
> +	{ "SPK Out", NULL, "Speaker" },
> +	/* HDMI */
> +	{ "HDMI Out", NULL, "TX" },
> +};
> +
> +static const struct snd_kcontrol_new
> +mt8186_mt6366_da7219_max98357_controls[] = {
> +	SOC_DAPM_PIN_SWITCH("SPK Out"),
> +	SOC_DAPM_PIN_SWITCH("HDMI Out"),
> +};
> +
> +static struct snd_soc_card mt8186_mt6366_da7219_max98357_soc_card = {
> +	.name = "mt8186_mt6366_da7219_max98357",
> +	.owner = THIS_MODULE,
> +	.dai_link = mt8186_mt6366_da7219_max98357_dai_links,
> +	.num_links = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_dai_links),
> +	.controls = mt8186_mt6366_da7219_max98357_controls,
> +	.num_controls = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_controls),
> +	.dapm_widgets = mt8186_mt6366_da7219_max98357_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_widgets),
> +	.dapm_routes = mt8186_mt6366_da7219_max98357_routes,
> +	.num_dapm_routes = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_routes),
> +	.codec_conf = mt8186_mt6366_da7219_max98357_codec_conf,
> +	.num_configs = ARRAY_SIZE(mt8186_mt6366_da7219_max98357_codec_conf),
> +};
> +
> +static int mt8186_mt6366_da7219_max98357_dev_probe(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card;
> +	struct snd_soc_dai_link *dai_link;
> +	struct mt8186_mt6366_da7219_max98357_priv *priv;
> +	struct device_node *platform_node, *headset_codec, *playback_codec;
> +	int ret, i;
> +
> +	card = (struct snd_soc_card *)of_device_get_match_data(&pdev->dev);
> +	if (!card)
> +		return -EINVAL;
> +	card->dev = &pdev->dev;
> +
> +	platform_node = of_parse_phandle(pdev->dev.of_node, "mediatek,platform", 0);
> +	if (!platform_node) {
> +		ret = -EINVAL;
> +		dev_err_probe(&pdev->dev, ret, "Property 'platform' missing or invalid\n");
> +		goto err_platform_node;

You don't need this particular goto, you can just return in here.

> +	}


Regards,
Angelo