Hi,

On Mon, Nov 11, 2019 at 02:20:07PM +0000, Adam Thomson wrote:
> 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@collabora.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.

So you are suggesting adding something like this to
da7213_set_bias_level()?

if (freq % da7213->mclk_rate == 0)
    source = DA7213_SYSCLK_MCLK
else
    source = DA7213_SYSCLK_PLL
snd_soc_component_set_pll(component, 0, source, da7213->mclk_rate, freq);

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

/**
 * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
 *                This is only valid once the clock source has been enabled.
 * @clk: clock source
 */
unsigned long clk_get_rate(struct clk *clk);

Which makes sense for a non-fixed clock, which uses the same API.

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

oops.

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

-- Sebastian