linux-kernel - RE: [PATCHv1 5/5] ASoC: da7213: add default clock handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 11 Nov 2019 14:20:07 +0000
From:   Adam Thomson <Adam.Thomson.Opensource@...semi.com>
To:     Sebastian Reichel <sebastian.reichel@...labora.com>,
        Support Opensource <Support.Opensource@...semi.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
CC:     Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kernel@...labora.com" <kernel@...labora.com>
Subject: RE: [PATCHv1 5/5] ASoC: da7213: add default clock handling

On 08 November 2019 17:49, Sebastian Reichel wrote:

> This adds default clock/PLL configuration to the driver
> for usage with generic drivers like simple-card for usage
> with a fixed rate clock.
> 
> Upstreaming this requires a good way to disable the automatic
> clock handling for systems doing it manually to avoid breaking
> existing setups.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@...labora.com>
> ---
>  sound/soc/codecs/da7213.c | 34 +++++++++++++++++++++++++++++++++-
>  sound/soc/codecs/da7213.h |  1 +
>  2 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/da7213.c b/sound/soc/codecs/da7213.c
> index 197609691525..a4ed382ddfc7 100644
> --- a/sound/soc/codecs/da7213.c
> +++ b/sound/soc/codecs/da7213.c
> @@ -1163,6 +1163,8 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  			    struct snd_soc_dai *dai)
>  {
>  	struct snd_soc_component *component = dai->component;
> +	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	int freq = 0;
>  	u8 dai_ctrl = 0;
>  	u8 fs;
> 
> @@ -1188,38 +1190,54 @@ static int da7213_hw_params(struct
> snd_pcm_substream *substream,
>  	switch (params_rate(params)) {
>  	case 8000:
>  		fs = DA7213_SR_8000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 11025:
>  		fs = DA7213_SR_11025;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 12000:
>  		fs = DA7213_SR_12000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 16000:
>  		fs = DA7213_SR_16000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 22050:
>  		fs = DA7213_SR_22050;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 32000:
>  		fs = DA7213_SR_32000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 44100:
>  		fs = DA7213_SR_44100;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 48000:
>  		fs = DA7213_SR_48000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	case 88200:
>  		fs = DA7213_SR_88200;
> +		freq = DA7213_PLL_FREQ_OUT_90316800;
>  		break;
>  	case 96000:
>  		fs = DA7213_SR_96000;
> +		freq = DA7213_PLL_FREQ_OUT_98304000;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
> 
> +	/* setup PLL */
> +	if (da7213->fixed_clk_auto) {
> +		snd_soc_component_set_pll(component, 0,
> DA7213_SYSCLK_PLL,
> +					  da7213->mclk_rate, freq);
> +	}
> +

Are we happy with the PLL being always enabled? Seems like a power drain,
especially if you're using an MCLK which is a natural harmonic for the SR in 
question in which case the PLL can be bypassed. Also the bias level function in
this driver will enable and disable the MCLK, if it has been provided, so it's a
bit strange to have the PLL enabled but it's clock source taken away.

>  	snd_soc_component_update_bits(component, DA7213_DAI_CTRL,
> DA7213_DAI_WORD_LENGTH_MASK,
>  			    dai_ctrl);
>  	snd_soc_component_write(component, DA7213_SR, fs);
> @@ -1700,10 +1718,10 @@ static struct da7213_platform_data
>  	return pdata;
>  }
> 
> -
>  static int da7213_probe(struct snd_soc_component *component)
>  {
>  	struct da7213_priv *da7213 =
> snd_soc_component_get_drvdata(component);
> +	int ret;
> 
>  	pm_runtime_get_sync(component->dev);
> 
> @@ -1836,6 +1854,20 @@ static int da7213_probe(struct snd_soc_component
> *component)
>  			return PTR_ERR(da7213->mclk);
>  		else
>  			da7213->mclk = NULL;
> +	} else {
> +		/* Store clock rate for fixed clocks for automatic PLL setup */
> +		ret = clk_prepare_enable(da7213->mclk);
> +		if (ret) {
> +			dev_err(component->dev, "Failed to enable mclk\n");
> +			return ret;
> +		}

I've not gone through the clk framework code but surprised to see the need to
enable a clock to retrieve it's rate.

> +
> +		da7213->mclk_rate = clk_get_rate(da7213->mclk);
> +
> +		clk_disable_unprepare(da7213->mclk);
> +
> +		/* assume fixed clock until set_sysclk() is being called */
> +		da7213->fixed_clk_auto = true;

I don't see any code where 'fixed_clk_auto' is set to false so it seems that
previous operational usage is being broken here.

>  	}
> 
>  	return 0;
> diff --git a/sound/soc/codecs/da7213.h b/sound/soc/codecs/da7213.h
> index 97a250ea39e6..00aca0126cdb 100644
> --- a/sound/soc/codecs/da7213.h
> +++ b/sound/soc/codecs/da7213.h
> @@ -532,6 +532,7 @@ struct da7213_priv {
>  	bool master;
>  	bool alc_calib_auto;
>  	bool alc_en;
> +	bool fixed_clk_auto;
>  	struct da7213_platform_data *pdata;
>  };
> 
> --
> 2.24.0.rc1