RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
> Subject: Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting > regulator consumers > > On Wed, Nov 27, 2013 at 08:13:03AM +, Li Xiubo wrote: > > Please fix your mailer to word wrap within paragraphs, it makes your mail > much more legible. > Yes, I will. > > There is one dependency patch: "regulator: core: Provide a dummy > regulator with full constraints". > > > From the dependency patch, we can see that using regulator_get_optional() > instead can resovle the problem you descripted above. > > > When or will this dependency patch be merged into the -next tree ? > > That patch is already in mainline. > Okey. > > > 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. > > > The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An > > optional third external power supply VDDD may be provided externally > > You need to put your working through of this stuff in the commit message. I did it already, please see the next version. -- Best Regards, -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
On Wed, Nov 27, 2013 at 08:13:03AM +, Li Xiubo wrote: Please fix your mailer to word wrap within paragraphs, it makes your mail much more legible. > There is one dependency patch: "regulator: core: Provide a dummy regulator > with full constraints". > From the dependency patch, we can see that using regulator_get_optional() > instead can resovle the problem you descripted above. > When or will this dependency patch be merged into the -next tree ? That patch is already in mainline. > > 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. > The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An > optional third external power supply VDDD may be provided externally You need to put your working through of this stuff in the commit message. signature.asc Description: Digital signature
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
> > +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.
>
There is one dependency patch: "regulator: core: Provide a dummy regulator with
full constraints".
>From the dependency patch, we can see that using regulator_get_optional()
>instead can resovle the problem you descripted above.
When or will this dependency patch be merged into the -next tree ?
> 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.
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 non-DT regulator device, and then provides the VDDD supply
consumer, and now there will be two register devices exist, non-DT regulator
for VDDD and DT 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 file, only "VDDA-supply"
and "VDDIO-supply" properties will be presented. This caused the following
kernel failed while trying to get the external VDDD regulator consumer before
trying to register it's own regulator device.
sgtl5000 0-000a: Failed to get supply 'VDDD': -19
While if an external supply is used for VDDD, in the DT file, all the three
"VDDA-supply", "VDDIO-supply" and "VDDD-supply" properties can absent, by just
using the dummy regulator.
IMO, the failed log will confuse most people(especially for those not familiar
with the SGTL5000 driver) that why getting the 'VDDD' supply failed but it
still works? or is there something wrong with it ? ,etc.
Yes, this is a buggy patch, and I will revise it later.
--
Best Regards,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
+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.
There is one dependency patch: regulator: core: Provide a dummy regulator with
full constraints.
From the dependency patch, we can see that using regulator_get_optional()
instead can resovle the problem you descripted above.
When or will this dependency patch be merged into the -next tree ?
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.
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 non-DT regulator device, and then provides the VDDD supply
consumer, and now there will be two register devices exist, non-DT regulator
for VDDD and DT 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 file, only VDDA-supply
and VDDIO-supply properties will be presented. This caused the following
kernel failed while trying to get the external VDDD regulator consumer before
trying to register it's own regulator device.
sgtl5000 0-000a: Failed to get supply 'VDDD': -19
While if an external supply is used for VDDD, in the DT file, all the three
VDDA-supply, VDDIO-supply and VDDD-supply properties can absent, by just
using the dummy regulator.
IMO, the failed log will confuse most people(especially for those not familiar
with the SGTL5000 driver) that why getting the 'VDDD' supply failed but it
still works? or is there something wrong with it ? ,etc.
Yes, this is a buggy patch, and I will revise it later.
--
Best Regards,
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
On Wed, Nov 27, 2013 at 08:13:03AM +, Li Xiubo wrote: Please fix your mailer to word wrap within paragraphs, it makes your mail much more legible. There is one dependency patch: regulator: core: Provide a dummy regulator with full constraints. From the dependency patch, we can see that using regulator_get_optional() instead can resovle the problem you descripted above. When or will this dependency patch be merged into the -next tree ? That patch is already in mainline. 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. The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An optional third external power supply VDDD may be provided externally You need to put your working through of this stuff in the commit message. signature.asc Description: Digital signature
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
Subject: Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers On Wed, Nov 27, 2013 at 08:13:03AM +, Li Xiubo wrote: Please fix your mailer to word wrap within paragraphs, it makes your mail much more legible. Yes, I will. There is one dependency patch: regulator: core: Provide a dummy regulator with full constraints. From the dependency patch, we can see that using regulator_get_optional() instead can resovle the problem you descripted above. When or will this dependency patch be merged into the -next tree ? That patch is already in mainline. Okey. 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. The SGTL5000 requires 2 external power supplies: VDDA and VDDIO. An optional third external power supply VDDD may be provided externally You need to put your working through of this stuff in the commit message. I did it already, please see the next version. -- Best Regards, -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
> > else {
> > ret = sgtl5000_replace_vddd_with_ldo(codec);
> >
> You could fix the coding style issue (braces on both branches of the if
> clause) here too.
>
>
Yes, I will.
Thanks.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
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.
signature.asc
Description: Digital signature
Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
Hi,
Xiubo Li wrote:
> SGTL5000 codec allows to use the internal LDO instead of VDDD, if the
> internal LDO is used, this caused the following kernel failed while trying
> to get the external VDDD regulator consumer.
>
> Adding sgtl5000_external_vddd_used() to fix it. And this has been tested on
> VF610-TWR board.
>
> sgtl5000 0-000a: Failed to get supply 'VDDD': -19
>
> Signed-off-by: Xiubo Li
> ---
>
> sound/soc/codecs/sgtl5000.c | 18 +++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index 1f4093f..1a2d35a 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -1298,6 +1298,20 @@ 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(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,9 +1324,7 @@ 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),
> - sgtl5000->supplies);
> - if (!ret)
> + if (sgtl5000_external_vddd_used(codec))
> external_vddd = 1;
> else {
> ret = sgtl5000_replace_vddd_with_ldo(codec);
>
You could fix the coding style issue (braces on both branches
of the if clause) here too.
Lothar Waßmann
--
___
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | i...@karo-electronics.de
___
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
Hi,
Xiubo Li wrote:
SGTL5000 codec allows to use the internal LDO instead of VDDD, if the
internal LDO is used, this caused the following kernel failed while trying
to get the external VDDD regulator consumer.
Adding sgtl5000_external_vddd_used() to fix it. And this has been tested on
VF610-TWR board.
sgtl5000 0-000a: Failed to get supply 'VDDD': -19
Signed-off-by: Xiubo Li li.xi...@freescale.com
---
sound/soc/codecs/sgtl5000.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 1f4093f..1a2d35a 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1298,6 +1298,20 @@ 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(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,9 +1324,7 @@ 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),
- sgtl5000-supplies);
- if (!ret)
+ if (sgtl5000_external_vddd_used(codec))
external_vddd = 1;
else {
ret = sgtl5000_replace_vddd_with_ldo(codec);
You could fix the coding style issue (braces on both branches
of the if clause) here too.
Lothar Waßmann
--
___
Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996
www.karo-electronics.de | i...@karo-electronics.de
___
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
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.
signature.asc
Description: Digital signature
RE: [PATCHv1] ASoC: SGTL5000: Fix kernel failed while getting regulator consumers
else {
ret = sgtl5000_replace_vddd_with_ldo(codec);
You could fix the coding style issue (braces on both branches of the if
clause) here too.
Yes, I will.
Thanks.
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf��^jǫy�m��@A�a���
0��h���i
