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

[Yauhen Kharuzhy]

On Tue, Mar 03, 2026 at 10:30:22AM +0100, Cezary Rojewski wrote:
> 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..

no problem, I am going to follow existing style anyway.

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

So, it should be enough to rename the driver and keep all
machine-specific things as is until we need to support another
configuration variant (likely never). Sounds good for me.

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

Yes, avoiding of spreading a bad practice is a good reason, even for cases
where we never get more than one card instance, thanks for explanation.

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

sure.


-- 
Yauhen Kharuzhy