| Message ID | 20150306225337.GH8656@n2100.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Russell, On 03/06/2015 03:53 PM, Russell King - ARM Linux wrote: > On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote: >> Based on your review, it seems like the following is needed for a >> V2 patch (set): >> >> - Move call to sgtl5000_fill_defaults() earlier in the >> sgtl5000_i2c_probe routine, before adjusting ANA_POWER. >> This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER >> back into the default register list. >> - Move regulators from codec to I2C device >> - switch to devm_regulator api >> - various code cleanups to simplify logic >> - Adjust regulator usage in SND_SOC_BIAS_OFF >> >> I'll leave out the "write all default" registers for the moment. > > I'll also interject here to say that, against mainline, I need this > patch to make the sgtl5k work on SolidRun's Hummingboard hardware. > Without this, the internal regulator seems to get powered down, and > we lose access to the device until the entire board is power cycled. > Mark applied essentially the patch below separately from the LDO support: http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/088376.html http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/088773.html > sound/soc/codecs/sgtl5000.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c > index e182e6569bbd..79d5cbd65f36 100644 > --- a/sound/soc/codecs/sgtl5000.c > +++ b/sound/soc/codecs/sgtl5000.c > @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev) > struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data; > > snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, > + SGTL5000_LINREG_SIMPLE_POWERUP, > + SGTL5000_LINREG_SIMPLE_POWERUP); > + > + snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, > SGTL5000_LINEREG_D_POWERUP, > 0); > > @@ -1155,8 +1159,11 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec) > * if vddio and vddd > 3.1v, > * charge pump should be clean before set ana_pwr > */ > - snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, > - SGTL5000_VDDC_CHRGPMP_POWERUP, 0); > +// FIXME: this is total crap - we have read this register above into > +// ana_pwr, which we then modify (above), and then write back to the > +// register below. This modification just gets completely overwritten. > +// snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, > +// SGTL5000_VDDC_CHRGPMP_POWERUP, 0); > > /* VDDC use VDDIO rail */ > lreg_ctrl |= SGTL5000_VDDC_ASSN_OVRD; > > The bulk of this patch set tries to make the VDDIO and VDDA regulators work and prep for the use of external VDDD, which errata ER1 says is best practice (and which nobody is following). Regards, Eric
Hi Russell, On 03/06/2015 03:58 PM, Eric Nelson wrote: > Hi Russell, > > On 03/06/2015 03:53 PM, Russell King - ARM Linux wrote: >> On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote: >>> Based on your review, it seems like the following is needed for a >>> V2 patch (set): >>> >>> - Move call to sgtl5000_fill_defaults() earlier in the >>> sgtl5000_i2c_probe routine, before adjusting ANA_POWER. >>> This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER >>> back into the default register list. >>> - Move regulators from codec to I2C device >>> - switch to devm_regulator api >>> - various code cleanups to simplify logic >>> - Adjust regulator usage in SND_SOC_BIAS_OFF >>> >>> I'll leave out the "write all default" registers for the moment. >> >> I'll also interject here to say that, against mainline, I need this >> patch to make the sgtl5k work on SolidRun's Hummingboard hardware. >> Without this, the internal regulator seems to get powered down, and >> we lose access to the device until the entire board is power cycled. >> > > Mark applied essentially the patch below separately from > the LDO support: > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/088376.html > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/088773.html > >> sound/soc/codecs/sgtl5000.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c >> index e182e6569bbd..79d5cbd65f36 100644 >> --- a/sound/soc/codecs/sgtl5000.c >> +++ b/sound/soc/codecs/sgtl5000.c >> @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev) >> struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data; >> Sorry. This part of the patch you sent wasn't included in the patch I mentioned above: >> snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, >> + SGTL5000_LINREG_SIMPLE_POWERUP, >> + SGTL5000_LINREG_SIMPLE_POWERUP); >> + >> + snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, >> SGTL5000_LINEREG_D_POWERUP, >> 0); >> In the full patch-set, initialization of ANA_POWER is placed inside the sgtl5000_i2c_probe routine and the ldo_regulator is discarded. I'm not sure why the code above is needed though. Do you know what sequence of events causes this to fail? Please advise, Eric
On Fri, Mar 06, 2015 at 04:16:18PM -0700, Eric Nelson wrote: > Hi Russell, > > On 03/06/2015 03:58 PM, Eric Nelson wrote: > > Hi Russell, > > > > On 03/06/2015 03:53 PM, Russell King - ARM Linux wrote: > >> On Fri, Mar 06, 2015 at 03:49:39PM -0700, Eric Nelson wrote: > >>> Based on your review, it seems like the following is needed for a > >>> V2 patch (set): > >>> > >>> - Move call to sgtl5000_fill_defaults() earlier in the > >>> sgtl5000_i2c_probe routine, before adjusting ANA_POWER. > >>> This will allow registers LINEREG_CTRL, CLK_CTRL, and even ANA_POWER > >>> back into the default register list. > >>> - Move regulators from codec to I2C device > >>> - switch to devm_regulator api > >>> - various code cleanups to simplify logic > >>> - Adjust regulator usage in SND_SOC_BIAS_OFF > >>> > >>> I'll leave out the "write all default" registers for the moment. > >> > >> I'll also interject here to say that, against mainline, I need this > >> patch to make the sgtl5k work on SolidRun's Hummingboard hardware. > >> Without this, the internal regulator seems to get powered down, and > >> we lose access to the device until the entire board is power cycled. > >> > > > > Mark applied essentially the patch below separately from > > the LDO support: > > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/088376.html > > http://mailman.alsa-project.org/pipermail/alsa-devel/2015-March/088773.html > > > >> sound/soc/codecs/sgtl5000.c | 11 +++++++++-- > >> 1 file changed, 9 insertions(+), 2 deletions(-) > >> > >> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c > >> index e182e6569bbd..79d5cbd65f36 100644 > >> --- a/sound/soc/codecs/sgtl5000.c > >> +++ b/sound/soc/codecs/sgtl5000.c > >> @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev) > >> struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data; > >> > > Sorry. This part of the patch you sent wasn't included in the > patch I mentioned above: > > >> snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, > >> + SGTL5000_LINREG_SIMPLE_POWERUP, > >> + SGTL5000_LINREG_SIMPLE_POWERUP); > >> + > >> + snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, > >> SGTL5000_LINEREG_D_POWERUP, > >> 0); > >> > > In the full patch-set, initialization of ANA_POWER is placed inside > the sgtl5000_i2c_probe routine and the ldo_regulator is discarded. > > I'm not sure why the code above is needed though. > > Do you know what sequence of events causes this to fail? All that I remember is that as the mainline code stood in 3.19 etc, the SGTL5k would be partially detected and then I2C communication with it was lost. Probing the pins with a scope showed that it had basically lost power. The patch above is over a year old now, and I don't remember much more detail than that. I'm not going to be physically in a state where I can look at it for about a week and a bit, sorry.
Hi Russell, On 03/06/2015 04:24 PM, Russell King - ARM Linux wrote: > On Fri, Mar 06, 2015 at 04:16:18PM -0700, Eric Nelson wrote: >> Hi Russell, >> >> <snip> >> >> I'm not sure why the code above is needed though. >> >> Do you know what sequence of events causes this to fail? > > All that I remember is that as the mainline code stood in 3.19 etc, > the SGTL5k would be partially detected and then I2C communication > with it was lost. Probing the pins with a scope showed that it had > basically lost power. > > The patch above is over a year old now, and I don't remember much > more detail than that. > > I'm not going to be physically in a state where I can look at it for > about a week and a bit, sorry. > No worries. I appreciate the feedback.
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index e182e6569bbd..79d5cbd65f36 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -825,6 +825,10 @@ static int ldo_regulator_disable(struct regulator_dev *dev) struct snd_soc_codec *codec = (struct snd_soc_codec *)ldo->codec_data; snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, + SGTL5000_LINREG_SIMPLE_POWERUP, + SGTL5000_LINREG_SIMPLE_POWERUP); + + snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, SGTL5000_LINEREG_D_POWERUP, 0); @@ -1155,8 +1159,11 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec) * if vddio and vddd > 3.1v, * charge pump should be clean before set ana_pwr */ - snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, - SGTL5000_VDDC_CHRGPMP_POWERUP, 0); +// FIXME: this is total crap - we have read this register above into +// ana_pwr, which we then modify (above), and then write back to the +// register below. This modification just gets completely overwritten. +// snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, +// SGTL5000_VDDC_CHRGPMP_POWERUP, 0); /* VDDC use VDDIO rail */ lreg_ctrl |= SGTL5000_VDDC_ASSN_OVRD;