On Wed 2010-12-08 05:11:55, Mark Brown wrote:
On Wed, Dec 08, 2010 at 10:06:29AM +0800, zhaoming.zeng@freescale.com wrote:
From: Zeng Zhaoming zhaoming.zeng@freescale.com
Broadly speaking this is OK - there's quite a few comments below but they're mostly fairly small and local things rather than major structural issues. The one thing that needs real work is the power management which is fairly non standard and seems to follow a mix of hard coding and DAPM. A lot of the issues here seem to be due to some rather nasty hardware issues so there's likely to be limits on how nice the software can be.
For the subject line, please use a prefix such as "ASoC: " like other patches do - in general check the changelogs for the area you're submitting against for things like this.
hi, Mark, thanks for so carefully review, and so many suggestion.
config SND_SOC_WM9090 tristate
+config SND_SOC_SGTL5000
- tristate
Keep both Kconfig and Makefile sorted, and remember to add your CODEC to SND_SOC_ALL_CODECS.
- struct regulator *supplies[SGTL5000_SUPPLY_NUM];
- int regulator_volt[SGTL5000_SUPPLY_NUM];
- struct regulator *reg_vddio;
- struct regulator *reg_vdda;
- struct regulator *reg_vddd;
- int vddio; /* voltage of VDDIO (mv) */
- int vdda; /* voltage of vdda (mv) */
- int vddd; /* voltage of vddd (mv), 0 if not
connected */
You appear to be keeping two copies of the regulators and their voltages. TBH I'm not clear why you're caching the voltages - you may as well just read them when you need them, you only refer to them at probe time anyway.
Sorry, I forget to clean up this code.
+static int sgtl5000_pcm_hw_params(struct snd_pcm_substream
*substream,
struct snd_pcm_hw_params *params,struct snd_soc_dai *dai)+{
- switch (sgtl5000->lrclk) {
- case 8000:
- case 16000:
sys_fs = 32000;break;- case 11025:
- case 22050:
sys_fs = 44100;break;- default:
sys_fs = sgtl5000->lrclk;break;A comment explaining what's going on here would be helpful - it looks like the device is running internally at 32kHz or higher and downsampling somehow to present lower output frequencies?
Right, we need to downsampling for low frequencies.
pop/clicks */
- sgtl5000_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
- if (codec->suspend_bias_level == SND_SOC_BIAS_ON)
sgtl5000_set_bias_level(codec, SND_SOC_BIAS_PREPARE);- sgtl5000_set_bias_level(codec, codec->suspend_bias_level);
The CODEC will never be in any state above STANDBY at suspend.
Is there any documentation describe the state machine?
- for (i = 0; i < SGTL5000_SUPPLY_NUM; i++) {
struct regulator *reg;reg = regulator_get(&client->dev, supply_names[i]);if (IS_ERR(reg))continue;sgtl5000->regulator_volt[i] = regulator_get_voltage(reg)/ 1000;
regulator_enable(reg);sgtl5000->supplies[i] = reg;- }
- sgtl5000->regulator_volt[VDDA] = 3300;
- sgtl5000->regulator_volt[VDDIO] = 3300;
Sorry again, forget to cleanup.
- msleep(1);
This delay looks like it's in the wrong place?
The SGTL5000 has an internal reset that is deasserted 8 SYS_MCLK cycles after all power rails have been brought up. After this time communication can start.
So I think delay 1ms is safe for the next operations.