Re: [PATCH v2] ASoC: Add support for NAU8825 codec to ASoC

[Chih-Chiang Chang <ccchang12@nuvoton.com>]

On 2015/4/20 下午 10:57, Lars-Peter Clausen wrote:
> On 04/16/2015 11:56 AM, Chih-Chiang Chang wrote:
>> The NAU88L25 is an ultra-low power high performance audio codec designed
>> for smartphone, tablet PC, and other portable devices by Nuvoton, now
>> add linux driver support for it.
>>
>> Signed-off-by: Chih-Chiang Chang <ccchang12@nuvoton.com>
> 
> Looks pretty good now.
> 
>> ---
>> v2->v1:
>>     - fixes according to Lars-Peter Clausen's review comments
>>     - removes unused platform data file
>>     - corrects the naming of DAPM input widget
>>     - fixes some wrong coding of SOC widgets and other codes
>>     - adds definition and remark for config FLL clock
>>     - moves the code of reset hardware registers from codec_probe() to i2c_probe()
>>     - removes unused codes
>>
> [...]
>> +static const struct snd_kcontrol_new nau8825_snd_controls[] = {
>> +
> 
> Here and a few other places you leave the first line in the struct 
> declaration empty. No need for that.
> 
Removed the space line in declarations.

>> +	SOC_SINGLE_TLV("MIC Volume", NAU8825_ADC_DGAIN_CTRL,
>> +				NAU8825_ADC_DGAIN_SFT,
>> +				NAU8825_ADC_VOL_RSCL_RANGE, 0, adc_vol_tlv),
>> +	SOC_DOUBLE_TLV("HP Volume", NAU8825_HSVOL_CTRL,
>> +				NAU8825_L_HSVOL_SFT, NAU8825_R_HSVOL_SFT,
>> +				NAU8825_VOL_RSCL_RANGE, 1, out_hp_vol_tlv),
>> +};
> [...]
>> +
>> +static void config_fll_clk_12m(struct snd_soc_codec *codec)
>> +{
>> +	struct nau8825_priv *nau8825 = snd_soc_codec_get_drvdata(codec);
>> +
>> +	regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
>> +				NAU8825_CLK_MCLK_SRC_MASK, 0x0003);
>> +	regmap_update_bits(nau8825->regmap, NAU8825_FLL_1,
>> +				NAU8825_FLL_RATIO_MASK, 0x0001);
>> +	/* FLL 16-bit fractional input */
>> +	regmap_write(nau8825->regmap, NAU8825_FLL_2, 0xC49B);
>> +	/* FLL 10-bit integer input */
>> +	regmap_update_bits(nau8825->regmap, NAU8825_FLL_3,
>> +				NAU8825_FLL_INTEGER_MASK, 0x0020);
>> +	/* FLL pre-scaler */
>> +	regmap_update_bits(nau8825->regmap, NAU8825_FLL_4,
>> +				NAU8825_FLL_REF_DIV_MASK, 0x0800);
> 
> This seems to use some constant dividers and multipliers for the FLL. Does 
> this expect a specific reference clock from somewhere?
>
Yes, current code expects ASoC will send 12 MHz MCLK to the codec. But
finally, we will support all possible reference clock frequencies.

