[alsa-devel] [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.
The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An optional third external power supply VDDD may be provided externally to achieve lower power.If an external supply is not used for VDDD, the SGTL5000 driver will register it's own regulator device, and then provides the VDDD supply consumer, and now there will be two regulator devices exist, local regulator for VDDD and platform regulator for VDDIO, VDDA.
******************* * *---------|3.3V VDDIO * * * SGTL5000 codec *---x VDDD * * * *---------|3.3V VDDA *******************
If an external supply is not used for VDDD, in the DT or architecture-specific file, only "VDDA-supply" and "VDDIO-supply" properties will be presented. This caused the following kernel failed while trying to get the external VDDD supply before trying to register it's own regulator device.
sgtl5000 0-000a: Failed to get supply 'VDDD': -19
Here use regulator_get_optional() trying to look at the fact that whether the external VDDD supply is used or not.
Signed-off-by: Xiubo Li Li.Xiubo@freescale.com --- sound/soc/codecs/sgtl5000.c | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 1f4093f..90d6d1e 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1298,6 +1298,21 @@ static int sgtl5000_replace_vddd_with_ldo(struct snd_soc_codec *codec) return 0; }
+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_optional(codec->dev, + sgtl5000->supplies[VDDD].supply); + if (IS_ERR(consumer)) + return 0; + + regulator_put(consumer); + + return 1; +} + static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) { int reg; @@ -1310,11 +1325,15 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) for (i = 0; i < ARRAY_SIZE(sgtl5000->supplies); i++) sgtl5000->supplies[i].supply = supply_names[i];
- ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies), + if (sgtl5000_external_vddd_used(codec)) { + ret = regulator_bulk_get(codec->dev, + ARRAY_SIZE(sgtl5000->supplies), sgtl5000->supplies); - if (!ret) + if (ret) + return ret; + external_vddd = 1; - else { + } else { ret = sgtl5000_replace_vddd_with_ldo(codec); if (ret) return ret;
On Thu, Nov 28, 2013 at 02:46:59PM +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_optional(codec->dev,
sgtl5000->supplies[VDDD].supply);- if (IS_ERR(consumer))
return 0;- regulator_put(consumer);
- return 1;
+}
This is broken with respect to deferred probe, deferred probe returns an error when an external regulator is in use but not yet registered. The driver needs to at least handle -EPROBE_DEFER specially here.
It's also not nice to get the regulator, release it and get it again which you end up needing to do because...
- ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
- if (sgtl5000_external_vddd_used(codec)) {
ret = regulator_bulk_get(codec->dev,ARRAY_SIZE(sgtl5000->supplies), sgtl5000->supplies);
...I think my previous comments about this code being poorly structured in the existing driver stand. The bulk get should be used for the supplies that are always needed and a separate optional get should be used for this one.
+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_optional(codec->dev,
sgtl5000->supplies[VDDD].supply);- if (IS_ERR(consumer))
return 0;- regulator_put(consumer);
- return 1;
+}
This is broken with respect to deferred probe, deferred probe returns an error when an external regulator is in use but not yet registered. The driver needs to at least handle -EPROBE_DEFER specially here.
Well, as we can see that: 1, If the dev has no regulator dt node, a -ENODEV error will be returned. 2, If the regulator dt node is exist but the optional VDDD is absent (i.e. The external VDDD is not used), a -EPROBE_DEFER will be returned, if just return the -EPROBE_DEFER to the probe(and then the probe deferral mechanism will do the probe again later, is that right ?), and then the regulator_get_optional() will be called later again, and the -EPROBE_DEFER will be returned again too, and now how should I handle -EPROBE_DEFER error twice ? Or should there be a counter about this ? That to say when the -EPROBE_DEFER error is the second time returned from regulator_get_optional() can we ensure that the optional VDDD is really not in use.
That's also to say, the regulator_get_optional() must be called twice then could we know whether the optional external VDDD is really in use or not by using one counter here.
Or maybe adding a counter in regulator_get_optional() is better.
And do you have any other suggestions to deal with this ?
It's also not nice to get the regulator, release it and get it again which you end up needing to do because...
I have also considered about this. Because if the regulator_bulk_get() has failed to get the other supplies, the optional one should be released too. And it can be more easy to deal with.
Yes, this is not very nice and I will revise this.
- ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
- if (sgtl5000_external_vddd_used(codec)) {
ret = regulator_bulk_get(codec->dev,ARRAY_SIZE(sgtl5000->supplies), sgtl5000->supplies);...I think my previous comments about this code being poorly structured in the existing driver stand. The bulk get should be used for the supplies that are always needed and a separate optional get should be used for this one.
Yes, it's.
-- Best Regards,
On Tue, Dec 03, 2013 at 09:49:47AM +0000, Li Xiubo wrote:
2, If the regulator dt node is exist but the optional VDDD is absent (i.e. The external VDDD is not used), a -EPROBE_DEFER will be returned, if just return the -EPROBE_DEFER to the probe(and then the probe deferral mechanism will do the probe again later, is that right ?), and then the regulator_get_optional() will be called later again, and the -EPROBE_DEFER will be returned again too, and now how should I handle -EPROBE_DEFER error twice ? Or should there be a counter about this ? That to say when the -EPROBE_DEFER error is the second time returned from regulator_get_optional() can we ensure that the optional VDDD is really not in use.
The driver should just defer when it's told to defer, I don't understand why it would want to count anything?
2, If the regulator dt node is exist but the optional VDDD is absent (i.e. The external VDDD is not used), a -EPROBE_DEFER will be returned, if just return the -EPROBE_DEFER to the probe(and then the probe deferral mechanism will do the probe again later, is that right ?), and then the regulator_get_optional() will be called later again, and the -EPROBE_DEFER will be returned again too, and now how should I handle -EPROBE_DEFER error twice ? Or should there be a counter about this ? That to say when the -EPROBE_DEFER error is the second time returned from regulator_get_optional() can we ensure that the optional VDDD is really
not in use.
The driver should just defer when it's told to defer, I don't understand why it would want to count anything?
It's just one idea for the special handling of regulator_get_optional() in this case.
-- Xiubo
participants (4)
-
Li Xiubo -
Li.Xiubo＠freescale.com
-
Mark Brown -
Xiubo Li