[alsa-devel] [PATCH] ASoC: nau8540: new codec driver

[Mark Brown]

On Thu, Jan 05, 2017 at 02:20:44PM +0800, John Hsu wrote:

This looks good overall, a couple of small points though:

> +static int nau8540_config_sysclk(struct nau8540 *nau8540,
> +       int clk_id, unsigned int freq)
> +{
> +       struct regmap *regmap = nau8540->regmap;
> +
> +       switch (clk_id) {
> +       case NAU8540_CLK_DIS:
> +       case NAU8540_CLK_MCLK:
> +               regmap_update_bits(regmap, NAU8540_REG_CLOCK_SRC,
> +                       NAU8540_CLK_SRC_MASK, NAU8540_CLK_SRC_MCLK);
> +               regmap_update_bits(regmap, NAU8540_REG_FLL6,
> +                       NAU8540_DCO_EN, 0);
> +               break;
> +
> +       case NAU8540_CLK_INTERNAL:
> +               regmap_update_bits(regmap, NAU8540_REG_FLL6,
> +                       NAU8540_DCO_EN, NAU8540_DCO_EN);
> +               regmap_update_bits(regmap, NAU8540_REG_CLOCK_SRC,
> +                       NAU8540_CLK_SRC_MASK, NAU8540_CLK_SRC_VCO);
> +               break;

The above are fine but...

> +
> +       case NAU8540_CLK_FLL_MCLK:
> +               regmap_update_bits(regmap, NAU8540_REG_FLL3,
> +                       NAU8540_FLL_CLK_SRC_MASK, NAU8540_FLL_CLK_SRC_MCLK);
> +               break;
> +
> +       case NAU8540_CLK_FLL_BLK:
> +               regmap_update_bits(regmap, NAU8540_REG_FLL3,
> +                       NAU8540_FLL_CLK_SRC_MASK, NAU8540_FLL_CLK_SRC_BLK);
> +               break;
> +
> +       case NAU8540_CLK_FLL_FS:
> +               regmap_update_bits(regmap, NAU8540_REG_FLL3,
> +                       NAU8540_FLL_CLK_SRC_MASK, NAU8540_FLL_CLK_SRC_FS);
> +               break;

...these look like they're mixing in FLL configuration which I'd expect
to be done with set_pll() via the source argument and I'm not seeing
anything that sets the device to use the FLL here.  I'd expect the
source selection to go into set_pll() and I'd expect this function to
have a setting which tells the device to use the FLL.

> +static int nau8540_set_sysclk(struct snd_soc_codec *codec,
> +       int clk_id, int source, unsigned int freq, int dir)
> +{
> +       struct nau8540 *nau8540 = snd_soc_codec_get_drvdata(codec);
> +
> +       return nau8540_config_sysclk(nau8540, clk_id, freq);
> +}

This wrapper isn't really adding anything.

> +static void nau8540_init_regs(struct nau8540 *nau8540)
> +{
> +       struct regmap *regmap = nau8540->regmap;
> +
> +       /* Enable Bias/VMID/VMID Tieoff */
> +       regmap_update_bits(regmap, NAU8540_REG_VMID_CTRL,
> +               NAU8540_VMID_EN | NAU8540_VMID_SEL_MASK,
> +               NAU8540_VMID_EN | (0x2 << NAU8540_VMID_SEL_SFT));
> +       regmap_update_bits(regmap, NAU8540_REG_REFERENCE,
> +               NAU8540_PRECHARGE_DIS | NAU8540_GLOBAL_BIAS_EN,
> +               NAU8540_PRECHARGE_DIS | NAU8540_GLOBAL_BIAS_EN);
> +       mdelay(2);
> +       regmap_update_bits(regmap, NAU8540_REG_MIC_BIAS,
> +               NAU8540_PU_PRE, NAU8540_PU_PRE);
> +       regmap_update_bits(regmap, NAU8540_REG_CLOCK_CTRL,
> +               NAU8540_CLK_ADC_EN | NAU8540_CLK_I2S_EN,
> +               NAU8540_CLK_ADC_EN | NAU8540_CLK_I2S_EN);
> +       /* ADC OSR selection, CLK_ADC = Fs * OSR */
> +       regmap_update_bits(regmap, NAU8540_REG_ADC_SAMPLE_RATE,
> +               NAU8540_ADC_OSR_MASK, NAU8540_ADC_OSR_64);
> +}

Does this sequence need to be done as part of resume as well?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20170109/e9c749cc/attachment.sig>