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@xxxxxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
