Re: [alsa-devel] [PATCH v4] ASoC: Add Freescale SGTL5000 codec support

zhaoming.zeng@freescale.com writes:

Hi,

[...]

+/*

• • set clock according to i2s frame clock,
• • sgtl5000 provide 2 clock sources.
• • 1. sys_mclk. sample freq can only configure to
• • 1/256, 1/384, 1/512 of sys_mclk.
• • 2. pll. can derive any audio clocks.
• • clock setting rules:
• • 1. in slave mode, only sys_mclk can use.
• • 2. as constraint by sys_mclk, sample freq should
• • set to 32k, 44.1k and above.
• • 3. using sys_mclk prefer to pll to save power.
• */

+static int sgtl5000_set_clock(struct snd_soc_codec *codec, int frame_rate) +{

• struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
• int clk_ctl = 0;
• int sys_fs; /* sample freq */
• /*

• * sample freq should be divided by frame clock,
  

• * if frame clock lower than 44.1khz, sample feq should set to
  

• * 32khz or 44.1khz.
  

• */
  

• switch (frame_rate) {
• case 8000:
• case 16000:

• sys_fs = 32000;
  

• break;
  

• case 11025:
• case 22050:

• sys_fs = 44100;
  

• break;
  

• default:

• sys_fs = frame_rate;
  

• break;
  

• }
• /* set devided factor of frame clock */
• switch (sys_fs / frame_rate) {
• case 4:

• clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;
  

• break;
  

• case 2:

• clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;
  

• break;
  

• case 1:

• clk_ctl |= SGTL5000_RATE_MODE_DIV_1 << SGTL5000_RATE_MODE_SHIFT;
  

• break;
  

• default:

• return -EINVAL;
  

• }
• /* set the sys_fs accroding to frame rate */
• switch (sys_fs) {
• case 32000:

• clk_ctl |= SGTL5000_SYS_FS_32k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• case 44100:

• clk_ctl |= SGTL5000_SYS_FS_44_1k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• case 48000:

• clk_ctl |= SGTL5000_SYS_FS_48k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• case 96000:

• clk_ctl |= SGTL5000_SYS_FS_96k << SGTL5000_SYS_FS_SHIFT;
  

• break;
  

• default:

• dev_err(codec->dev, "frame rate %d not supported\n",
  

• 	frame_rate);
  

• return -EINVAL;
  

• }
• /*

• * calculate the divider of mclk/sample_freq,
  

• * factor of freq =96k can only be 256, since mclk in range (12m,27m)
  

• */
  

• switch (sgtl5000->sysclk / sys_fs) {
• case 256:

• clk_ctl |= SGTL5000_MCLK_FREQ_256FS <<
  

• 	SGTL5000_MCLK_FREQ_SHIFT;
  

• break;
  

• case 384:

• clk_ctl |= SGTL5000_MCLK_FREQ_384FS <<
  

• 	SGTL5000_MCLK_FREQ_SHIFT;
  

• break;
  

• case 512:

• clk_ctl |= SGTL5000_MCLK_FREQ_512FS <<
  

• 	SGTL5000_MCLK_FREQ_SHIFT;
  

• break;
  

• default:

• /* if mclk not satisify the divider, use pll */
  

• if (sgtl5000->master)
  

• 	clk_ctl |= SGTL5000_MCLK_FREQ_PLL <<
  

• 		SGTL5000_MCLK_FREQ_SHIFT;
  

• else {
  

• 	pr_err("%s: PLL not supported in slave mode\n",
  

• 		__func__);
  

• 	return -EINVAL;
  

• }
  

• }
• /* if using pll, please check manual 6.4.2 for detail */
• if ((clk_ctl & SGTL5000_MCLK_FREQ_MASK) == SGTL5000_MCLK_FREQ_PLL) {

• u64 out, t;
  

• int div2;
  

• int pll_ctl;
  

• unsigned int in, int_div, frac_div;
  

btw, I don't know for other developers, but I don't like variables declared like this.

• if (sgtl5000->sysclk > 17000000) {
  

• 	div2 = 1;
  

• 	in = sgtl5000->sysclk / 2;
  

• } else {
  

• 	div2 = 0;
  

• 	in = sgtl5000->sysclk;
  

• }
  

• if (sys_fs == 44100)
  

• 	out = 180633600;
  

• else
  

• 	out = 196608000;
  

• t = do_div(out, in);
  

• int_div = out;
  

• t *= 2048;
  

• do_div(t, in);
  

• frac_div = t;
  

• pll_ctl = int_div << SGTL5000_PLL_INT_DIV_SHIFT |
  

•     frac_div << SGTL5000_PLL_FRAC_DIV_SHIFT;
  

• snd_soc_write(codec, SGTL5000_CHIP_PLL_CTRL, pll_ctl);
  

• if (div2)
  

• 	snd_soc_update_bits(codec,
  

• 		SGTL5000_CHIP_CLK_TOP_CTRL,
  

• 		SGTL5000_INPUT_FREQ_DIV2,
  

• 		SGTL5000_INPUT_FREQ_DIV2);
  

• else
  

• 	snd_soc_update_bits(codec,
  

• 		SGTL5000_CHIP_CLK_TOP_CTRL,
  

• 		SGTL5000_INPUT_FREQ_DIV2,
  

• 		0);
  

• /* power up pll */
  

• snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
  

• 	SGTL5000_PLL_POWERUP | SGTL5000_VCOAMP_POWERUP,
  

• 	SGTL5000_PLL_POWERUP | SGTL5000_VCOAMP_POWERUP);
  

• } else {

• /* power down pll */
  

• snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
  

• 	SGTL5000_PLL_POWERUP | SGTL5000_VCOAMP_POWERUP,
  

• 	0);
  

