[PATCH] ASoC: Intel: avs: Add support for RT5663 codec

[Amadeusz Sławiński]

On 12/21/2022 9:13 PM, Alicja Michalska wrote:
> This patch adds support for RT5663 codec on KBL platform.
> Such hardware configuration can be found in Google Pixelbook (Google/Eve).
> 
> Reported-and-tested-by: CoolStar <coolstarorganization at gmail.com>
> Signed-off-by: Alicja Michalska <ahplka19 at gmail.com>
> 

I've added few comments, but overall it looks ok, some minor differences 
from internal code I wrote for this device, but that can be changed 
later if we see that it is needed. The most important parts look fine to me.

> diff --git a/sound/soc/intel/avs/board_selection.c b/sound/soc/intel/avs/board_selection.c
> index b2823c2107f7..b167a641d1d5 100644
> --- a/sound/soc/intel/avs/board_selection.c
> +++ b/sound/soc/intel/avs/board_selection.c
> @@ -159,6 +159,14 @@ static struct snd_soc_acpi_mach avs_kbl_i2s_machines[] = {
>   		},
>   		.tplg_filename = "da7219-tplg.bin",
>   	},
> +	{
> +		.id = "10EC5663",
> +		.drv_name = "avs_rt5663",
> +		.mach_params = {
> +			.i2s_link_mask = AVS_SSP(1),
> +		},
> +		.tplg_filename = "rt5663-tplg.bin",
> +	},
>   	{},
>   };
>   
> diff --git a/sound/soc/intel/avs/boards/Kconfig b/sound/soc/intel/avs/boards/Kconfig
> index e4c230efe8d7..04c090c3f9b5 100644
> --- a/sound/soc/intel/avs/boards/Kconfig
> +++ b/sound/soc/intel/avs/boards/Kconfig
> @@ -125,6 +125,16 @@ config SND_SOC_INTEL_AVS_MACH_RT5682
>   	   Say Y or m if you have such a device. This is a recommended option.
>   	   If unsure select "N".
>   
> +config SND_SOC_INTEL_AVS_MACH_RT5663
> +	tristate "rt5663 in I2S mode"
> +	depends on I2C
> +	depends on MFD_INTEL_LPSS || COMPILE_TEST
> +	select SND_SOC_RT5663_I2C
> +	help
> +	  This adds support for ASoC machine driver with RT5663 I2S audio codec.
> +	  Say Y or m if you have such a device. This is a recommended option.
> +	  If unsure select "N".
> +

Add it before SND_SOC_INTEL_AVS_MACH_RT5682.

>   config SND_SOC_INTEL_AVS_MACH_SSM4567
>   	tristate "ssm4567 I2S board"
>   	depends on I2C
> diff --git a/sound/soc/intel/avs/boards/Makefile b/sound/soc/intel/avs/boards/Makefile
> index b81343420370..3db863fc26a7 100644
> --- a/sound/soc/intel/avs/boards/Makefile
> +++ b/sound/soc/intel/avs/boards/Makefile
> @@ -13,6 +13,7 @@ snd-soc-avs-rt274-objs := rt274.o
>   snd-soc-avs-rt286-objs := rt286.o
>   snd-soc-avs-rt298-objs := rt298.o
>   snd-soc-avs-rt5682-objs := rt5682.o
> +snd-soc-avs-rt5663-objs := rt5663.o

Add it in alphabetical order.

>   snd-soc-avs-ssm4567-objs := ssm4567.o
>   
>   obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_DA7219) += snd-soc-avs-da7219.o
> @@ -28,4 +29,5 @@ obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT274) += snd-soc-avs-rt274.o
>   obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT286) += snd-soc-avs-rt286.o
>   obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT298) += snd-soc-avs-rt298.o
>   obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT5682) += snd-soc-avs-rt5682.o
> +obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_RT5663) += snd-soc-avs-rt5663.o

Same here.

>   obj-$(CONFIG_SND_SOC_INTEL_AVS_MACH_SSM4567) += snd-soc-avs-ssm4567.o
> diff --git a/sound/soc/intel/avs/boards/rt5663.c b/sound/soc/intel/avs/boards/rt5663.c
> new file mode 100644
> index 000000000000..7d8f45267d27
> --- /dev/null
> +++ b/sound/soc/intel/avs/boards/rt5663.c
> @@ -0,0 +1,249 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright(c) CoolStar. All rights reserved.
> +// Based off da7219 module

No need to mention on which module it is based, they are all based on 
each other and also Skylake driver boards ;).

