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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

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.

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

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

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