Re: [PATCH v3 3/3] ASoC: Intel: Add cht_rt5677 driver
From: Cezary Rojewski
Date: Thu Jun 11 2026 - 05:22:51 EST
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,