> +// Copyright(c) 2021-2022 Intel Corporation. All rights reserved.
> +//
> +// Author: CoolStar <coolstarorganization.com>
> +// Author: Cezary Rojewski <cezary.rojewski at intel.com>
> +//
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <sound/jack.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <sound/soc-acpi.h>
> +#include <sound/soc-dapm.h>
> +#include <uapi/linux/input-event-codes.h>
> +#include "../../../codecs/rt5663.h"
> +
> +#define RT5663_DAI_NAME		"rt5663-aif"
> +
> +static const struct snd_kcontrol_new card_controls[] = {
> +	SOC_DAPM_PIN_SWITCH("Headphone Jack"),
> +	SOC_DAPM_PIN_SWITCH("Headset Mic"),
> +};
> +
> +static const struct snd_soc_dapm_widget card_widgets[] = {
> +	SND_SOC_DAPM_HP("Headphone Jack", NULL),
> +	SND_SOC_DAPM_MIC("Headset Mic", NULL),
> +};
> +
> +static const struct snd_soc_dapm_route card_base_routes[] = {
> +	/* HP jack connectors - unknown if we have jack detection */
> +	{"Headphone Jack", NULL, "HPOL"},
> +	{"Headphone Jack", NULL, "HPOR"},
> +
> +	{"IN1P", NULL, "Headset Mic"},
> +	{"IN1N", NULL, "Headset Mic"},
> +};
> +
> +static int avs_rt5663_codec_init(struct snd_soc_pcm_runtime *runtime)
> +{
> +	struct snd_soc_component *component = asoc_rtd_to_codec(runtime, 0)->component;
> +	struct snd_soc_card *card = runtime->card;
> +	struct snd_soc_jack *jack;
> +	struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(runtime, 0);
> +	int ret;

Reverse Christmas tree style.

> +
> +	jack = snd_soc_card_get_drvdata(card);
> +

There is a comment in Skylake machine driver:
/* use ASRC for internal clocks, as PLL rate isn't multiple of BCLK */
I think it is good to have it before calling this function.

> +	rt5663_sel_asrc_clk_src(component,

Just use codec_dai->component as argument here and component in 
declarations above can be removed then.

> +				RT5663_DA_STEREO_FILTER | RT5663_AD_STEREO_FILTER,
> +				RT5663_CLK_SEL_I2S1_ASRC);
> +				
> +	snd_soc_dai_set_sysclk(codec_dai,
> +			RT5663_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN);
> +

Just a note, that in internal code I did same as Skylake machine driver 
and put above two functions in separate _hw_params function, but putting 
it in codec_init should also be fine.

> +	/*
> +	 * Headset buttons map to the google Reference headset.
> +	 * These can be configured by userspace.
> +	 */
> +	ret = snd_soc_card_jack_new(card, "Headset Jack",
> +				    SND_JACK_HEADSET | SND_JACK_BTN_0 |
> +				    SND_JACK_BTN_1 | SND_JACK_BTN_2 |
> +				    SND_JACK_BTN_3 | SND_JACK_LINEOUT, jack);
> +	if (ret) {
> +		dev_err(card->dev, "Headset Jack creation failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_1, KEY_VOLUMEUP);
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_2, KEY_VOLUMEDOWN);
> +	snd_jack_set_key(jack->jack, SND_JACK_BTN_3, KEY_VOICECOMMAND);
> +
> +	return 0;
> +}
> +
> +static int avs_create_dai_link(struct device *dev, const char *platform_name, int ssp_port,
> +			       struct snd_soc_dai_link **dai_link)
> +{
> +	struct snd_soc_dai_link_component *platform;
> +	struct snd_soc_dai_link *dl;
> +
> +	dl = devm_kzalloc(dev, sizeof(*dl), GFP_KERNEL);
> +	platform = devm_kzalloc(dev, sizeof(*platform), GFP_KERNEL);
> +	if (!dl || !platform)
> +		return -ENOMEM;
> +
> +	platform->name = platform_name;
> +
> +	dl->name = devm_kasprintf(dev, GFP_KERNEL, "SSP%d-Codec", ssp_port);
> +	dl->cpus = devm_kzalloc(dev, sizeof(*dl->cpus), GFP_KERNEL);
> +	dl->codecs = devm_kzalloc(dev, sizeof(*dl->codecs), GFP_KERNEL);
> +	if (!dl->name || !dl->cpus || !dl->codecs)
> +		return -ENOMEM;
> +
> +	dl->cpus->dai_name = devm_kasprintf(dev, GFP_KERNEL, "SSP%d Pin", ssp_port);
> +	dl->codecs->name = devm_kasprintf(dev, GFP_KERNEL, "i2c-10EC5663:00");
> +	dl->codecs->dai_name = RT5663_DAI_NAME;

dl->codecs->dai_name = devm_kasprintf(dev, GFP_KERNEL, RT5663_DAI_NAME);

> +	if (!dl->cpus->dai_name || !dl->codecs->name || !dl->codecs->dai_name)
> +		return -ENOMEM;
> +
> +	dl->num_cpus = 1;
> +	dl->num_codecs = 1;
> +	dl->platforms = platform;
> +	dl->num_platforms = 1;
> +	dl->id = 0;
> +	dl->dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS; > +	dl->init = avs_rt5663_codec_init;
> +	dl->nonatomic = 1;
> +	dl->no_pcm = 1;
> +	dl->dpcm_capture = 1;
> +	dl->dpcm_playback = 1;
> +
> +	*dai_link = dl;
> +
> +	return 0;
> +}
> +
> +static int avs_create_dapm_routes(struct device *dev, int ssp_port,
> +				  struct snd_soc_dapm_route **routes, int *num_routes)
> +{
> +	struct snd_soc_dapm_route *dr;
> +	const int num_base = ARRAY_SIZE(card_base_routes);
> +	const int num_dr = num_base + 2;
> +	int idx;
> +
> +	dr = devm_kcalloc(dev, num_dr, sizeof(*dr), GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	memcpy(dr, card_base_routes, num_base * sizeof(*dr));
> +
> +	idx = num_base;
> +	dr[idx].sink = devm_kasprintf(dev, GFP_KERNEL, "AIF Playback");
> +	dr[idx].source = devm_kasprintf(dev, GFP_KERNEL, "ssp%d Tx", ssp_port);
> +	if (!dr[idx].sink || !dr[idx].source)
> +		return -ENOMEM;
> +
> +	idx++;
> +	dr[idx].sink = devm_kasprintf(dev, GFP_KERNEL, "ssp%d Rx", ssp_port);
> +	dr[idx].source = devm_kasprintf(dev, GFP_KERNEL, "AIF Capture");
> +	if (!dr[idx].sink || !dr[idx].source)
> +		return -ENOMEM;
> +
> +	*routes = dr;
> +	*num_routes = num_dr;
> +
> +	return 0;
> +}
> +
> +static int avs_card_set_jack(struct snd_soc_card *card, struct snd_soc_jack *jack)
> +{
> +	struct snd_soc_component *component;
> +
> +	for_each_card_components(card, component)
> +		snd_soc_component_set_jack(component, jack, NULL);
> +	return 0;
> +}
> +

This change seems to be based on a bit older code, take a look at 
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/commit/sound/soc/intel/avs/boards/da7219.c?id=833e250ef592c3c02dda400c1c44306f74241d33
this will remove the above function and change jack handling to better 
flow. As mentioned in above commit we don't need to loop to find jack, 
as we can find right component using codec name.

> +static int avs_card_remove(struct snd_soc_card *card)
> +{
> +	return avs_card_set_jack(card, NULL);
> +}
> +
> +static int avs_card_suspend_pre(struct snd_soc_card *card)
> +{
> +	return avs_card_set_jack(card, NULL);
> +}
> +
> +static int avs_card_resume_post(struct snd_soc_card *card)
> +{
> +	struct snd_soc_jack *jack = snd_soc_card_get_drvdata(card);
> +
> +	return avs_card_set_jack(card, jack);
> +}
> +
> +static int avs_rt5663_probe(struct platform_device *pdev)
> +{
> +	struct snd_soc_dapm_route *routes;
> +	struct snd_soc_dai_link *dai_link;
> +	struct snd_soc_acpi_mach *mach;
> +	struct snd_soc_card *card;
> +	struct snd_soc_jack *jack;
> +	struct device *dev = &pdev->dev;
> +	const char *pname;
> +	int num_routes, ssp_port, ret;
> +
> +	mach = dev_get_platdata(dev);
> +	pname = mach->mach_params.platform;
> +	ssp_port = __ffs(mach->mach_params.i2s_link_mask);
> +
> +	ret = avs_create_dai_link(dev, pname, ssp_port, &dai_link);
> +	if (ret) {
> +		dev_err(dev, "Failed to create dai link: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = avs_create_dapm_routes(dev, ssp_port, &routes, &num_routes);
> +	if (ret) {
> +		dev_err(dev, "Failed to create dapm routes: %d", ret);
> +		return ret;
> +	}
> +
> +	jack = devm_kzalloc(dev, sizeof(*jack), GFP_KERNEL);
> +	card = devm_kzalloc(dev, sizeof(*card), GFP_KERNEL);
> +	if (!jack || !card)
> +		return -ENOMEM;
> +
> +	card->name = "avs_rt5663";
> +	card->dev = dev;
> +	card->owner = THIS_MODULE;
> +	card->remove = avs_card_remove;
> +	card->suspend_pre = avs_card_suspend_pre;
> +	card->resume_post = avs_card_resume_post;
> +	card->dai_link = dai_link;
> +	card->num_links = 1;
> +	card->controls = card_controls;
> +	card->num_controls = ARRAY_SIZE(card_controls);
> +	card->dapm_widgets = card_widgets;
> +	card->num_dapm_widgets = ARRAY_SIZE(card_widgets);
> +	card->dapm_routes = routes;
> +	card->num_dapm_routes = num_routes;
> +	card->fully_routed = true;
> +	snd_soc_card_set_drvdata(card, jack);
> +
> +	ret = snd_soc_fixup_dai_links_platform_name(card, pname);
> +	if (ret)
> +		return ret;
> +
> +	return devm_snd_soc_register_card(dev, card);
> +}
> +
> +static struct platform_driver avs_rt5663_driver = {
> +	.probe = avs_rt5663_probe,
> +	.driver = {
> +		.name = "avs_rt5663",
> +		.pm = &snd_soc_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(avs_rt5663_driver);
> +
> +MODULE_AUTHOR("CoolStar <coolstarorganization at gmail.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:avs_rt5663");