[alsa-devel] [RFC PATCH 1/6] ASoC: sgtl5000: fix regulator support

[Mark Brown]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150306/7a4a9d5f/attachment.sig>