Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open

From: Maxime Ripard <maxime@cerno.tech>
To: Samuel Holland <samuel@sholland.org>
Cc: Ondrej Jirman <megous@megous.com>,
	alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
	Takashi Iwai <tiwai@suse.com>,
	Liam Girdwood <lgirdwood@gmail.com>, Chen-Yu Tsai <wens@csie.org>,
	Mark Brown <broonie@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 20/25] ASoC: sun8i-codec: Protect the clock rate while streams are open
Date: Mon, 5 Oct 2020 14:01:01 +0200	[thread overview]
Message-ID: <20201005120101.igzzwosnq6bzbua6@gilmour.lan> (raw)
In-Reply-To: <20201001021148.15852-21-samuel@sholland.org>

[-- Attachment #1: Type: text/plain, Size: 2987 bytes --]

On Wed, Sep 30, 2020 at 09:11:43PM -0500, Samuel Holland wrote:
> The codec's clock input is shared among all AIFs, and shared with other
> audio-related hardware in the SoC, including I2S and SPDIF controllers.
> To ensure sample rates selected by userspace or by codec2codec DAI links
> are maintained, the clock rate must be protected while it is in use.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
>  sound/soc/sunxi/sun8i-codec.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun8i-codec.c b/sound/soc/sunxi/sun8i-codec.c
> index 501af64d43a0..86065bee7cd3 100644
> --- a/sound/soc/sunxi/sun8i-codec.c
> +++ b/sound/soc/sunxi/sun8i-codec.c
> @@ -416,27 +416,32 @@ static int sun8i_codec_get_lrck_div_order(unsigned int slots,
>  	unsigned int div = slots * slot_width;
>  
>  	if (div < 16 || div > 256)
>  		return -EINVAL;
>  
>  	return order_base_2(div);
>  }
>  
> +static unsigned int sun8i_codec_get_sysclk_rate(unsigned int sample_rate)
> +{
> +	return sample_rate % 4000 ? 22579200 : 24576000;
> +}
> +
>  static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>  				 struct snd_pcm_hw_params *params,
>  				 struct snd_soc_dai *dai)
>  {
>  	struct sun8i_codec *scodec = snd_soc_dai_get_drvdata(dai);
>  	struct sun8i_codec_aif *aif = &scodec->aifs[dai->id];
>  	unsigned int sample_rate = params_rate(params);
>  	unsigned int slots = aif->slots ?: params_channels(params);
>  	unsigned int slot_width = aif->slot_width ?: params_width(params);
> -	unsigned int sysclk_rate = clk_get_rate(scodec->clk_module);
> -	int lrck_div_order, word_size;
> +	unsigned int sysclk_rate = sun8i_codec_get_sysclk_rate(sample_rate);
> +	int lrck_div_order, ret, word_size;
>  	u8 bclk_div;
>  
>  	/* word size */
>  	switch (params_width(params)) {
>  	case 8:
>  		word_size = 0x0;
>  		break;
>  	case 16:
> @@ -466,17 +471,30 @@ static int sun8i_codec_hw_params(struct snd_pcm_substream *substream,
>  			   (lrck_div_order - 4) << SUN8I_AIF1CLK_CTRL_AIF1_LRCK_DIV);
>  
>  	/* BCLK divider (SYSCLK/BCLK ratio) */
>  	bclk_div = sun8i_codec_get_bclk_div(sysclk_rate, lrck_div_order, sample_rate);
>  	regmap_update_bits(scodec->regmap, SUN8I_AIF1CLK_CTRL,
>  			   SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV_MASK,
>  			   bclk_div << SUN8I_AIF1CLK_CTRL_AIF1_BCLK_DIV);
>  
> -	if (!aif->open_streams) {
> +	/* SYSCLK rate */
> +	if (aif->open_streams) {
> +		ret = clk_set_rate(scodec->clk_module, sysclk_rate);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = clk_set_rate_exclusive(scodec->clk_module, sysclk_rate);

It's not really clear to me why we wouldn't want to always protect the
clock rate here?

> +		if (ret == -EBUSY)
> +			dev_err(dai->dev, "%s: clock is busy! Sample rate %u Hz "
> +				"conflicts with other audio streams.\n",

This string creates a checkpatch warning.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]