• }
• /* if using pll, clk_ctrl must be set after pll power up */
• snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL, clk_ctl);
• return 0;

+}

[...]

+static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) +{

• u16 reg;
• int ret;
• int rev, i;
• int external_vddd = 0;
• struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
• for (i = 0; i < ARRAY_SIZE(sgtl5000->supplies); i++)

• sgtl5000->supplies[i].supply = supply_names[i];
  

• ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),

• 		sgtl5000->supplies);
  

so here, you're calling '_get' for vdda, vddd, vddio.

• if (!ret)

• external_vddd = 1;
  

• else {

• /* set internal ldo to 1.2v */
  

• int voltage = LDO_VOLTAGE;
  

• sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
  

• ret = ldo_regulator_register(codec, &ldo_init_data, voltage);
  

• if (ret) {
  

• 	dev_err(codec->dev,
  

• 	"Failed to register vddd internal supplies: %d\n",
  

• 		ret);
  

• 	return ret;
  

• }
  

• ret = regulator_bulk_get(codec->dev,
  

• 		ARRAY_SIZE(sgtl5000->supplies),
  

• 		sgtl5000->supplies);
  

• if (ret) {
  

• 	ldo_regulator_remove(codec);
  

• 	dev_err(codec->dev,
  

• 		"Failed to request supplies: %d\n", ret);
  

• 	return ret;
  

• }
  

• }
• ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),

• 			sgtl5000->supplies);
  

• if (ret)

• goto err_regulator_free;
  

• /* wait for all power rails bring up */
• udelay(10);
• /* read chip information */
• reg = snd_soc_read(codec, SGTL5000_CHIP_ID);
• if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) !=

•    SGTL5000_PARTID_PART_ID) {
  

• dev_err(codec->dev,
  

• 	"Device with ID register %x is not a sgtl5000\n", reg);
  

• ret = -ENODEV;
  

• goto err_regulator_disable;
  

• }
• rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT;
• dev_info(codec->dev, "sgtl5000 revision %d\n", rev);
• /*

• * workaround for revision 0x11 and later,
  

• * roll back to use internal LDO
  

• */
  

• if (external_vddd && rev >= 0x11) {

• int voltage = LDO_VOLTAGE;
  

• /* disable all regulator first */
  

• regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
  

• 			sgtl5000->supplies);
  

• /* free VDDD regulator */
  

• regulator_put(sgtl5000->supplies[VDDD].consumer);
  

so you call put for vddd only and in regulator_put there's a line doing kfree(....consumer) so now sgtl5000->supplies[VDDD].consumer is null.

• sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME;
  

• ret = ldo_regulator_register(codec, &ldo_init_data, voltage);
  

• if (ret)
  

• 	return ret;
  

• ret = regulator_bulk_get(codec->dev,
  

• 		ARRAY_SIZE(sgtl5000->supplies),
  

• 		sgtl5000->supplies);
  

