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.