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:
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.
@@ -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).
- /* 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).
- 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.
- ret = regulator_bulk_enable(sgtl5000->num_supplies,
sgtl5000->supplies);- if (!ret)
usleep_range(10, 20);
This is a separate change...
- else
regulator_bulk_free(sgtl5000->num_supplies,sgtl5000->supplies);
Convert to using devm_ since you're doing all this stuff.
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?
- /* 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.