and here you're calling 'get' for vdda, vddio and vddd_lo. This means that in this code path, you're requesting 2 times vdda/vddio. It's a bug. You should call only regulator_get for vddd_lo and set sgtl5000->supplies[VDDD].consumer in case of success. If you don't set it, calling regulator_bulk_enable will result in a oops as .consumer is null.

• if (ret) {
  

• 	ldo_regulator_remove(codec);
  

• 	dev_err(codec->dev,
  

• 		"Failed to request supplies: %d\n", ret);
  

• 	return ret;
  

• }
  

• ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies),
  

• 				sgtl5000->supplies);
  

• if (ret)
  

• 	goto err_regulator_free;
  

• }
• return 0;

+err_regulator_disable:

• regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),

• 		sgtl5000->supplies);
  

+err_regulator_free:

• regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies),

• 		sgtl5000->supplies);
  

• if (external_vddd)

• ldo_regulator_remove(codec);
  

• return ret;

+} +static int sgtl5000_probe(struct snd_soc_codec *codec) +{

• int ret;
• /* setup i2c data ops */
• ret = snd_soc_codec_set_cache_io(codec, 16, 16, SND_SOC_I2C);
• if (ret < 0) {

• dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
  

• return ret;
  

• }
• ret = sgtl5000_enable_regulators(codec);
• if (ret)

• return ret;
  

• /* power up sgtl5000 */
• ret = sgtl5000_set_power_regs(codec);
• if (ret)

• return ret;
  

• /* enable small pop, introduce 400ms delay in turning off */
• snd_soc_update_bits(codec, SGTL5000_CHIP_REF_CTRL,

• 		SGTL5000_SMALL_POP,
  

• 		SGTL5000_SMALL_POP);
  

• /* disable short cut detector */
• snd_soc_write(codec, SGTL5000_CHIP_SHORT_CTRL, 0);
• /*

• * set i2s as default input of sound switch
  

• * TODO: add sound switch to control and dapm widge.
  

• */
  

• snd_soc_write(codec, SGTL5000_CHIP_SSS_CTRL,

• 	SGTL5000_DAC_SEL_I2S_IN << SGTL5000_DAC_SEL_SHIFT);
  

• snd_soc_write(codec, SGTL5000_CHIP_DIG_POWER,

• 	SGTL5000_ADC_EN | SGTL5000_DAC_EN);
  

• /* enable dac volume ramp by default */
• snd_soc_write(codec, SGTL5000_CHIP_ADCDAC_CTRL,

• 	SGTL5000_DAC_VOL_RAMP_EN |
  

• 	SGTL5000_DAC_MUTE_RIGHT |
  

• 	SGTL5000_DAC_MUTE_LEFT);
  

• snd_soc_write(codec, SGTL5000_CHIP_PAD_STRENGTH, 0x015f);
• snd_soc_write(codec, SGTL5000_CHIP_ANA_CTRL,

• 	SGTL5000_HP_ZCD_EN |
  

• 	SGTL5000_ADC_ZCD_EN);
  

• snd_soc_write(codec, SGTL5000_CHIP_MIC_CTRL, 0);
• /*

• * disable DAP
  

• * TODO:
  

• * Enable DAP in kcontrol and dapm.
  

• */
  

• snd_soc_write(codec, SGTL5000_DAP_CTRL, 0);
• /* leading to standby state */
• ret = sgtl5000_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
• if (ret)

• return ret;
  

• snd_soc_add_controls(codec, sgtl5000_snd_controls,

• 	     ARRAY_SIZE(sgtl5000_snd_controls));
  

• snd_soc_dapm_new_controls(&codec->dapm, sgtl5000_dapm_widgets,

• 		  ARRAY_SIZE(sgtl5000_dapm_widgets));
  

• snd_soc_dapm_add_routes(&codec->dapm, audio_map,

• 		ARRAY_SIZE(audio_map));
  

• snd_soc_dapm_new_widgets(&codec->dapm);
• return 0;

+}

+static int sgtl5000_remove(struct snd_soc_codec *codec) +{

• struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
• sgtl5000_set_bias_level(codec, SND_SOC_BIAS_OFF);
• snd_soc_dapm_free(&codec->dapm);

someone will have to check but iirc, the core is calling snd_soc_dapm_free() for you after calling the remove() hook.

Arnaud