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

[Cezary Rojewski]

On 2026-03-03 1:12 AM, Yauhen Kharuzhy wrote:
> 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.

"cht_rt5677" sounds like a good filename for this very driver. Yeah, I 
should have used '_' when mentioning <platform>_<codec> in my previous 
response, so that the naming remains cohesive with the vast majority. Oh 
well..

In regard to the generic/specific - I typically turn to the coverage and 
the schedule to answer such question. I believe the specific tablets you 
mentioned are the only coverage for the driver. And thus I do not see a 
reason to scale beyond that.

The generic approach is good (and usually favoured) when one provides a 
feature or a driver for specific configuration XYZ_v1 but with XYZ_v2, 
that's coming a year from now, already in mind. I do not think this is 
the case either.

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

Few years ago the limit was raised and basically anything we do for 
intel-sound utilizes that limit. checkpatch warns about violations, 80 
or 100 is more of a preference. So, nothing to worry about, just a 
request to align with what we provide daily.

...

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

Most of the "legacy" machine-board drivers I'd say. If you take a look 
at SOF's or avs's (intel/avs/boards) the number of static cards is limited.

There is no _strong_ argument against or in favour of either. The reason 
I suggest to switch to the dynamic approach is similar to what you do 
here - base your code on someone else' work. When such copy lands on a 
configuration where not one but two I2C codecs are present, more than 
one sound card might be desired.
With statics, things would get messy in runtime. With dynamic allocation 
developer can forget about problems related to inheriting some unwanted 
data.

Small bonus is not occupying space for the card object until the 
platform-device (that represents the sound card) is ready for probing. 
The static descriptor occupies space the moment the module is launched.

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

These older machine-boards drivers contain some bad examples, unfortunately.