Re: [PATCH v5 1/2] ASoC: Intel: Add Cherry Trail RT5677 machine driver

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 8/27/2026 8:12 PM, Maurizio Casciano wrote:
> The Lenovo Yoga Book YB1-X91F/L uses the Cherry Trail SSP2 link with
> an RT5677 codec and a TS3A227E headset detector. Its amplifiers and
> headphone path also need board-specific GPIO sequencing.
> 
> Add a Cherry Trail RT5677 machine driver based on Yauhen Kharuzhy's
> implementation. Clone the card, DAI links and link components per device
> so probing never mutates global templates, and allocate jack storage with
> the device lifetime. Support SST and SOF parent naming, MCLK and PLL setup,
> jack buttons, microphones, and output controls.
> 
> Link: https://github.com/jekhor/yogabook-linux-kernel/commit/4e41d0ff72c1d1fd8d5c59dda247e4ca8a1da999
> Link: https://lore.kernel.org/linux-sound/20260611-asoc-yogabook-v2-v3-3-3128447b67b6@gmail.com/
> Assisted-by: Codex:gpt-5.6-sol sparse
> Co-developed-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Maurizio Casciano <mauriziocasciano7@gmail.com>

Looks better, a bit of work and we have a merge candidate.

> +static void cht_rt5677_platform_clock_disable(struct snd_soc_card *card,
> +					      struct snd_soc_dai *codec_dai)
> +{
> +	struct cht_rt5677_private *ctx = snd_soc_card_get_drvdata(card);
> +	int ret;
> +
> +	ret = snd_soc_dai_set_sysclk(codec_dai, RT5677_SCLK_S_RCCLK,
> +				     48000 * 512, SND_SOC_CLOCK_IN);
> +	if (ret)
> +		dev_warn(card->dev, "setting codec idle sysclk failed: %d\n", ret);
> +
> +	clk_disable_unprepare(ctx->mclk);
> +}
> +
> +static int cht_rt5677_platform_clock_control(struct snd_soc_dapm_widget *w,
> +					     struct snd_kcontrol *kctl,
> +					     int event)
> +{
> +	struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
> +	struct snd_soc_dai *codec_dai;
> +
> +	codec_dai = snd_soc_card_get_codec_dai(card, CHT_CODEC_DAI);
> +	if (!codec_dai) {
> +		dev_err(card->dev, "codec DAI not found\n");
> +		return -EIO;
> +	}
> +
> +	if (SND_SOC_DAPM_EVENT_ON(event))
> +		return cht_rt5677_platform_clock_enable(card, codec_dai);
> +
> +	cht_rt5677_platform_clock_disable(card, codec_dai);
> +
> +	return 0;

I believe clock_control(event=disable) should not be ignoring result of
cht_rt5677_platform_clock_disable().  Yeah, currently the function
returns void but with small update, this could be corrected.  In
general, the right approach is based on the use-case - teardown path
invoked during error handling should be persmissive.  Conscious disable
operation (here, clock control) I believe shall not.

> +}
> +
> +static int cht_rt5677_hp_event(struct snd_soc_dapm_widget *w,
> +			       struct snd_kcontrol *kctl, int event)
> +{
> +	struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
> +	struct cht_rt5677_private *ctx = snd_soc_card_get_drvdata(card);
> +
> +	gpiod_set_value_cansleep(ctx->gpio_hp_en, SND_SOC_DAPM_EVENT_ON(event));
> +
> +	return 0;

gpiod_set_value_cansleep() can fail and blind "return 0" is discouraged.

> +}
> +
> +static int cht_rt5677_spk_event(struct snd_soc_dapm_widget *w,
> +				struct snd_kcontrol *kctl, int event)
> +{
> +	struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
> +	struct cht_rt5677_private *ctx = snd_soc_card_get_drvdata(card);
> +
> +	gpiod_set_value_cansleep(ctx->gpio_spk_en1, SND_SOC_DAPM_EVENT_ON(event));
> +	gpiod_set_value_cansleep(ctx->gpio_spk_en2, SND_SOC_DAPM_EVENT_ON(event));
> +
> +	return 0;

Ditto.

> +static int cht_rt5677_codec_init(struct snd_soc_pcm_runtime *runtime)
> +{
> +	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(runtime, 0);
> +	struct snd_soc_component *component = codec_dai->component;
> +	struct cht_rt5677_private *ctx = snd_soc_card_get_drvdata(runtime->card);
> +	int ret;
> +
> +	/*
> +	 * The codec derives its asynchronous sample-rate conversion clocks from
> +	 * I2S1 while the SSP link runs from the Cherry Trail platform clock.
> +	 */
> +	rt5677_sel_asrc_clk_src(component, RT5677_DA_STEREO_FILTER |
> +					    RT5677_AD_STEREO1_FILTER |
> +					    RT5677_I2S1_SOURCE,
> +				     RT5677_CLK_SEL_I2S1_ASRC);

Please do not ignore the result of rt5677_sel_asrc_clk_src().

> +
> +	/* Mono ADC L uses the codec system clock rather than the I2S1 clock. */
> +	rt5677_sel_asrc_clk_src(component, RT5677_AD_MONO_L_FILTER, RT5677_CLK_SEL_SYS2);

Ditto.

> +
> +	/* Firmware may leave MCLK enabled without updating the CCF count. */
> +	ret = clk_prepare_enable(ctx->mclk);
> +	if (ret) {
> +		dev_err(runtime->dev, "preparing MCLK failed: %d\n", ret);
> +		return ret;
> +	}
> +	clk_disable_unprepare(ctx->mclk);
> +
> +	ret = clk_set_rate(ctx->mclk, CHT_PLAT_CLK_3_HZ);
> +	if (ret) {
> +		dev_err(runtime->dev, "setting MCLK rate failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

> +SND_SOC_DAILINK_DEF(dummy, DAILINK_COMP_ARRAY(COMP_DUMMY()));
> +
> +SND_SOC_DAILINK_DEF(media, DAILINK_COMP_ARRAY(COMP_CPU("media-cpu-dai")));
> +
> +SND_SOC_DAILINK_DEF(deepbuffer, DAILINK_COMP_ARRAY(COMP_CPU("deepbuffer-cpu-dai")));
> +
> +SND_SOC_DAILINK_DEF(ssp2_port, DAILINK_COMP_ARRAY(COMP_CPU("ssp2-port")));
> +SND_SOC_DAILINK_DEF(ssp2_codec, DAILINK_COMP_ARRAY(COMP_CODEC(RT5677_I2C, CHT_CODEC_DAI)));
> +
> +SND_SOC_DAILINK_DEF(platform, DAILINK_COMP_ARRAY(COMP_PLATFORM("sst-mfld-platform")));

I'd suggest to drop the newlines between SND_SOC_DAILINK_DEF entries.
Perhaps also have a tab(s) instead of a space before DAILINK_COMP_ARRAY
so all the definitions are aligned nicely.

> +
> +static const struct snd_soc_dai_link cht_rt5677_dailink[] = {
> +	/* Front End DAI links */
> +	[MERR_DPCM_AUDIO] = {
> +		.name = "Audio Port",
> +		.stream_name = "Audio",
> +		.nonatomic = true,
> +		.dynamic = 1,
> +		.ops = &cht_rt5677_aif1_ops,
> +		SND_SOC_DAILINK_REG(media, dummy, platform),
> +	},
> +	[MERR_DPCM_DEEP_BUFFER] = {
> +		.name = "Deep-Buffer Audio Port",
> +		.stream_name = "Deep-Buffer Audio",

Did you test both endpoints, the low-latency and the high-latency one?
While I did not touch atom-driver really, when refactoring its sibling
(now called catpt-driver) I had to do quite a bit of tinkering to verify
the high-latency playback path actually works.

> +		.nonatomic = true,
> +		.dynamic = 1,
> +		.playback_only = 1,
> +		.ops = &cht_rt5677_aif1_ops,
> +		SND_SOC_DAILINK_REG(deepbuffer, dummy, platform),
> +	},
> +
> +	/* Back End DAI links */
> +	{
> +		/* SSP2 - Codec */
> +		.name = "SSP2-Codec",
> +		.id = 0,
> +		.no_pcm = 1,
> +		.nonatomic = true,
> +		.init = cht_rt5677_codec_init,
> +		.be_hw_params_fixup = cht_rt5677_codec_fixup,
> +		.ops = &cht_rt5677_be_ssp2_ops,
> +		SND_SOC_DAILINK_REG(ssp2_port, ssp2_codec, platform),
> +	},
> +};
> +
> +/* SoC card */

The comment is redundant.

> +static const struct snd_soc_card cht_rt5677_card = {
> +	.owner = THIS_MODULE,
> +	.num_links = ARRAY_SIZE(cht_rt5677_dailink),
> +	.num_aux_devs = 1,
> +	.dapm_widgets = cht_rt5677_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(cht_rt5677_widgets),
> +	.dapm_routes = cht_rt5677_map,
> +	.num_dapm_routes = ARRAY_SIZE(cht_rt5677_map),
> +	.controls = cht_rt5677_controls,
> +	.num_controls = ARRAY_SIZE(cht_rt5677_controls),
> +};
> +
> +static const struct acpi_gpio_params speaker_enable_gpio = { 2, 0, false };
> +static const struct acpi_gpio_mapping cht_rt5677_gpios[] = {
> +	{ "speaker-enable-gpios", &speaker_enable_gpio, 1 },
> +	{ }
> +};
> +
> +#define SOF_CARD_NAME "cht yogabook"
> +#define SOF_DRIVER_NAME "SOF"
> +
> +#define CARD_NAME "cht-rt5677"
> +#define DRIVER_NAME NULL

I'm going to repeat myself from v2 [1]:

Have you tested the driver with both, legacy -and- SOF firmware? If
you're using just one of them, let's limit the driver to that one.
Anything else can be part of a follow up series if there is a need to
support multiple solutions. Otherwise we'd be merging code with no
coverage and no user.

[1]:
https://lore.kernel.org/all/7932c58b-b6fa-40c3-8967-7710d84f9667@intel.com/

> +static int snd_cht_rt5677_probe(struct platform_device *pdev)

> +
> +	card->dev = dev;
> +	platform_name = mach->mach_params.platform;
> +
> +	ret = snd_soc_fixup_dai_links_platform_name(card, platform_name);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "fixing DAI link platform name failed\n");

Could you run a test without snd_soc_fixup_dai_links_platform_name() ?
The function is more of a relict of the past.