>> +	/* select divied VCO input */
>> +	regmap_update_bits(nau8825->regmap, NAU8825_FLL_5,
>> +				NAU8825_FLL_FILTER_SW_MASK, 0x0000);
>> +	/* FLL sigma delta modulator enable */
>> +	regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
>> +				NAU8825_SDM_EN_MASK, 0x4000);
>> +}
>> +
>> +static void set_sys_clk(struct snd_soc_codec *codec, int sys_clk)
>> +{
>> +	struct nau8825_priv *nau8825 = snd_soc_codec_get_drvdata(codec);
>> +
>> +	pr_debug("%s :: sys_clk=%x\n", __func__, sys_clk);
>> +	switch (sys_clk) {
>> +	case NAU8825_INTERNALCLOCK:
>> +		regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
>> +				NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_DIS);
>> +		regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
>> +				NAU8825_DCO_EN_MASK, NAU8825_DCO_EN);
>> +		regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
>> +				NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_EN);
>> +		break;
>> +	case NAU8825_MCLK:
>> +	default:
>> +		regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
>> +				NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_DIS);
>> +		regmap_update_bits(nau8825->regmap, NAU8825_FLL_6,
>> +				NAU8825_DCO_EN_MASK, NAU8825_DCO_DIS);
>> +		regmap_update_bits(nau8825->regmap, NAU8825_CLK_DIVIDER,
>> +				NAU8825_SYSCLK_EN_MASK, NAU8825_SYSCLK_EN);
>> +		break;
>> +	}
>> +}
>> +
>> +static int nau8825_dai_set_sysclk(struct snd_soc_dai *dai,
>> +		int clk_id, unsigned int freq, int dir)
>> +{
>> +	struct snd_soc_codec *codec = dai->codec;
>> +
>> +	switch (clk_id) {
>> +	case NAU8825_MCLK:
>> +		config_fll_clk_12m(codec);
>> +		set_sys_clk(codec, clk_id);
> 
> Maybe just inline the contents of set_sys_clk here ...
> 
Since another code as below still uses set_sys_clk(), we reserve
function calling here.

	case SND_SOC_BIAS_OFF:
		dev_dbg(codec->dev, "###nau8825_set_bias_level OFF\n");
		set_sys_clk(codec, NAU8825_INTERNALCLOCK);
		regmap_update_bits(nau8825->regmap, NAU8825_BIAS_ADJ,
				NAU8825_VMID_MASK, NAU8825_VMID_DIS);

>> +		break;
>> +	case NAU8825_INTERNALCLOCK:
>> +		set_sys_clk(codec, clk_id);
> 
> ... and here
> 
The same answer to previous.

>> +		break;
>> +	default:
>> +		dev_err(codec->dev, "Wrong clock src\n");
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
> [...]
>  > +static const struct reg_default nau8825_reg[] = {
>  > +	/* enable clock source */
>  > +	{0x0001, 0x07FF},
>  > +	/* enable VMID and Bias */
>  > +	{0x0076, 0x3140},
>  > +	/* setup clock divider */
>  > +	{0x0003, 0x0050},
>  > +	/* jack detection configuration */
>  > +	{0x000C, 0x0004},
>  > +	{0x000D, 0x00E0},
>  > +	{0x000F, 0x0801},
>  > +	{0x0012, 0x0010},
>  > +	/* keypad detection configuration */
>  > +	{0x0013, 0x0280},
>  > +	{0x0014, 0x7310},
>  > +	{0x0015, 0x050E},
>  > +	{0x0016, 0x1B2A},
>  > +	/* audio format configuration */
>  > +	{0x001A, 0x0800},
>  > +	{0x001C, 0x000E},
>  > +	{0x001D, 0x0010},
>  > +	/* sampling rate control */
>  > +	{0x002B, 0x0012},
>  > +	/* DAC sampling rate control */
>  > +	{0x002C, 0x0082},
>  > +	/* ADC and DAC mixer control */
>  > +	{0x0030, 0x00D2},
>  > +	{0x0033, 0x00CF},
>  > +	{0x0034, 0x02CF},
>  > +	/* DAC class-G control */
>  > +	{0x006A, 0x1003},
>  > +	{0x0050, 0x2007},
>  > +	/* ADC PGA control */
>  > +	{0x0072, 0x0260},
>  > +	/* DAC power down enabled */
>  > +	{0x0080, 0x03A0},
>  > +	/* enable DAC clock */
>  > +	{0x0073, 0x336C},
>  > +	/* enable MIC Bias */
>  > +	{0x0074, 0x5502},
>  > +	/* powerup output driver */
>  > +	{0x007F, 0x473C},
>  > +	{0x007F, 0x473F},
>  > +	/* DAC power down disabled */
>  > +	{0x0080, 0x00A0},
>  > +	/* adjust MIC Bias */
>  > +	{0x0066, 0x2060},
>  > +	{0x0066, 0x2060},
>  > +	{0x0066, 0x0060},
>  > +};
> 
> The register defaults should hold the content of the registers in the 
> power-on reset state of the chip. This is not meant to be used as a 
> initialization sequence.
> 
Modified code to make register defaults hold the register values in the
power-on reset state.

static const struct reg_default nau8825_reg[] = {
	{0x000, 0x0000},
	{0x001, 0x00ff},
	{0x003, 0x0050},
	{0x004, 0x0000},
	{0x005, 0x3126},
	{0x006, 0x0008},
	{0x007, 0x0010},
	{0x008, 0x0000},
	{0x009, 0x6000},
	{0x00a, 0xf13c},
	{0x00c, 0x000c},
	{0x00d, 0x0000},
	{0x00f, 0x0800},
	{0x010, 0x0000},
	{0x011, 0x0000},
	{0x012, 0x0010},
	{0x013, 0x0015},
	{0x014, 0x0110},
	{0x015, 0x0000},
	...

>  > +
> [...]
>> +static struct snd_soc_dai_driver nau8825_dai_driver[] = {
>> +	{
>> +		.name = "nau8825-aif1",
>> +			.playback = {
>> +				.stream_name	 = "AIF1 Playback",
>> +				.channels_min	 = 1,
>> +				.channels_max	 = 2,
>> +				.rates		 = NAU8825_RATES,
>> +				.formats	 = NAU8825_FORMATS,
>> +			},
>> +			.capture = {
>> +				.stream_name	 = "AIF1 Capture",
>> +				.channels_min	 = 1,
>> +				.channels_max	 = 2,
>> +				.rates		 = NAU8825_RATES,
>> +				.formats	 = NAU8825_FORMATS,
>> +			},
> 
> The indent is a bit strange above.
> 
Fixed the code as below.

static struct snd_soc_dai_driver nau8825_dai_driver[] = {
	{
		.name = "nau8825-aif1",
		.playback = {
			.stream_name	 = "AIF1 Playback",
			.channels_min	 = 1,
			.channels_max	 = 2,
			.rates		 = NAU8825_RATES,
			.formats	 = NAU8825_FORMATS,
		},
		.capture = {
			.stream_name	 = "AIF1 Capture",
			.channels_min	 = 1,
			.channels_max	 = 2,
			.rates		 = NAU8825_RATES,
			.formats	 = NAU8825_FORMATS,
		},
		.ops = &nau8825_dai_ops,
	}
};

>> +		.ops = &nau8825_dai_ops,
>> +	}
>> +};
>> +
>> +
>> +static int nau8825_i2c_probe(struct i2c_client *i2c,
>> +				 const struct i2c_device_id *i2c_id)
>> +{
>> +	struct nau8825_priv *nau8825;
>> +	int i, ret;
>> +
>> +	nau8825 = devm_kzalloc(&i2c->dev, sizeof(*nau8825),
>> +		GFP_KERNEL);
>> +	if (nau8825 == NULL)
>> +		return -ENOMEM;
>> +	nau8825->i2c = i2c;
>> +	i2c_set_clientdata(i2c, nau8825);
>> +	nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap);
>> +	if (IS_ERR(nau8825->regmap)) {
>> +		ret = PTR_ERR(nau8825->regmap);
>> +		dev_err(&i2c->dev, "Failed to allocate regmap: %d\n", ret);
>> +		goto err_enable;
> 
> Since there is nothing to do at err_enable, just return ret directly and get 
> rid of the label.
> 
Modified the code as below.

	i2c_set_clientdata(i2c, nau8825);
	nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap);
	if (IS_ERR(nau8825->regmap)) {
		ret = PTR_ERR(nau8825->regmap);
		dev_err(&i2c->dev, "Failed to allocate regmap: %d\n", ret);
		return ret;
	}
	/* software reset */
	regmap_write(nau8825->regmap, NAU8825_RESET, 0x00);
	regmap_write(nau8825->regmap, NAU8825_RESET, 0x00);

	/* register sound card */
	ret = snd_soc_register_codec(&i2c->dev, &soc_codec_driver_nau8825,
				nau8825_dai_driver,
				ARRAY_SIZE(nau8825_dai_driver));
	return ret;
}

>> +	}
>> +	/* software reset */
>> +	regmap_write(nau8825->regmap, NAU8825_RESET, 0x01);
>> +	regmap_write(nau8825->regmap, NAU8825_RESET, 0x02);
>> +	/*writing initial register values to the codec*/
>> +	for (i = 0; i < ARRAY_SIZE(nau8825_reg); i++)
>> +		regmap_write(nau8825->regmap, nau8825_reg[i].reg,
>> +		nau8825_reg[i].def);
>> +
>> +	ret = snd_soc_register_codec(&i2c->dev, &soc_codec_driver_nau8825,
>> +				nau8825_dai_driver,
>> +				ARRAY_SIZE(nau8825_dai_driver));
>> +err_enable:
>> +	return ret;
>> +}
>> +
> [...]
>> +#define NAU8825_RATES	SNDRV_PCM_RATE_8000_192000
>> +#define NAU8825_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S20_3LE \
>> +			 | SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)
> 
> These defines should probably be moved to the C file close to their users.
>
Move the defines to C file as below.

