Re: [PATCH v1 1/2] ASoC: Intel: cht_yogabook: Add driver for Lenovo Yoga Book tablets

[Yauhen Kharuzhy]

On Wed, Feb 18, 2026 at 11:31:38AM +0100, Pierre-Louis Bossart wrote:
> Thanks for the patch, looks mostly good to me, see below a couple of comments.
> 
> > +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_mc_private *ctx = snd_soc_card_get_drvdata(card);
> > +
> > +	dev_dbg(card->dev, "HP event: %s\n",
> > +		SND_SOC_DAPM_EVENT_ON(event) ? "ON" : "OFF");
> > +
> > +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> > +		msleep(20);
> > +		gpiod_set_value_cansleep(ctx->gpio_hp_en, 1);
> > +		msleep(50);
> > +	} else {
> > +		gpiod_set_value_cansleep(ctx->gpio_hp_en, 0);
> > +	}
> 
> It'd be worth having a comment on why you need the 2 separate msleep.
> IIRC for Broadwell we only had a common 70ms sleep for both enable and disable.

Actually I don't know, it just a copy-paste from the Lenovo Android
kernel code, and I didn't check what happens if this sleeps are removed,
AFAIR, if there are any sound artifacts or something else.

Will check.

> 
> > +
> > +	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_mc_private *ctx = snd_soc_card_get_drvdata(card);
> > +
> > +	dev_dbg(card->dev, "SPK event: %s\n",
> > +		SND_SOC_DAPM_EVENT_ON(event) ? "ON" : "OFF");
> > +
> > +	/* Black magic from the Lenovo's Android kernel
> > +	 * FIXME: Check if it is needed, an original comment:
> > +	 * "use gpio GPIO_SPK_EN to enable/disable ext boost pa
> > +	 * use mode 3"
> > +	 */
> > +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> > +		gpiod_set_value_cansleep(ctx->gpio_spk_en1, 1);
> > +		udelay(2);
> > +		gpiod_set_value_cansleep(ctx->gpio_spk_en1, 0);
> > +		udelay(2);
> > +		gpiod_set_value_cansleep(ctx->gpio_spk_en1, 1);
> > +		udelay(2);
> > +		gpiod_set_value_cansleep(ctx->gpio_spk_en1, 0);
> > +		udelay(2);
> > +	}
> 
> That seems weird indeed, never seen this before. What happens if that block is removed?

Sure. Will check, I cannot remember if I checked this during initial porting...

> 
> > +	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));
> > +	msleep(50);
> > +
> > +	return 0;
> > +}
> > +
> 
> > +static int cht_yb_codec_fixup(struct snd_soc_pcm_runtime *rtd,
> > +			      struct snd_pcm_hw_params *params)
> > +{
> > +	struct snd_interval *rate = hw_param_interval(params,
> > +			SNDRV_PCM_HW_PARAM_RATE);
> > +	struct snd_interval *channels = hw_param_interval(params,
> > +						SNDRV_PCM_HW_PARAM_CHANNELS);
> > +
> > +	/* The DSP will convert the FE rate to 48k, stereo, 24bits */
> > +	rate->min = rate->max = 48000;
> > +	channels->min = channels->max = 2;
> > +
> > +	/*
> > +	 * set SSP2 to 24-bit
> > +	 * Looks like strange black magic because ssp2-port supports S16LE
> > +	 * format only, taken from Intel's code
> > +	 */
> 
> No, SSP2 supports 24 bits in TDM mode. only SSP0 has firmware restrictions to S16LE.

Thanks, I even don't remember why I added this comment. To be removed.

> 
> > +	params_set_format(params, SNDRV_PCM_FORMAT_S24_LE);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct snd_soc_jack_pin cht_yb_jack_pins[] = {
> > +	{
> > +		.pin = "Headphone",
> > +		.mask = SND_JACK_HEADPHONE,
> > +	},
> > +	{
> > +		.pin = "Headset Mic",
> > +		.mask = SND_JACK_MICROPHONE,
> > +	},
> > +};
> > +

-- 
Yauhen Kharuzhy