Re: [PATCH v3 3/3] ASoC: Intel: Add cht_rt5677 driver

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

On 6/11/2026 1:51 AM, Yauhen Kharuzhy wrote:
> Add a new ASoC machine driver for Intel Cherry Trail platforms with
> rt5677 codec.

...

> +struct cht_rt5677_private {
> +	struct snd_soc_card card;
> +	char codec_name[SND_ACPI_I2C_ID_LEN];
> +	struct snd_soc_jack jack;

I'm strongly against any private context encompassing card and jack as 
fields (sick!).  The design looks off - it's very unlikely you need to 
store the card (even as pointer) at all.  All the APIs found below 
either hand you *card on the platter or provide means to do so e.g.: 
component->card.  Retrieving private context from the card is a formality.

> +	struct clk *mclk;
> +	struct gpio_desc *gpio_spk_en1;
> +	struct gpio_desc *gpio_spk_en2;
> +	struct gpio_desc *gpio_hp_en;
> +};
> +
> +static int cht_rt5677_platform_clock_enable(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 = clk_prepare_enable(ctx->mclk);

(save: enable succeeds)

> +	if (ret < 0) {
> +		dev_err(card->dev, "could not configure MCLK state: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* set codec PLL source to the 19.2MHz platform clock (MCLK) */
> +	ret = snd_soc_dai_set_pll(codec_dai, 0, RT5677_PLL1_S_MCLK,
> +				  CHT_PLAT_CLK_3_HZ, 48000 * 512);
> +	if (ret < 0) {
> +		dev_err(card->dev, "can't set codec pll: %d\n", ret);
> +		return ret;

(save: set_pll fails)

Result: clk is still running despite cht_rt5677_platform_clock_enable() 
failing.

> +	}
> +
> +	/* set codec sysclk source to PLL */
> +	ret = snd_soc_dai_set_sysclk(codec_dai, RT5677_SCLK_S_PLL1,
> +				     48000 * 512, SND_SOC_CLOCK_IN);
> +	if (ret < 0) {
> +		dev_err(card->dev, "can't set codec sysclk: %d\n", ret);
> +		return ret;

And the story repeats itself here too.

> +	}
> +
> +	return 0;
> +}

...

> +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 = 0;
> +
> +	/* Enable codec ASRC function for Stereo DAC/Stereo1 ADC/DMIC/I2S1.
> +	 * The ASRC clock source is clk_i2s1_asrc.
> +	 */

Explain _why_, don't just repeat the what's clearly visible in the 
parameter list.  Otherwise, just drop the comment.

> +	rt5677_sel_asrc_clk_src(component, RT5677_DA_STEREO_FILTER |
> +			RT5677_AD_STEREO1_FILTER | RT5677_I2S1_SOURCE,
> +			RT5677_CLK_SEL_I2S1_ASRC);
> +	/* Enable codec ASRC function for Mono ADC L.
> +	 * The ASRC clock source is clk_sys2_asrc.
> +	 */

Ditto.

> +	rt5677_sel_asrc_clk_src(component, RT5677_AD_MONO_L_FILTER,
> +				RT5677_CLK_SEL_SYS2);
> +
> +	/*
> +	 * The firmware might enable the clock at
> +	 * boot (this information may or may not
> +	 * be reflected in the enable clock register).
> +	 * To change the rate we must disable the clock
> +	 * first to cover these cases. Due to common
> +	 * clock framework restrictions that do not allow
> +	 * to disable a clock that has not been enabled,
> +	 * we need to enable the clock first.
> +	 */
> +	ret = clk_prepare_enable(ctx->mclk);
> +	if (!ret)
> +		clk_disable_unprepare(ctx->mclk);
> +
> +	ret = clk_set_rate(ctx->mclk, CHT_PLAT_CLK_3_HZ);
> +	if (ret) {
> +		dev_err(runtime->dev, "unable to set MCLK rate\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

...

> +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 },
> +	{ NULL }

Drop the NULL.

> +};
> +
> +static int snd_cht_rt5677_probe(struct platform_device *pdev)

...

> +	ctx->gpio_hp_en = gpiod_get(codec_dev, "headphone-enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->gpio_hp_en)) {
> +		ret = PTR_ERR(ctx->gpio_hp_en);
> +		dev_err_probe(codec_dev, ret, "getting headphone enable GPIO\n");
> +		goto out_put_spken2_gpio;
> +	}
> +
> +	/* override platform name, if required */

The comment is redundant, explains nothing, drop it.

> +	platform_name = mach->mach_params.platform;
> +
> +	ret = snd_soc_fixup_dai_links_platform_name(card, platform_name);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "fixing up dai links platform name\n");
> +		goto out_put_hpen_gpio;
> +	}
> +
> +	ctx->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
> +	if (IS_ERR(ctx->mclk)) {
> +		ret = PTR_ERR(ctx->mclk);
> +		dev_err_probe(&pdev->dev, ret, "getting MCLK from pmc_plt_clk_3\n");
> +		goto out_put_hpen_gpio;
> +	}
> +
> +	snd_soc_card_set_drvdata(card, ctx);
> +
> +	/* register the soc card */

