| Message ID | 1386916983-27832-2-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shawn, > - ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies), > - sgtl5000->supplies); > - if (!ret) > - external_vddd = 1; > - else { > + /* External VDDD only works before revision 0x11 */ > + if (sgtl5000->revision < 0x11) { > + vddd = regulator_get_optional(codec->dev, "VDDD"); > + if (IS_ERR(vddd)) { > + /* See if it's just not registered yet */ > + if (PTR_ERR(vddd) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + } else { > + external_vddd = 1; > + regulator_put(vddd); > + } Shouldn't we handle other errors ? We can see that from the regulator_get_optional()-->_regulator_get(), there are many other errors maybe returned. Not considering other error cases, If the dt is used here, only ERR_PTR(-ENODEV) will be returned if the external VDDD is absent, and only this case the ERR_PTR(_ENODEV) error will be returned, Or if dt is not used and there has no external VDDD was found, the ERR_PTR(-EPROBE_DEFER) will be returned. So, if the external VDDD is not used, only ERR_PTR(-ENODEV) and ERR_PTR(-EPROBE_DEFER) maybe returned ignoring other errors. How about the following ? ----------------- + if (IS_ERR(vddd)) { + /* See if it's just not registered yet */ + if (PTR_ERR(vddd) != -ENODEV) + return PTR_ERR(vddd); + } else { + external_vddd = 1; + regulator_put(vddd); + } ----------------- -- Xiubo
On Fri, Dec 13, 2013 at 10:29:43AM +0000, Li.Xiubo@freescale.com wrote: > Hi Shawn, > > > - ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies), > > - sgtl5000->supplies); > > - if (!ret) > > - external_vddd = 1; > > - else { > > + /* External VDDD only works before revision 0x11 */ > > + if (sgtl5000->revision < 0x11) { > > + vddd = regulator_get_optional(codec->dev, "VDDD"); > > + if (IS_ERR(vddd)) { > > + /* See if it's just not registered yet */ > > + if (PTR_ERR(vddd) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + } else { > > + external_vddd = 1; > > + regulator_put(vddd); > > + } > > Shouldn't we handle other errors ? We do not really care other errors. In case that an external VDDD is present on hardware design but regulator_get_optional("VDDD") still fails for some other reason, we should turn to use the internal LDO anyway rather than simply failing out. > We can see that from the regulator_get_optional()-->_regulator_get(), there > are many other errors maybe returned. Not considering other error cases, If > the dt is used here, only ERR_PTR(-ENODEV) will be returned if the external > VDDD is absent, and only this case the ERR_PTR(_ENODEV) error will be returned, > Or if dt is not used and there has no external VDDD was found, the > ERR_PTR(-EPROBE_DEFER) will be returned. > > So, if the external VDDD is not used, only ERR_PTR(-ENODEV) and ERR_PTR(-EPROBE_DEFER) > maybe returned ignoring other errors. > > How about the following ? > ----------------- > + if (IS_ERR(vddd)) { > + /* See if it's just not registered yet */ > + if (PTR_ERR(vddd) != -ENODEV) > + return PTR_ERR(vddd); Just for example, if PTR_ERR(vddd) is -EBUSY here, instead of failing out, why do not we switch to internal LDO for continuing the probe? Shawn > + } else { > + external_vddd = 1; > + regulator_put(vddd); > + } > -----------------
> > > - ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies), > > > - sgtl5000->supplies); > > > - if (!ret) > > > - external_vddd = 1; > > > - else { > > > + /* External VDDD only works before revision 0x11 */ > > > + if (sgtl5000->revision < 0x11) { > > > + vddd = regulator_get_optional(codec->dev, "VDDD"); > > > + if (IS_ERR(vddd)) { > > > + /* See if it's just not registered yet */ > > > + if (PTR_ERR(vddd) == -EPROBE_DEFER) > > > + return -EPROBE_DEFER; > > > + } else { > > > + external_vddd = 1; > > > + regulator_put(vddd); > > > + } > > > > Shouldn't we handle other errors ? > > We do not really care other errors. In case that an external VDDD is present > on hardware design but regulator_get_optional("VDDD") still fails for some > other reason, we should turn to use the internal LDO anyway rather than simply > failing out. > > > We can see that from the regulator_get_optional()-->_regulator_get(), > > there are many other errors maybe returned. Not considering other > > error cases, If the dt is used here, only ERR_PTR(-ENODEV) will be > > returned if the external VDDD is absent, and only this case the > > ERR_PTR(_ENODEV) error will be returned, Or if dt is not used and > > there has no external VDDD was found, the > > ERR_PTR(-EPROBE_DEFER) will be returned. > > > > So, if the external VDDD is not used, only ERR_PTR(-ENODEV) and > > ERR_PTR(-EPROBE_DEFER) maybe returned ignoring other errors. > > > > How about the following ? > > ----------------- > > + if (IS_ERR(vddd)) { > > + /* See if it's just not registered yet */ > > + if (PTR_ERR(vddd) != -ENODEV) > > + return PTR_ERR(vddd); > > Just for example, if PTR_ERR(vddd) is -EBUSY here, instead of failing out, why > do not we switch to internal LDO for continuing the probe? > Yes, sounds perfect. Well, the regulator_get_optional() is still needed to improve. For instance, if dts is not used for one platform and the external VDDD supply is absent. The regulator_get_optional() will return -EPROBE_DEFER, so the SGTL5000's probe, and then the SGTL5000 will do the deferral probe, and this time the same -EPROBE_DEFER error will return again. Where should this special handling be dealt with ? Is here or regulator_get_optional() will be better ? Thanks, -- Xiubo
On Mon, Dec 16, 2013 at 03:31:29AM +0000, Li.Xiubo@freescale.com wrote: > Well, the regulator_get_optional() is still needed to improve. > For instance, if dts is not used for one platform and the external VDDD supply is > absent. The regulator_get_optional() will return -EPROBE_DEFER, so the SGTL5000's > probe, and then the SGTL5000 will do the deferral probe, and this time the same > -EPROBE_DEFER error will return again. Yea, it seems in non-DT that regulator core can not figure out the -ENODEV case like it does in DT, so it simply returns -EPROBE_DEFER no matter whether the regulator is absent or it's present but just not registered yet. IOW, it can not distinguish between these two cases in non-DT case. Neither am I aware of any non-DT SGTL5000 users in the tree, nor I care. I'm not concerned by this issue. > Where should this special handling be dealt with ? Is here or regulator_get_optional() > will be better ? Anyway, this is another issue which should be addressed in regulator core, IMO. Shawn
> > Well, the regulator_get_optional() is still needed to improve. > > For instance, if dts is not used for one platform and the external > > VDDD supply is absent. The regulator_get_optional() will return > > -EPROBE_DEFER, so the SGTL5000's probe, and then the SGTL5000 will do > > the deferral probe, and this time the same -EPROBE_DEFER error will > return again. > > Yea, it seems in non-DT that regulator core can not figure out the -ENODEV > case like it does in DT, so it simply returns -EPROBE_DEFER no matter > whether the regulator is absent or it's present but just not registered yet. > IOW, it can not distinguish between these two cases in non-DT case. > > Neither am I aware of any non-DT SGTL5000 users in the tree, nor I care. > I'm not concerned by this issue. > > > Where should this special handling be dealt with ? Is here or > > regulator_get_optional() will be better ? > > Anyway, this is another issue which should be addressed in regulator core, > IMO. > Yes, agree.
On Mon, Dec 16, 2013 at 07:29:12PM +0800, Shawn Guo wrote: > On Mon, Dec 16, 2013 at 03:31:29AM +0000, Li.Xiubo@freescale.com wrote: [Deferring probe in non-DT cases] > > Where should this special handling be dealt with ? Is here or regulator_get_optional() > > will be better ? > Anyway, this is another issue which should be addressed in regulator > core, IMO. It's something that the platform needs to address - the core has no way of figuring out what devices might register in future if the platform doesn't tell them. With DT we can do that because you can't create a device without providing information about what supplies will be made avaialable, other systems may be able to do the same thing.
On Tue, Dec 17, 2013 at 01:59:39AM +0000, Mark Brown wrote: > On Mon, Dec 16, 2013 at 07:29:12PM +0800, Shawn Guo wrote: > > On Mon, Dec 16, 2013 at 03:31:29AM +0000, Li.Xiubo@freescale.com wrote: > > [Deferring probe in non-DT cases] > > > Where should this special handling be dealt with ? Is here or regulator_get_optional() > > > will be better ? > > > Anyway, this is another issue which should be addressed in regulator > > core, IMO. > > It's something that the platform needs to address - the core has no way > of figuring out what devices might register in future if the platform > doesn't tell them. Okay. But is there an established means for platform to tell that? Shawn > With DT we can do that because you can't create a > device without providing information about what supplies will be made > avaialable, other systems may be able to do the same thing.
On Tue, Dec 17, 2013 at 10:16:13AM +0800, Shawn Guo wrote: > On Tue, Dec 17, 2013 at 01:59:39AM +0000, Mark Brown wrote: > > It's something that the platform needs to address - the core has no way > > of figuring out what devices might register in future if the platform > > doesn't tell them. > Okay. But is there an established means for platform to tell that? That's what has_full_constraints() is for.
On Tue, Dec 17, 2013 at 11:37:05AM +0000, Mark Brown wrote: > On Tue, Dec 17, 2013 at 10:16:13AM +0800, Shawn Guo wrote: > > On Tue, Dec 17, 2013 at 01:59:39AM +0000, Mark Brown wrote: > > > > It's something that the platform needs to address - the core has no way > > > of figuring out what devices might register in future if the platform > > > doesn't tell them. > > > Okay. But is there an established means for platform to tell that? > > That's what has_full_constraints() is for. Hmm, I do not quite follow. The regulator core needs to know whether a regulator device is present or absent, before the regulator is registered, so that regulator core can decide to return -EDEFER_PROBE or -ENODEV when consumer calls regulator_get_optional() to get the regulator. Are you saying platform can call regulator_has_full_constraints() to help regulator core to make this decision? I do not understand how it works. Shawn
On Tue, Dec 17, 2013 at 08:39:48PM +0800, Shawn Guo wrote: > Hmm, I do not quite follow. The regulator core needs to know whether a > regulator device is present or absent, before the regulator is > registered, so that regulator core can decide to return -EDEFER_PROBE > or -ENODEV when consumer calls regulator_get_optional() to get the > regulator. Are you saying platform can call > regulator_has_full_constraints() to help regulator core to make this > decision? I do not understand how it works. It tells the core that it now knows about all regulator mappings so it knows if it's not got a mapping none will ever appear.
On Tue, Dec 17, 2013 at 12:46:57PM +0000, Mark Brown wrote: > On Tue, Dec 17, 2013 at 08:39:48PM +0800, Shawn Guo wrote: > > > Hmm, I do not quite follow. The regulator core needs to know whether a > > regulator device is present or absent, before the regulator is > > registered, so that regulator core can decide to return -EDEFER_PROBE > > or -ENODEV when consumer calls regulator_get_optional() to get the > > regulator. Are you saying platform can call > > regulator_has_full_constraints() to help regulator core to make this > > decision? I do not understand how it works. > > It tells the core that it now knows about all regulator mappings so it > knows if it's not got a mapping none will ever appear. Okay, it sounds like what we need. But I searched all the occurrences of have_full_constraints() in core.c, and haven't found how it influences the regulator_map_list searching. I'm looking at function regulator_dev_lookup(). Can you point me the correct place of the code? Shawn
On Tue, Dec 17, 2013 at 09:16:15PM +0800, Shawn Guo wrote: > On Tue, Dec 17, 2013 at 12:46:57PM +0000, Mark Brown wrote: > > It tells the core that it now knows about all regulator mappings so it > > knows if it's not got a mapping none will ever appear. > Okay, it sounds like what we need. But I searched all the occurrences > of have_full_constraints() in core.c, and haven't found how it > influences the regulator_map_list searching. I'm looking at function > regulator_dev_lookup(). Can you point me the correct place of the code? For platform data it can't influence the map searching since we rely on the driver registering outbound supplies, all it does is change if we defer or error. Though the erroring code seems to have gone AWOL at some point and it'd never have been robust for modular drivers anyway.
On Fri, Dec 13, 2013 at 02:43:03PM +0800, Shawn Guo wrote: > Function sgtl5000_enable_regulators() is somehow odd in handling the > optional external VDDD supply. The driver can only enable this supply > on SGTL5000 chip before revision 0x11, and of course when this external > VDDD is present. It currently does something like below. > > 1. Check if regulator_bulk_get() on VDDA, VDDIO and VDDD will fail. If > it fails, VDDD must be absent and it falls on internal LDO by calling > sgtl5000_replace_vddd_with_ldo(). Otherwise, VDDD is used. And in > either case, regulator_bulk_enable() will be called to enable > 3 supplies. > > 2. In case that SGTL5000 revision is later than 0x11, even if external > VDDD is present, it has to roll back the 'enable' and 'get' calls > with regulator_bulk_disable() and regulator_bulk_free(), and starts > over again by calling sgtl5000_replace_vddd_with_ldo() and > regulator_bulk_enable(). > > Such back and forth calls sequence is complicated and unnecessary. > Also, since commit 4ddfebd (regulator: core: Provide a dummy regulator > with full constraints), regulator_bulk_get() will always succeeds > because of the dummy regulator. Thus the VDDD detection is broken. > > The patch changes the flow to something like the following, which should > be more reasonable and clear, and also fix the VDDD detection breakage. > > 1. Check if we're running a chip before revision 0x11, on which an > external VDDD can possibly be an option. > > 2. If it is an early revision, call regulator_get_optional() to detect > whether an external VDDD supply is available. > > 3. If external VDDD is present, call sgtl5000_replace_vddd_with_ldo() to > update sgtl5000->supplies info. > > 4. Drop regulator_bulk_get() call in sgtl5000_replace_vddd_with_ldo(), > and call it in sgtl5000_enable_regulators() no matter it's an > external VDDD or internal LDO. > > 5. Call regulator_bulk_enable() to enable these 3 regulators. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Mark, Since what Xiubo is concerned by is another issue apart from this patch itself and being discussed, can you apply the patch? Shawn
On Fri, Dec 13, 2013 at 02:43:03PM +0800, Shawn Guo wrote: > Function sgtl5000_enable_regulators() is somehow odd in handling the > optional external VDDD supply. The driver can only enable this supply > on SGTL5000 chip before revision 0x11, and of course when this external > VDDD is present. It currently does something like below. Applied, thanks.
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index bd291d2..0fcbe90 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1286,15 +1286,6 @@ static int sgtl5000_replace_vddd_with_ldo(struct snd_soc_codec *codec) sgtl5000->supplies[VDDD].supply = LDO_CONSUMER_NAME; - ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies), - sgtl5000->supplies); - - if (ret) { - ldo_regulator_remove(codec); - dev_err(codec->dev, "Failed to request supplies: %d\n", ret); - return ret; - } - dev_info(codec->dev, "Using internal LDO instead of VDDD\n"); return 0; } @@ -1305,20 +1296,35 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) int i; int external_vddd = 0; struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec); + struct regulator *vddd; 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) - external_vddd = 1; - else { + /* External VDDD only works before revision 0x11 */ + if (sgtl5000->revision < 0x11) { + vddd = regulator_get_optional(codec->dev, "VDDD"); + if (IS_ERR(vddd)) { + /* See if it's just not registered yet */ + if (PTR_ERR(vddd) == -EPROBE_DEFER) + return -EPROBE_DEFER; + } else { + external_vddd = 1; + regulator_put(vddd); + } + } + + if (!external_vddd) { ret = sgtl5000_replace_vddd_with_ldo(codec); if (ret) return ret; } + ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies), + sgtl5000->supplies); + if (ret) + goto err_ldo_remove; + ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies), sgtl5000->supplies); if (ret) @@ -1327,37 +1333,13 @@ static int sgtl5000_enable_regulators(struct snd_soc_codec *codec) /* wait for all power rails bring up */ udelay(10); - /* - * workaround for revision 0x11 and later, - * roll back to use internal LDO - */ - if (external_vddd && sgtl5000->revision >= 0x11) { - /* disable all regulator first */ - regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies), - sgtl5000->supplies); - /* free VDDD regulator */ - regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies), - sgtl5000->supplies); - - ret = sgtl5000_replace_vddd_with_ldo(codec); - if (ret) - return ret; - - ret = regulator_bulk_enable(ARRAY_SIZE(sgtl5000->supplies), - sgtl5000->supplies); - if (ret) - goto err_regulator_free; - - /* wait for all power rails bring up */ - udelay(10); - } - return 0; err_regulator_free: regulator_bulk_free(ARRAY_SIZE(sgtl5000->supplies), sgtl5000->supplies); - if (external_vddd) +err_ldo_remove: + if (!external_vddd) ldo_regulator_remove(codec); return ret;
Function sgtl5000_enable_regulators() is somehow odd in handling the optional external VDDD supply. The driver can only enable this supply on SGTL5000 chip before revision 0x11, and of course when this external VDDD is present. It currently does something like below. 1. Check if regulator_bulk_get() on VDDA, VDDIO and VDDD will fail. If it fails, VDDD must be absent and it falls on internal LDO by calling sgtl5000_replace_vddd_with_ldo(). Otherwise, VDDD is used. And in either case, regulator_bulk_enable() will be called to enable 3 supplies. 2. In case that SGTL5000 revision is later than 0x11, even if external VDDD is present, it has to roll back the 'enable' and 'get' calls with regulator_bulk_disable() and regulator_bulk_free(), and starts over again by calling sgtl5000_replace_vddd_with_ldo() and regulator_bulk_enable(). Such back and forth calls sequence is complicated and unnecessary. Also, since commit 4ddfebd (regulator: core: Provide a dummy regulator with full constraints), regulator_bulk_get() will always succeeds because of the dummy regulator. Thus the VDDD detection is broken. The patch changes the flow to something like the following, which should be more reasonable and clear, and also fix the VDDD detection breakage. 1. Check if we're running a chip before revision 0x11, on which an external VDDD can possibly be an option. 2. If it is an early revision, call regulator_get_optional() to detect whether an external VDDD supply is available. 3. If external VDDD is present, call sgtl5000_replace_vddd_with_ldo() to update sgtl5000->supplies info. 4. Drop regulator_bulk_get() call in sgtl5000_replace_vddd_with_ldo(), and call it in sgtl5000_enable_regulators() no matter it's an external VDDD or internal LDO. 5. Call regulator_bulk_enable() to enable these 3 regulators. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- sound/soc/codecs/sgtl5000.c | 62 +++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 40 deletions(-)