Re: [PATCH v2 3/3] ASoC: Intel: cht_yogabook: Add driver for Lenovo Yoga Book tablets
From: Cezary Rojewski
Date: Mon Mar 02 2026 - 07:06:02 EST
On 2026-03-01 10:33 PM, Yauhen Kharuzhy wrote:
Add a new ASoC machine driver for Lenovo Yoga Book Intel Cherry
Trail-based tablets (YB1-X90F/L, YB1-X91F/L models).
This platform uses a Realtek ALC5677 codec accompanied by a TS3A227E jack
configuration detection IC.
The driver is based on the cht_bsw_rt5672.c mainline driver and the driver
from the vendor's Android kernel [1].
There are no other known Cherry Trail platforms using an RT5677 codec, so
the driver is named 'cht_yogabook', and some device-specific tricks are
hardcoded, such as jack events and additional GPIOs controlling the
speaker amplifier.
I'd switch to more specific naming. From the Intel-maintainer point of view, it's easier to navigate between configurations when <platform>-<codec> naming is in use. TBH, we do not see "yogabook", we configuration composed of Cherrytrail-based device and rt5677 I2C codec.
For the entire driver:
- fix alignments
- fix char-per-line limit, submission rules expect 100c-per-line limit
Below additional comments.
[1] https://github.com/deadman96385/android_kernel_lenovo_yeti/blob/master/sound/soc/intel/board/cht_bl_dpcm_rt5677.c
Signed-off-by: Yauhen Kharuzhy <jekhor@xxxxxxxxx>
...
diff --git a/sound/soc/intel/boards/Makefile b/sound/soc/intel/boards/Makefile
index 25a1a9066cbf..58bd6029545b 100644
--- a/sound/soc/intel/boards/Makefile
+++ b/sound/soc/intel/boards/Makefile
@@ -15,6 +15,7 @@ snd-soc-sst-cht-bsw-nau8824-y := cht_bsw_nau8824.o
snd-soc-sst-byt-cht-cx2072x-y := bytcht_cx2072x.o
snd-soc-sst-byt-cht-da7213-y := bytcht_da7213.o
snd-soc-sst-byt-cht-es8316-y := bytcht_es8316.o
+snd-soc-sst-cht-yogabook-objs := cht_yogabook.o
Let's follow recommendation from Takashi [1]. Suffix "-objs" is obsolete.
[1]: https://lore.kernel.org/all/20240507155540.24815-13-tiwai@xxxxxxx/
snd-soc-sst-byt-cht-nocodec-y := bytcht_nocodec.o
snd-soc-sof_rt5682-y := sof_rt5682.o
snd-soc-sof_cs42l42-y := sof_cs42l42.o
@@ -47,6 +48,7 @@ obj-$(CONFIG_SND_SOC_INTEL_CHT_BSW_NAU8824_MACH) += snd-soc-sst-cht-bsw-nau8824.
obj-$(CONFIG_SND_SOC_INTEL_BYT_CHT_CX2072X_MACH) += snd-soc-sst-byt-cht-cx2072x.o
obj-$(CONFIG_SND_SOC_INTEL_BYT_CHT_DA7213_MACH) += snd-soc-sst-byt-cht-da7213.o
obj-$(CONFIG_SND_SOC_INTEL_BYT_CHT_ES8316_MACH) += snd-soc-sst-byt-cht-es8316.o
+obj-$(CONFIG_SND_SOC_INTEL_CHT_YOGABOOK_MACH) += snd-soc-sst-cht-yogabook.o
obj-$(CONFIG_SND_SOC_INTEL_BYT_CHT_NOCODEC_MACH) += snd-soc-sst-byt-cht-nocodec.o
obj-$(CONFIG_SND_SOC_INTEL_SKL_HDA_DSP_GENERIC_MACH) += snd-soc-skl_hda_dsp.o
obj-$(CONFIG_SND_SOC_INTEL_EHL_RT5660_MACH) += snd-soc-ehl-rt5660.o
diff --git a/sound/soc/intel/boards/cht_yogabook.c b/sound/soc/intel/boards/cht_yogabook.c
new file mode 100644
index 000000000000..9d945cad5660
--- /dev/null
+++ b/sound/soc/intel/boards/cht_yogabook.c
@@ -0,0 +1,551 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cht_yogabook.c - ASoc Machine driver for Lenovo Yoga Book YB1-X90/X91
+ * tablets, based on Intel Cherrytrail platform with RT5677 codec.
+ *
+ * Copyright (C) 2026 Yauhen Kharuzhy <jekhor@xxxxxxxxx>
+ *
+ * Based on cht_bsw_rt5672.c:
+ * Copyright (C) 2014 Intel Corp
+ * Author: Subhransu S. Prusty <subhransu.s.prusty@xxxxxxxxx>
+ * Mengdong Lin <mengdong.lin@xxxxxxxxx>
+ *
+ * And based on the cht_bl_dpcm_rt5677.c from the Lenovo's Android kernel:
+ * Copyright (C) 2014 Intel Corp
+ * Author: Mythri P K <mythri.p.k@xxxxxxxxx>
Not sure about the boiler plate here. Mentioning people is OK but the emails are all obsolete. Such information does not help anyone. Perhaps "based on XYZ" is enough.
+ */
+
+#include <linux/clk.h>
+#include <linux/dmi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
+#include <linux/i2c.h>
+#include <linux/input.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc-acpi.h>
+#include <sound/soc.h>
+#include "../../codecs/rt5677.h"
+#include "../../codecs/ts3a227e.h"
+#include "../atom/sst-atom-controls.h"
+
+#define RT5677_I2C_DEFAULT "i2c-rt5677"
+
+/* The platform clock #3 outputs 19.2Mhz clock to codec as I2S MCLK */
+#define CHT_PLAT_CLK_3_HZ 19200000
+#define CHT_CODEC_DAI "rt5677-aif1"
+
+struct cht_yb_private {
+ char codec_name[SND_ACPI_I2C_ID_LEN];
+ struct snd_soc_jack jack;
+ struct clk *mclk;
+ struct gpio_desc *gpio_spk_en1;
+ struct gpio_desc *gpio_spk_en2;
+ struct gpio_desc *gpio_hp_en;
+};
+
+static int cht_yb_platform_clock_control(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *k, int event)
Please replace 'k' with 'kctl' for the entire file.
+{
+ struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
+ struct snd_soc_dai *codec_dai;
+ struct cht_yb_private *ctx = snd_soc_card_get_drvdata(card);
+ int ret;
+
+ codec_dai = snd_soc_card_get_codec_dai(card, CHT_CODEC_DAI);
+ if (!codec_dai) {
+ dev_err(card->dev,
+ "Codec dai not found; Unable to set platform clock\n");
+ return -EIO;
+ }
+
+ if (SND_SOC_DAPM_EVENT_ON(event)) {
+ if (ctx->mclk) {
+ ret = clk_prepare_enable(ctx->mclk);
+ if (ret < 0) {
+ dev_err(card->dev,
+ "could not configure MCLK state");
+ 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;
+ }
+
+ /* 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;
+ }
+ } else {
+ /* Set codec sysclk source to its internal clock because codec
+ * PLL will be off when idle and MCLK will also be off by ACPI
+ * when codec is runtime suspended. Codec needs clock for jack
+ * detection and button press.
+ */
+ snd_soc_dai_set_sysclk(codec_dai, RT5677_SCLK_S_RCCLK,
+ 48000 * 512, SND_SOC_CLOCK_IN);
+
+ if (ctx->mclk)
+ clk_disable_unprepare(ctx->mclk);
+ }
The entire function is made of if-statement. I'd split this into:
xyz_platform_clock_control()
|_ xyz_platform_clock_enable()
|_ xyz_platform_clock_disable()
+ return 0;
+}
+
+static int cht_yb_hp_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *k, int event)
+{
+ struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
+ struct cht_yb_private *ctx = snd_soc_card_get_drvdata(card);
+
+ dev_dbg(card->dev, "HP event: %s\n",
+ SND_SOC_DAPM_EVENT_ON(event) ? "ON" : "OFF");
These dev_dbg() can be dropped. Function tracer or DAPM events found within tracing/events are sufficient.
+
+ gpiod_set_value_cansleep(ctx->gpio_hp_en, SND_SOC_DAPM_EVENT_ON(event));
+
+ return 0;
+}
+
+static int cht_yb_spk_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *k, int event)
+{
+ struct snd_soc_card *card = snd_soc_dapm_to_card(w->dapm);
+ struct cht_yb_private *ctx = snd_soc_card_get_drvdata(card);
+
+ dev_dbg(card->dev, "SPK event: %s\n",
+ SND_SOC_DAPM_EVENT_ON(event) ? "ON" : "OFF");
Ditto.
+
+ 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;
+}
...
+static int cht_yb_codec_init(struct snd_soc_pcm_runtime *runtime)
+{
+ int ret = 0;
Move as last.
+ struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(runtime, 0);
+ struct snd_soc_component *component = codec_dai->component;
+ struct cht_yb_private *ctx = snd_soc_card_get_drvdata(runtime->card);
+
+ /* Enable codec ASRC function for Stereo DAC/Stereo1 ADC/DMIC/I2S1.
+ * The ASRC clock source is clk_i2s1_asrc.
+ */
+ 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.
+ */
+ rt5677_sel_asrc_clk_src(component, RT5677_AD_MONO_L_FILTER,
+ RT5677_CLK_SEL_SYS2);
+
+ ctx->gpio_spk_en1 = devm_gpiod_get(component->dev, "speaker-enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->gpio_spk_en1)) {
+ dev_err(component->dev, "Can't find speaker enable GPIO\n");
+ return PTR_ERR(ctx->gpio_spk_en1);
+ }
+
+ ctx->gpio_spk_en2 = devm_gpiod_get(component->dev, "speaker-enable2",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->gpio_spk_en2)) {
+ dev_err(component->dev, "Can't find speaker enable 2 GPIO\n");
+ return PTR_ERR(ctx->gpio_spk_en2);
+ }
+
+ ctx->gpio_hp_en = devm_gpiod_get(component->dev, "headphone-enable",
+ GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->gpio_hp_en)) {
+ dev_err(component->dev, "Can't find headphone enable GPIO\n");
The messages here are misleading. "Can't find ..." when devm_gpiod_get() returns different IS_ERR() than -ENOENT is incorrect. I'd just state that XYZ operation failed and append the return code. Scales nicely into the future.
+ return PTR_ERR(ctx->gpio_hp_en);
+ }
+
+ if (ctx->mclk) {
If no-MCLK case even valid here? You're providing new driver for an older, specific configuration. Perhaps limiting the code to the actual (and only) user is the way to go?
+ /*
+ * 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;
+}
...
+#define SOF_CARD_NAME "cht yogabook"
+#define SOF_DRIVER_NAME "SOF"
+
+#define CARD_NAME "cht-yogabook"
+#define DRIVER_NAME NULL
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.
+
+static int snd_cht_yb_mc_probe(struct platform_device *pdev)
+{
+ int ret_val = 0;
+ struct cht_yb_private *drv;
No. 'drv', short for 'driver', is not the right name for the private context. Typically 'priv' or 'data' is used.
+ struct snd_soc_acpi_mach *mach = pdev->dev.platform_data;
dev_get_platdata(), no reason to avoid well-established APIs.
+ const char *platform_name;
+ struct acpi_device *adev;
+ struct device *codec_dev;
+ bool sof_parent;
+ int i;
+
+ drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+ if (!drv)
+ return -ENOMEM;
+
+ strscpy(drv->codec_name, RT5677_I2C_DEFAULT);
+
+ /* fixup codec name based on HID if ACPI node is present */
+ adev = acpi_dev_get_first_match_dev(mach->id, NULL, -1);
+ if (adev) {
+ snprintf(drv->codec_name, sizeof(drv->codec_name),
+ "i2c-%s", acpi_dev_name(adev));
+ dev_info(&pdev->dev, "real codec name: %s\n", drv->codec_name);
+
+ put_device(&adev->dev);
+ for (i = 0; i < ARRAY_SIZE(cht_yb_dailink); i++) {
+ if (cht_yb_dailink[i].codecs->name &&
+ !strcmp(cht_yb_dailink[i].codecs->name,
+ RT5677_I2C_DEFAULT)) {
+ cht_yb_dailink[i].codecs->name = drv->codec_name;
+ break;
+ }
+ }
+ }
+
+ codec_dev = bus_find_device_by_name(&i2c_bus_type, NULL,
+ drv->codec_name);
+ if (!codec_dev)
+ return -EPROBE_DEFER;
+
+ if (adev) {
+ ret_val = devm_acpi_dev_add_driver_gpios(codec_dev,
+ cht_yb_gpios);
+ if (ret_val)
+ dev_warn(&pdev->dev,
+ "Unable to add GPIO mapping table: %d\n",
+ ret_val);
+ }
+
+ /* override platform name, if required */
+ snd_soc_card_cht_yb.dev = &pdev->dev;
+ platform_name = mach->mach_params.platform;
+
+ ret_val = snd_soc_fixup_dai_links_platform_name(&snd_soc_card_cht_yb,
+ platform_name);
+ if (ret_val) {
+ dev_err(&pdev->dev,
+ "snd_soc_fixup_dai_links_platform_name failed: %d\n",
+ ret_val);
+ return ret_val;
+ }
+
+ drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+ if (IS_ERR(drv->mclk)) {
+ dev_err(&pdev->dev,
+ "Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+ PTR_ERR(drv->mclk));
+ return PTR_ERR(drv->mclk);
+ }
+ snd_soc_card_set_drvdata(&snd_soc_card_cht_yb, drv);
+
+ sof_parent = snd_soc_acpi_sof_parent(&pdev->dev);
+
+ /* set the card and driver name */
+ if (sof_parent) {
+ snd_soc_card_cht_yb.name = SOF_CARD_NAME;
+ snd_soc_card_cht_yb.driver_name = SOF_DRIVER_NAME;
+ } else {
+ snd_soc_card_cht_yb.name = CARD_NAME;
+ snd_soc_card_cht_yb.driver_name = DRIVER_NAME;
+ }
+
+ /* register the soc card */
+ snd_soc_card_cht_yb.dev = &pdev->dev;
I'd move away from static-card approach and dynamically allocate the object here, in the probe() function.
+ ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht_yb);
+ if (ret_val) {
+ dev_err(&pdev->dev,
+ "snd_soc_register_card failed %d\n", ret_val);
+ return ret_val;
+ }
+ platform_set_drvdata(pdev, &snd_soc_card_cht_yb);
This seems bogus. The probe() found here is not the function that spawned the platform-device 'pdev' yet it attempts to manipulate device's private data. At the same time there is no platform_get_drvdata() anywhere. Unless there's a strong reason for the call, please drop the line.
+
+ return ret_val;
+}
+
+static struct platform_driver snd_cht_yb_mc_driver = {
+ .driver = {
+ .name = "cht-yogabook",
It seems .pm assignment is missing. I see that the "base" is missing this too. Looks like a flaw, I'd assign snd_soc_pm_ops and re-test. The ops describe basic bring-up and tear-down procedures and I'm expecting them to notify us about any problems rather than harm the configuration.
+ },
+ .probe = snd_cht_yb_mc_probe,
+};
+
+module_platform_driver(snd_cht_yb_mc_driver);
+
+MODULE_DESCRIPTION("Lenovo Yoga Book YB1-X9x machine driver");
+MODULE_AUTHOR("Yauhen Kharuzhy");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:cht-yogabook");