Ditto.  The funtion's name says it all.

> +	ret = devm_snd_soc_register_card(&pdev->dev, card);
> +	if (ret) {
> +		dev_err_probe(&pdev->dev, ret, "registering card\n");
> +		goto out_put_hpen_gpio;
> +	}
> +
> +	return 0;
> +
> +out_put_hpen_gpio:
> +	gpiod_put(ctx->gpio_hp_en);
> +out_put_spken2_gpio:
> +	gpiod_put(ctx->gpio_spk_en2);
> +out_put_spken_gpio:
> +	gpiod_put(ctx->gpio_spk_en1);
> +
> +	return ret;
> +}
> +
> +static void snd_cht_rt5677_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct cht_rt5677_private *ctx = snd_soc_card_get_drvdata(card);
> +	int i;
> +
> +	/*
> +	 * Reset the codec name in the global dailink array back to the default
> +	 * to avoid a use-after-free on driver rebind (unbind/bind):
> +	 * ctx->codec_name is about to be freed together with ctx (devm), but
> +	 * cht_rt5677_dailink[] is a static global that persists across binds.
> +	 */

All of this looks like a hack and brings me closer to simply stating:

NAK

as I do not believe the patch has been thoroughly tested if such hacks 
exist.  remove() procedure should not care about codecs->name.

> +	for (i = 0; i < ARRAY_SIZE(cht_rt5677_dailink); i++) {
> +		if (cht_rt5677_dailink[i].codecs->name == ctx->codec_name) {
> +			cht_rt5677_dailink[i].codecs->name = RT5677_I2C_DEFAULT;
> +			break;
> +		}
> +	}
> +
> +	gpiod_put(ctx->gpio_hp_en);
> +	gpiod_put(ctx->gpio_spk_en2);
> +	gpiod_put(ctx->gpio_spk_en1);
> +}

...

> diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> index 5e8a1dc84ee1..c719c3ec8314 100644
> --- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c
> @@ -17,12 +17,11 @@ static struct snd_soc_acpi_mach cht_surface_mach = {
>   	.sof_tplg_filename = "sof-cht-rt5645.tplg",
>   };
>   
> -static struct snd_soc_acpi_mach cht_yogabook_mach = {
> +static struct snd_soc_acpi_mach cht_rt5677_mach = {
>   	.id = "10EC5677",
> -	.drv_name = "cht-yogabook",
> +	.drv_name = "cht-rt5677",
>   	.fw_filename = "intel/fw_sst_22a8.bin",
> -	.board = "cht-yogabook",
> -	.sof_tplg_filename = "sof-cht-rt5677.tplg",
> +	.board = "cht_rt5677",
>   };
>   
>   static struct snd_soc_acpi_mach cht_lenovo_yoga_tab3_x90_mach = {
> @@ -41,17 +40,9 @@ static const struct dmi_system_id cht_table[] = {
>   			DMI_MATCH(DMI_PRODUCT_NAME, "Surface 3"),
>   		},
>   	},
> -	{
> -		.ident = "Lenovo Yoga Book YB1-X91",
> -		.driver_data = (void *)&cht_yogabook_mach,
> -		/* YB1-X91L/F */
> -		.matches = {
> -			DMI_MATCH(DMI_PRODUCT_NAME, "Lenovo YB1-X91"),
> -		}
> -	},
>   	{
>   		.ident = "Lenovo Yoga Book YB1-X90",
> -		.driver_data = (void *)&cht_yogabook_mach,
> +		.driver_data = (void *)&cht_rt5677_mach,
>   		/* YB1-X90L/F, codec is not listed in DSDT */
>   		.matches = {
>   			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
> @@ -147,19 +138,11 @@ struct snd_soc_acpi_mach  snd_soc_acpi_intel_cherrytrail_machines[] = {
>   		.board = "cht-bsw",
>   		.sof_tplg_filename = "sof-cht-rt5670.tplg",
>   	},
> -	/*
> -	 * The only known Cherry Trail device with RT5677 codec and 10EC677
> -	 * DSTD entry is the Lenovo Yoga Book YB1-X91. It has a device-specific
> -	 * driver, so check DMI and use a machine quirk to override the default
> -	 * (non-existent) machine driver.
> -	 */

You are removing and editing (the below table) code you've just added 
with 2/3.  This looks very bad.

>   	{
>   		.id = "10EC5677",
> -		.drv_name = "cht-bsw-rt5677",
> +		.drv_name = "cht-rt5677",
>   		.fw_filename = "intel/fw_sst_22a8.bin",
> -		.board = "cht-bsw",
> -		.machine_quirk = cht_quirk,
> -		.sof_tplg_filename = "sof-cht-rt5677.tplg",
> +		.board = "cht_rt5677",
>   	},
>   	{
>   		.comp_ids = &rt5645_comp_ids,
>