| Message ID | 1383289495-24523-6-git-send-email-Li.Xiubo@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Nov 01, 2013 at 03:04:52PM +0800, Xiubo Li wrote: > On VF610 series there are no regulators used, and now whether the > CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio micro? or macro? > patch series, the board cannot be probe successfully. > And this patch will solve this issue. > I finally got your idea about what this patch does. But it seems to be more likely a work around. Maybe I here can't think it through comprehensively, but I think you can try to add a fixed regulator to sgtl5000 in the devicetree, rather than disabling common functions even if not breaking others. > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com> > --- > sound/soc/codecs/sgtl5000.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c > index 1f4093f..c2f6d86 100644 > --- a/sound/soc/codecs/sgtl5000.c > +++ b/sound/soc/codecs/sgtl5000.c > @@ -61,6 +61,7 @@ static const struct reg_default sgtl5000_reg_defaults[] = { > { SGTL5000_DAP_AVC_DECAY, 0x0050 }, > }; > > +#ifdef CONFIG_REGULATOR > /* regulator supplies for sgtl5000, VDDD is an optional external supply */ > enum sgtl5000_regulator_supplies { > VDDA, > @@ -93,6 +94,9 @@ static struct regulator_init_data ldo_init_data = { > .num_consumer_supplies = 1, > .consumer_supplies = &ldo_consumer[0], > }; > +#else > +#define SGTL5000_SUPPLY_NUM 0 > +#endif > > /* > * sgtl5000 internal ldo regulator, > @@ -112,7 +116,9 @@ struct sgtl5000_priv { > int master; /* i2s master or not */ > int fmt; /* i2s data format */ > struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM]; > +#ifdef CONFIG_REGULATOR > struct ldo_regulator *ldo; > +#endif > struct regmap *regmap; > struct clk *mclk; > }; > @@ -879,6 +885,7 @@ static int ldo_regulator_remove(struct snd_soc_codec *codec) > return 0; > } > #else > +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610 Checking a platform or SoC specific define here is just a bit.... Could you pls find a better solution? Like I said at first, use fixed regulator or figure out why your System would hang with current sgtl5000 driver. If it really has a critical bug in its regulator-related code, I think you can then make a greater contribution by fixing the bug rather than bypassing it. Best regards, Nicolin Chen > static int ldo_regulator_register(struct snd_soc_codec *codec, > struct regulator_init_data *init_data, > int voltage) > @@ -886,6 +893,7 @@ static int ldo_regulator_register(struct snd_soc_codec *codec, > dev_err(codec->dev, "this setup needs regulator support in the kernel\n"); > return -EINVAL; > } > +#endif > > static int ldo_regulator_remove(struct snd_soc_codec *codec) > { > @@ -1137,6 +1145,7 @@ static int sgtl5000_resume(struct snd_soc_codec *codec) > #define sgtl5000_resume NULL > #endif /* CONFIG_SUSPEND */ > > +#ifdef CONFIG_REGULATOR > /* > * sgtl5000 has 3 internal power supplies: > * 1. VAG, normally set to vdda/2 > @@ -1373,6 +1382,7 @@ err_regulator_free: > return ret; > > } > +#endif > > static int sgtl5000_probe(struct snd_soc_codec *codec) > { > @@ -1387,6 +1397,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec) > return ret; > } > > +#ifdef CONFIG_REGULATOR > ret = sgtl5000_enable_regulators(codec); > if (ret) > return ret; > @@ -1395,6 +1406,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec) > ret = sgtl5000_set_power_regs(codec); > if (ret) > goto err; > +#endif > > /* enable small pop, introduce 400ms delay in turning off */ > snd_soc_update_bits(codec, SGTL5000_CHIP_REF_CTRL, > -- > 1.8.4 >
On Fri, Nov 01, 2013 at 03:04:52PM +0800, Xiubo Li wrote: > On VF610 series there are no regulators used, and now whether the > CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio > patch series, the board cannot be probe successfully. > And this patch will solve this issue. I don't understand what this is for at all, you're just saying there is a problem you're trying to solve but you don't explain anything about what the problem is or how your changes address it. > +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610 > static int ldo_regulator_register(struct snd_soc_codec *codec, This is definitely broken, it won't work with multi-platform kernels, and I don't understand what this is supposed to do - what is the reason for making this change?
> > On VF610 series there are no regulators used, and now whether the > > CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio patch > > series, the board cannot be probe successfully. > > And this patch will solve this issue. > > I don't understand what this is for at all, you're just saying there is a > problem you're trying to solve but you don't explain anything about what > the problem is or how your changes address it. > Sorry for confusing. The SGTL5000 is based on regulators and when it is disabled, there will be an error returns directly while the SGTL5000 codec is probing. But on VF610 boards there has no regulators or needn't configure them, so the CONFIG_REGULATOR is disabled, and then the error is returned causing to the SGTL5000 probe broken. Has I make it clear? > > +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610 > > static int ldo_regulator_register(struct snd_soc_codec *codec, > > This is definitely broken, it won't work with multi-platform kernels, and > I don't understand what this is supposed to do - what is the reason for > making this change? > Yes, it is. How about adding one node properties like : "regulator-exist", which will be controlled by platform data ?
On Wed, Nov 06, 2013 at 08:59:53AM +0000, Li Xiubo wrote: Please fix your mailer to word wrap within paragraphs. > The SGTL5000 is based on regulators and when it is disabled, there > will be an error returns directly while the SGTL5000 codec is probing. What makes you say this? That's not how the regulator API works. Have you actually seen any problems?
> > The SGTL5000 is based on regulators and when it is disabled, there > > will be an error returns directly while the SGTL5000 codec is probing. > > What makes you say this? > From the code: File path: "sound/soc/codecs/sgtl5000.c" ================== #ifdef CONFIG_REGULATOR ..... #else static int ldo_regulator_register(struct snd_soc_codec *codec, struct regulator_init_data *init_data, int voltage) { dev_err(codec->dev, "this setup needs regulator support in the kernel\n"); return -EINVAL; } static int ldo_regulator_remove(struct snd_soc_codec *codec) { return 0; } #endif ------------------- sgtl5000_probe() --> sgtl5000_enable_regulators() --> sgtl5000_replace_vddd_with_ldo() --> ldo_regulator_register(). > That's not how the regulator API works. Have > you actually seen any problems? There are some regulator APIs implemented by SGTL5000 driver, not the regulator subsystem APIs.
On Thu, Nov 07, 2013 at 03:01:02AM +0000, Li Xiubo wrote: > > > The SGTL5000 is based on regulators and when it is disabled, there > > > will be an error returns directly while the SGTL5000 codec is probing. > > What makes you say this? > static int ldo_regulator_register(struct snd_soc_codec *codec, > struct regulator_init_data *init_data, > int voltage) If the regulator is not used in the system then why is the driver getting as far as trying to register it? Surely this is a system configuration error? This all sounds like some problem with either the system integration or the driver which is causing it to try to register the regulator needlessly - you should be fixing that problem, not adding ifdefs. I'm still unclear on exactly what the issue is so it's hard to say exactly what the best way forwards is.
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 1f4093f..c2f6d86 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -61,6 +61,7 @@ static const struct reg_default sgtl5000_reg_defaults[] = { { SGTL5000_DAP_AVC_DECAY, 0x0050 }, }; +#ifdef CONFIG_REGULATOR /* regulator supplies for sgtl5000, VDDD is an optional external supply */ enum sgtl5000_regulator_supplies { VDDA, @@ -93,6 +94,9 @@ static struct regulator_init_data ldo_init_data = { .num_consumer_supplies = 1, .consumer_supplies = &ldo_consumer[0], }; +#else +#define SGTL5000_SUPPLY_NUM 0 +#endif /* * sgtl5000 internal ldo regulator, @@ -112,7 +116,9 @@ struct sgtl5000_priv { int master; /* i2s master or not */ int fmt; /* i2s data format */ struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM]; +#ifdef CONFIG_REGULATOR struct ldo_regulator *ldo; +#endif struct regmap *regmap; struct clk *mclk; }; @@ -879,6 +885,7 @@ static int ldo_regulator_remove(struct snd_soc_codec *codec) return 0; } #else +#ifndef CONFIG_SND_SOC_FSL_SGTL5000_VF610 static int ldo_regulator_register(struct snd_soc_codec *codec, struct regulator_init_data *init_data, int voltage) @@ -886,6 +893,7 @@ static int ldo_regulator_register(struct snd_soc_codec *codec, dev_err(codec->dev, "this setup needs regulator support in the kernel\n"); return -EINVAL; } +#endif static int ldo_regulator_remove(struct snd_soc_codec *codec) { @@ -1137,6 +1145,7 @@ static int sgtl5000_resume(struct snd_soc_codec *codec) #define sgtl5000_resume NULL #endif /* CONFIG_SUSPEND */ +#ifdef CONFIG_REGULATOR /* * sgtl5000 has 3 internal power supplies: * 1. VAG, normally set to vdda/2 @@ -1373,6 +1382,7 @@ err_regulator_free: return ret; } +#endif static int sgtl5000_probe(struct snd_soc_codec *codec) { @@ -1387,6 +1397,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec) return ret; } +#ifdef CONFIG_REGULATOR ret = sgtl5000_enable_regulators(codec); if (ret) return ret; @@ -1395,6 +1406,7 @@ static int sgtl5000_probe(struct snd_soc_codec *codec) ret = sgtl5000_set_power_regs(codec); if (ret) goto err; +#endif /* enable small pop, introduce 400ms delay in turning off */ snd_soc_update_bits(codec, SGTL5000_CHIP_REF_CTRL,
On VF610 series there are no regulators used, and now whether the CONFIG_REGULATOR mirco is enabled or not, for the VF610 audio patch series, the board cannot be probe successfully. And this patch will solve this issue. Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com> --- sound/soc/codecs/sgtl5000.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)