From: John Hsu <KCHSU0@nuvoton.com>
To: Anatol Pomozov <anatol.pomozov@gmail.com>
Cc: "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
Ben Zhang <benzh@chromium.org>,
Liam Girdwood <lgirdwood@gmail.com>,
YHCHuang@nuvoton.com, Mark Brown <broonie@kernel.org>,
CTLIN0@nuvoton.com, Yong Zhi <yong.zhi@intel.com>
Subject: Re: [PATCH] ASoC: Extend FLL function
Date: Thu, 25 Feb 2016 06:51:00 +0800 [thread overview]
Message-ID: <56CE33D4.10009@nuvoton.com> (raw)
In-Reply-To: <CAOMFOmX_fu94AYJpP5x4=r9GXb5CztxhMNNv7rHENO8fWbgjeg@mail.gmail.com>
Hi Mark,
Could you help to review and merge this patch?
Some patches later about nau8825 driver will patch base on it. Thank you
very much.
BR.
John
On 12/31/2015 5:14 PM, Anatol Pomozov wrote:
> Hi
>
> On Wed, Dec 30, 2015 at 11:55 PM, John Hsu <KCHSU0@nuvoton.com> wrote:
>
>> Extend FFL clock source from MCLK/BCLK/FS;
>> And modify some FLL parameter setting.
>>
>> Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
>> ---
>> sound/soc/codecs/nau8825.c | 100 +++++++++++++++++++++++++++++++++------------
>> sound/soc/codecs/nau8825.h | 14 ++++++-
>> 2 files changed, 86 insertions(+), 28 deletions(-)
>>
>> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
>> index 7fc7b4e..16da8ef 100644
>> --- a/sound/soc/codecs/nau8825.c
>> +++ b/sound/soc/codecs/nau8825.c
>> @@ -926,7 +926,8 @@ static void nau8825_fll_apply(struct nau8825 *nau8825,
>> struct nau8825_fll *fll_param)
>> {
>> regmap_update_bits(nau8825->regmap, NAU8825_REG_CLK_DIVIDER,
>> - NAU8825_CLK_MCLK_SRC_MASK, fll_param->mclk_src);
>> + NAU8825_CLK_SRC_MASK | NAU8825_CLK_MCLK_SRC_MASK,
>> + NAU8825_CLK_SRC_MCLK | fll_param->mclk_src);
>> regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL1,
>> NAU8825_FLL_RATIO_MASK, fll_param->ratio);
>> /* FLL 16-bit fractional input */
>> @@ -939,10 +940,20 @@ static void nau8825_fll_apply(struct nau8825 *nau8825,
>> NAU8825_FLL_REF_DIV_MASK, fll_param->clk_ref_div);
>> /* select divided VCO input */
>> regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
>> - NAU8825_FLL_FILTER_SW_MASK, 0x0000);
>> - /* FLL sigma delta modulator enable */
>> + NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
>> + NAU8825_FLL_FILTER_SW_MASK,
>> + NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
>> + NAU8825_FLL_FILTER_SW_REF);
>> + /* Disable free-running mode */
>> regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
>> - NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
>> + NAU8825_DCO_EN, 0);
>> + /* FLL sigma delta modulator enable */
>> + if (fll_param->fll_frac)
>> + regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
>> + NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
>> + else
>> + regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
>> + NAU8825_SDM_EN_MASK, 0);
>> }
>>
>> /* freq_out must be 256*Fs in order to achieve the best performance */
>> @@ -970,6 +981,37 @@ static int nau8825_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
>> return 0;
>> }
>>
>> +static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
>> +{
>> + int ret = 0;
>> +
>> + /* We selected MCLK source but the clock itself managed externally */
>> + if (!nau8825->mclk)
>> + goto done;
>>
>
> A nitpick.
>
> This "goto" style is used for recovering resources or something that
> should be restored in case of error. But in your case "done:" does not
> recover anything. It is cleaner remove "goto" and replace with "return
> 0" or "return err".
>
>
>> +
>> + if (!nau8825->mclk_freq) {
>> + ret = clk_prepare_enable(nau8825->mclk);
>> + if (ret) {
>> + dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
>> + goto done;
>> + }
>> + }
>> +
>> + if (nau8825->mclk_freq != freq) {
>> + nau8825->mclk_freq = freq;
>> +
>> + freq = clk_round_rate(nau8825->mclk, freq);
>> + ret = clk_set_rate(nau8825->mclk, freq);
>> + if (ret) {
>> + dev_err(nau8825->dev, "Unable to set mclk rate\n");
>> + goto done;
>> + }
>> + }
>> +
>> +done:
>> + return ret;
>> +}
>> +
>> static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>> unsigned int freq)
>> {
>> @@ -981,29 +1023,9 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>> regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
>> NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_MCLK);
>> regmap_update_bits(regmap, NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
>> -
>> - /* We selected MCLK source but the clock itself managed externally */
>> - if (!nau8825->mclk)
>> - break;
>> -
>> - if (!nau8825->mclk_freq) {
>> - ret = clk_prepare_enable(nau8825->mclk);
>> - if (ret) {
>> - dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
>> - return ret;
>> - }
>> - }
>> -
>> - if (nau8825->mclk_freq != freq) {
>> - nau8825->mclk_freq = freq;
>> -
>> - freq = clk_round_rate(nau8825->mclk, freq);
>> - ret = clk_set_rate(nau8825->mclk, freq);
>> - if (ret) {
>> - dev_err(nau8825->dev, "Unable to set mclk rate\n");
>> - return ret;
>> - }
>> - }
>> + ret = nau8825_mclk_prepare(nau8825, freq);
>> + if (ret)
>> + return ret;
>>
>> break;
>> case NAU8825_CLK_INTERNAL:
>> @@ -1018,6 +1040,30 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>> }
>>
>> break;
>> + case NAU8825_CLK_FLL_MCLK:
>> + regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> + NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_MCLK);
>> + ret = nau8825_mclk_prepare(nau8825, freq);
>> + if (ret)
>> + return ret;
>> +
>> + break;
>> + case NAU8825_CLK_FLL_BLK:
>> + regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> + NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_BLK);
>> + ret = nau8825_mclk_prepare(nau8825, freq);
>>
>
>
> Based on names for these constants I believe CODEC is going to use
> BCLK/FS signal of I2S bus as a clock signal reference.
>
> Do you still need MCLK signal in this case? If no then you should
> disable MCLK to save some power.
>
>
>
>> + if (ret)
>> + return ret;
>> +
>> + break;
>> + case NAU8825_CLK_FLL_FS:
>> + regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> + NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_FS);
>> + ret = nau8825_mclk_prepare(nau8825, freq);
>> + if (ret)
>> + return ret;
>> +
>> + break;
>> default:
>> dev_err(nau8825->dev, "Invalid clock id (%d)\n", clk_id);
>> return -EINVAL;
>> diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
>> index dff8edb..e8f7ca5 100644
>> --- a/sound/soc/codecs/nau8825.h
>> +++ b/sound/soc/codecs/nau8825.h
>> @@ -112,12 +112,21 @@
>>
>> /* FLL3 (0x06) */
>> #define NAU8825_FLL_INTEGER_MASK (0x3ff << 0)
>> +#define NAU8825_FLL_CLK_SRC_SFT 10
>> +#define NAU8825_FLL_CLK_SRC_MASK (0x3 << NAU8825_FLL_CLK_SRC_SFT)
>> +#define NAU8825_FLL_CLK_SRC_MCLK (0 << NAU8825_FLL_CLK_SRC_SFT)
>> +#define NAU8825_FLL_CLK_SRC_BLK (0x2 << NAU8825_FLL_CLK_SRC_SFT)
>> +#define NAU8825_FLL_CLK_SRC_FS (0x3 << NAU8825_FLL_CLK_SRC_SFT)
>>
>> /* FLL4 (0x07) */
>> #define NAU8825_FLL_REF_DIV_MASK (0x3 << 10)
>>
>> /* FLL5 (0x08) */
>> -#define NAU8825_FLL_FILTER_SW_MASK (0x1 << 14)
>> +#define NAU8825_FLL_PDB_DAC_EN (0x1 << 15)
>> +#define NAU8825_FLL_LOOP_FTR_EN (0x1 << 14)
>> +#define NAU8825_FLL_FILTER_SW_MASK (0x1 << 13)
>> +#define NAU8825_FLL_FILTER_SW_N2 (0x1 << 13)
>> +#define NAU8825_FLL_FILTER_SW_REF (0x0 << 13)
>>
>> /* FLL6 (0x9) */
>> #define NAU8825_DCO_EN_MASK (0x1 << 15)
>> @@ -306,6 +315,9 @@
>> enum {
>> NAU8825_CLK_MCLK = 0,
>> NAU8825_CLK_INTERNAL,
>> + NAU8825_CLK_FLL_MCLK,
>> + NAU8825_CLK_FLL_BLK,
>> + NAU8825_CLK_FLL_FS,
>> };
>>
>> struct nau8825 {
>> --
>> 1.9.1
>>
>>
> .
>
>
next prev parent reply other threads:[~2016-02-24 22:51 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-31 7:55 [PATCH] ASoC: Extend FLL function John Hsu
2015-12-31 9:14 ` Anatol Pomozov
2016-01-04 10:04 ` John Hsu
2016-02-24 22:51 ` John Hsu [this message]
2016-02-25 1:55 ` Mark Brown
2016-02-25 17:34 ` John Hsu
2016-02-26 1:34 ` Mark Brown
2016-02-26 16:16 ` John Hsu
-- strict thread matches above, loose matches on Subject: below --
2016-01-06 7:27 [PATCH] ASoC: extend " John Hsu
2016-01-07 1:26 ` Anatol Pomozov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56CE33D4.10009@nuvoton.com \
--to=kchsu0@nuvoton.com \
--cc=CTLIN0@nuvoton.com \
--cc=YHCHuang@nuvoton.com \
--cc=alsa-devel@alsa-project.org \
--cc=anatol.pomozov@gmail.com \
--cc=benzh@chromium.org \
--cc=broonie@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=yong.zhi@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox