Re: [PATCH v2 3/3] ASoC: Intel: cht_yogabook: Add driver for Lenovo Yoga Book tablets

[Yauhen Kharuzhy <jekhor@gmail.com>]

On Mon, Mar 02, 2026 at 01:03:04PM +0100, Cezary Rojewski wrote:
> 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.

So, should it just be named "cht-rt5677", but the code inside will be Yoga
Book-specific without generalization? Or is it better to make the code as
generic as possible and add machine-specific things via some kind of quirks
selected by DMI data, for example? I doubt if we will meet another
working Cherry Trail-based device with RT5677 codec in the future.

> 
> For the entire driver:
> - fix alignments

There are alignment rule violations only at SND_SOC_DAILINK_DEF()
declarations where they looks acceptable. I just copied their style from
other intel board drivers.

But OK, I will change this.

> - fix char-per-line limit, submission rules expect 100c-per-line limit

checkpatch.pl reports no such violations except of the link in the commit
description.


> 
> 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@gmail.com>
> 
> ...
> 
> 
> > 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@suse.de/

OK.

> 
> >   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@gmail.com>
> > + *
> > + *  Based on cht_bsw_rt5672.c:
> > + *  Copyright (C) 2014 Intel Corp
> > + *  Author: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
> > + *          Mengdong Lin <mengdong.lin@intel.com>
> > + *
> > + *  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@intel.com>
> 
> 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.

OK

> 
> > + */
> > +
> > +#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.

OK.

> > +{
> > +	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()
> 
> 

OK

> > +	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.

OK

> 
> > +
> > +	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.

OK

> 
> > +	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.

ok

> 
> > +		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?

Yes, you are correct, the driver fails to probe if the mclk is not obtained.
Will remove such checks.

> 
> > +		/*
> > +		 * 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.

No, I tested only with non-SOF firmware because topology file for rt5677
is missing. Agree, I will remove SOF related code.

> 
> > +
> > +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.

OK

> 
> > +	struct snd_soc_acpi_mach *mach = pdev->dev.platform_data;
> 
> dev_get_platdata(), no reason to avoid well-established APIs.
OK

> 
> > +	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.

hmm... OK but why? Most sound drivers use a static approach to simplify
declaration.

> 
> > +	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.

Sure, snaked from the cht_bsw_rt5672.c

> 
> > +
> > +	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.

OK

> 
> > +	},
> > +	.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");
> > 
> 

-- 
Yauhen Kharuzhy