[PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.

[PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.

[Xiubo Li]

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;
-- 
1.8.4

Re: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.

[Mark Brown]

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

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.

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

RE: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.

[Li Xiubo]

> > +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,

Re: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.

[Mark Brown]

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

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?

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

RE: [PATCH v2] ASoC: SGTL5000: Fix kernel failed while trying to get optional VDDD supply.

[Li.Xiubo]

> > 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