On Wed 2011-02-23 08:47:48, Arnaud Patard wrote:
zhaoming.zeng@freescale.com writes:
Hi,
[...]
[...]
- /* 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;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.
sorry, my test case not cover this code. what about regulator_bulk_free() instead of regulator_put(). This aligns to regulator bulk way.
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.
You are right, I'm not notice this changes. It is better to let ASoC code to free it.
Arnaud
Thanks.