#define NAU8825_RATES	SNDRV_PCM_RATE_8000_192000
#define NAU8825_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE |
SNDRV_PCM_FMTBIT_S20_3LE \
			 | SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S32_LE)

static const struct snd_soc_dai_ops nau8825_dai_ops = {
	.trigger	= nau8825_trigger,
	.hw_params	= nau8825_hw_params,
	.set_sysclk	= nau8825_dai_set_sysclk,
	.set_fmt	= nau8825_set_dai_fmt,
	.digital_mute	= nau8825_dac_mute,
};

static struct snd_soc_dai_driver nau8825_dai_driver[] = {
	{
		.name = "nau8825-aif1",

>> +
>> +struct nau8825_priv {
>> +	struct snd_soc_codec *codec;
>> +	struct regmap *regmap;
>> +	struct i2c_client *i2c;
>> +	struct snd_soc_jack *jack;
>> +	struct delayed_work jack_detect_work;
> 
> All fields in here except for the regmap field are still unused.
> 
Remove some unused field. But the i2c field is used in nau8825_i2c_probe().

struct nau8825_priv {
	struct regmap *regmap;
	struct i2c_client *i2c;
};

static int nau8825_i2c_probe(struct i2c_client *i2c,
				 const struct i2c_device_id *i2c_id)
{
	struct nau8825_priv *nau8825;
	int ret;

	nau8825 = devm_kzalloc(&i2c->dev, sizeof(*nau8825),
		GFP_KERNEL);
	if (nau8825 == NULL)
		return -ENOMEM;
	nau8825->i2c = i2c;
	i2c_set_clientdata(i2c, nau8825);
	nau8825->regmap = devm_regmap_init_i2c(i2c, &nau8825_regmap);

>> +};
>> +#endif	/* _NAU8825_H */
>>
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel