-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi Mark,
Thanks for the review.
On 03/06/2015 01:04 PM, Mark Brown wrote:
On Thu, Feb 26, 2015 at 03:54:28PM -0700, Eric Nelson wrote:
Since the internal LDO can only be enabled after I2C is available, there's no benefit in treating it as a regulator, so this patch removes the regulator structure surrounding it.
The expected benefit would be that it is possible to then have the code that uses the regulator be constant:
That's a nice plan, but doesn't fit, since the internal regulator requires I2C access and can only be used after the rest of the power-up sequence has completed.
case SND_SOC_BIAS_STANDBY: if (codec->dapm.bias_level == SND_SOC_BIAS_OFF) { ret = regulator_bulk_enable( - ARRAY_SIZE(sgtl5000->supplies), + sgtl5000->num_supplies, sgtl5000->supplies);
so we avoid stuff like this.
I understand the intent, but that doesn't work. If the internal LDO is wrapped in a regulator and placed here, the sequence needs to be: enable VDDIO and VDDA regulators re-enable the clock wait 8 cycles enable the LDO for VDDD
@@ -1115,7 +938,9 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec)
vdda = regulator_get_voltage(sgtl5000->supplies[VDDA].consumer); vddio = regulator_get_voltage(sgtl5000->supplies[VDDIO].consumer); - vddd = regulator_get_voltage(sgtl5000->supplies[VDDD].consumer); + vddd = (sgtl5000->num_supplies > VDDD) + ? regulator_get_voltage(sgtl5000->supplies[VDDD].consumer) + : LDO_VOLTAGE;
Please write a normal if statement, this is not legible (and the test is more than a little obscure).
Will do (in a V2).
- /* External VDDD only works before revision 0x11 */ - if
(sgtl5000->revision < 0x11) { - vddd = regulator_get_optional(codec->dev, "VDDD");
It'd be good to keep at least a warning about this (not that there's one now but it's a good idea).
I haven't been able to find the origin of this test, but it's in conflict with ER1.
- ret = regulator_bulk_get(codec->dev,
ARRAY_SIZE(sgtl5000->supplies), + sgtl5000->num_supplies = ARRAY_SIZE(sgtl5000->supplies) + - 1 + external_vddd;
This is a bit obscure, why not just do this sooner (with a comment for the - 1) and increment num_supplies when we add the external regulator. A comment on the supply list saying that VDDD must be last would be good too.
Agreed. I'll rework it.
- ret = regulator_bulk_enable(sgtl5000->num_supplies, +
sgtl5000->supplies); + if (!ret) + usleep_range(10, 20);
This is a separate change...
I changed udelay() to usleep_range() in order to keep checkpatch happy.
- else + regulator_bulk_free(sgtl5000->num_supplies, +
sgtl5000->supplies);
Convert to using devm_ since you're doing all this stuff.
Will do.
ret = clk_prepare_enable(sgtl5000->mclk); - if (ret) - return ret; + if (ret) { + dev_err(&client->dev, "Error enabling clock %d\n", ret); + goto disable_regs; + }
This is a separate fix as far as I can tell?
Not really. Once regulators are moved into the I2C device, we can't just return because that would leave the regulators enabled.
- /* Follow section 2.2.1.1 of AN3663 */ + if
(sgtl5000->num_supplies <= VDDD) { + /* internal VDDD at 1.2V */
These checks are just far too obscure, please set a flag or something.
Got it. I'll fix this in V2 (not RFC).
Regards,
Eric