Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1654 bytes --]

On Tue, Nov 26, 2013 at 05:55:13PM +0800, Xiubo Li wrote:

> +static int sgtl5000_external_vddd_used(struct snd_soc_codec *codec)
> +{
> +	struct regulator *consumer;
> +	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> +
> +	consumer = regulator_get(codec->dev, sgtl5000->supplies[VDDD].supply);
> +	if (IS_ERR(consumer)) {
> +		return 0;
> +	}
> +	regulator_put(consumer);
> +
> +	return 1;
> +}

This is going to fail to do what you expect if a dummy regulator is
substituted which it will almost all of the time when the supply is
genuinely absent.  It's also going to interact poorly with probe
deferral.

> -	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
> -				sgtl5000->supplies);
> -	if (!ret)
> +	if (sgtl5000_external_vddd_used(codec))
>  		external_vddd = 1;
>  	else {
>  		ret = sgtl5000_replace_vddd_with_ldo(codec);

This looks buggy, it removes the regulator_get() for most of the
supplies for the device.

I think the driver may want to use regulator_get_optional(), though
looking at the fact that there's an external_vdd flag there already it
seems like there's already some attempt to handle these configuratins in
the driver that isn't working so perhaps that just needs to be fixed.
Or possibly DTs need to be changed to describe the supply.  

I'd expect to see a commit description that describes how the driver
currently tries to handle this, why it doesn't work and how the patch
fixes it.  Right now it seems like the change is jumping to a solution
without understanding the problem and making some already problematic
code more confused.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]