On Wed 2010-12-08 05:11:55, Mark Brown wrote:
> On Wed, Dec 08, 2010 at 10:06:29AM +0800, zhaoming.zeng@xxxxxxxxxxxxx
> wrote:
> > From: Zeng Zhaoming <zhaoming.zeng@xxxxxxxxxxxxx>
>
> 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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
