| Message ID | 1424991273-10081-4-git-send-email-eric.nelson@boundarydevices.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 26, 2015 at 03:54:30PM -0700, Eric Nelson wrote: > Initialize CHIP_ANA_POWER to match power on defaults, which disables > ADC, DAC, and charge pumps. > +++ b/sound/soc/codecs/sgtl5000.c > @@ -47,12 +47,10 @@ static const struct reg_default sgtl5000_reg_defaults[] = { > { SGTL5000_CHIP_ANA_ADC_CTRL, 0x0000 }, > { SGTL5000_CHIP_ANA_HP_CTRL, 0x1818 }, > { SGTL5000_CHIP_ANA_CTRL, 0x0111 }, > - { SGTL5000_CHIP_LINREG_CTRL, 0x0000 }, > { SGTL5000_CHIP_REF_CTRL, 0x0000 }, > { SGTL5000_CHIP_MIC_CTRL, 0x0000 }, > { SGTL5000_CHIP_LINE_OUT_CTRL, 0x0000 }, > { SGTL5000_CHIP_LINE_OUT_VOL, 0x0404 }, > - { SGTL5000_CHIP_ANA_POWER, 0x7060 }, Two big problems here. One is that this appears to also affect LINREG_CTRL which your changelog didn't mention. The other is that contrary to what the changelog says this isn't fixing the default, it's removing it. The whole point of the register defaults table is to contain the default power on register values, if it contains other things that is a bug and should be fixed by changing the values not removing them. Given that confusion I'm really having a hard time understanding the rest of the commit.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Mark, On 03/06/2015 01:12 PM, Mark Brown wrote: > On Thu, Feb 26, 2015 at 03:54:30PM -0700, Eric Nelson wrote: >> Initialize CHIP_ANA_POWER to match power on defaults, which >> disables ADC, DAC, and charge pumps. > >> +++ b/sound/soc/codecs/sgtl5000.c @@ -47,12 +47,10 @@ static >> const struct reg_default sgtl5000_reg_defaults[] = { { >> SGTL5000_CHIP_ANA_ADC_CTRL, 0x0000 }, { >> SGTL5000_CHIP_ANA_HP_CTRL, 0x1818 }, { SGTL5000_CHIP_ANA_CTRL, >> 0x0111 }, - { SGTL5000_CHIP_LINREG_CTRL, 0x0000 }, { >> SGTL5000_CHIP_REF_CTRL, 0x0000 }, { SGTL5000_CHIP_MIC_CTRL, >> 0x0000 }, { SGTL5000_CHIP_LINE_OUT_CTRL, 0x0000 }, { >> SGTL5000_CHIP_LINE_OUT_VOL, 0x0404 }, - { >> SGTL5000_CHIP_ANA_POWER, 0x7060 }, > > Two big problems here. One is that this appears to also affect > LINREG_CTRL which your changelog didn't mention. It did mention the change: > Initialize CHIP_ANA_POWER to match power on defaults, which > disables ADC, DAC, and charge pumps. > > In the process, remove references to the following > register/bitfields from the sgttl5000_set_power_regs routine: > CHIP_ANA_POWER/LINREG_SIMPLE_POWERUP and > CHIP_LINREG_CTRL/LINREG_VDD_MASK > > And remove CHIP_ANA_POWER and CHIP_LINREG_CTRL from the set > of default registers so they don't get clobbered by > sgtl5000_fill_defaults(). The CHIP_LINREG_CTRL value needs to be set based on the presence or absence of external VDDD, so writing a default (power-on) value doesn't help much, and this certainly shouldn't happen after the proper value is written. > The other is that contrary to what the changelog says this isn't > fixing the default, it's removing it. > You're right about the commit message. It should be re-worded. > The whole point of the register defaults table is to contain the > default power on register values, if it contains other things that > is a bug and should be fixed by changing the values not removing > them. > I understand that, but the crux of the problem is that these registers need to be set early, their order is critical, and some of them need to be written based on the internal/external VDDD decision. In a soft reboot on a machine that doesn't actually control the power to the SGTL5000 (which is all supported boards at the moment), a write to the ANA_POWER register with stale values in either LINREG_CTRL or CLK_CTRL (from patch 5) will fail and cause the chip to lock up. Discussion of this is the primary reason I sent this patch set as an RFC: to highlight the issues. > Given that confusion I'm really having a hard time understanding > the rest of the commit. > Some of this (and some of your expressed concerns) could be alleviated by moving the call to sgtl5000_fill_defaults() before the newly-added code to initialize ANA_POWER based on whether an external VDDD is supplied or the internal LDO is used, but then the dependencies would be hidden in the order of registers in the table. This is just more explicit. I hope that clarifies things. Regards, Eric -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJU+ia3AAoJEFUqXmm9AiVrU6AH/iu2bxDRrhBPaBBLMnXQUqiG w49zAs74PA3LcFAvR0zPhBeRjbw3NOFjAVqsr2nejpq4jtNnlxG0aYYiX+bsWA4H 52vplw8BaJxRUnR/pwa+cj7HzUwK637t8/19Zk3mfxbeqaUMX6zDS1w5k8c8U5DE 1497cxLXnX5OVjClzCyrLiZMUhLqu2BCXFgxJLHe9315MFz5T+Nd1tFXDFSVmNB8 oO85GSMBvdz2CkQ9X6wjMVVS9QnISoisEjBOhoqzfGP6A5C3p2f2zi+Sm/2Rote7 E3diMCmrH22q+IruCfQbzzVVB0B3SiU+ckQvvEWtCXWNr0RVaJwzuzdonD51Zkk= =9hrU -----END PGP SIGNATURE-----
On Fri, Mar 06, 2015 at 03:14:16PM -0700, Eric Nelson wrote: > On 03/06/2015 01:12 PM, Mark Brown wrote: > > Two big problems here. One is that this appears to also affect > > LINREG_CTRL which your changelog didn't mention. > It did mention the change: > > Initialize CHIP_ANA_POWER to match power on defaults, which > > disables ADC, DAC, and charge pumps. > > > > In the process, remove references to the following > > register/bitfields from the sgttl5000_set_power_regs routine: That's burying the lead a bit - that just looked like a list of bitfields within the register (hardware designers often call ndividual bitfields registers). > The CHIP_LINREG_CTRL value needs to be set based on the presence > or absence of external VDDD, so writing a default (power-on) value > doesn't help much, and this certainly shouldn't happen after > the proper value is written. This indicates that there is a greater confusion in the code which should be fixed, this sounds more like a patch of some kind at this point. At the very least this needs to be renamed, or there needs to be handling in the sync code for the more sensitive registers. My understanding was that this was being done to regain control of the chip after driver start because there's no reset feature in the chip. That's a reasonable thing to want to do but apparently the current code isn't very well thought out.
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 2ac4db5..0a7b96d7 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -47,12 +47,10 @@ static const struct reg_default sgtl5000_reg_defaults[] = { { SGTL5000_CHIP_ANA_ADC_CTRL, 0x0000 }, { SGTL5000_CHIP_ANA_HP_CTRL, 0x1818 }, { SGTL5000_CHIP_ANA_CTRL, 0x0111 }, - { SGTL5000_CHIP_LINREG_CTRL, 0x0000 }, { SGTL5000_CHIP_REF_CTRL, 0x0000 }, { SGTL5000_CHIP_MIC_CTRL, 0x0000 }, { SGTL5000_CHIP_LINE_OUT_CTRL, 0x0000 }, { SGTL5000_CHIP_LINE_OUT_VOL, 0x0404 }, - { SGTL5000_CHIP_ANA_POWER, 0x7060 }, { SGTL5000_CHIP_PLL_CTRL, 0x5000 }, { SGTL5000_CHIP_CLK_TOP_CTRL, 0x0000 }, { SGTL5000_CHIP_ANA_STATUS, 0x0000 }, @@ -93,6 +91,7 @@ static const char *supply_names[SGTL5000_SUPPLY_NUM] = { }; #define LDO_VOLTAGE 1200000 +#define LINREG_VDDD ((1600 - LDO_VOLTAGE / 1000) / 50) enum sgtl5000_micbias_resistor { SGTL5000_MICBIAS_OFF = 0, @@ -993,25 +992,6 @@ static int sgtl5000_set_power_regs(struct snd_soc_codec *codec) snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, ana_pwr); - /* set voltage to register */ - snd_soc_update_bits(codec, SGTL5000_CHIP_LINREG_CTRL, - SGTL5000_LINREG_VDDD_MASK, 0x8); - - /* - * if vddd linear reg has been enabled, - * simple digital supply should be clear to get - * proper VDDD voltage. - */ - if (ana_pwr & SGTL5000_LINEREG_D_POWERUP) - snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, - SGTL5000_LINREG_SIMPLE_POWERUP, - 0); - else - snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER, - SGTL5000_LINREG_SIMPLE_POWERUP | - SGTL5000_STARTUP_POWERUP, - 0); - /* * set ADC/DAC VAG to vdda / 2, * should stay in range (0.8v, 1.575v) @@ -1211,6 +1191,7 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, int ret, reg, rev; struct device_node *np = client->dev.of_node; u32 value; + u16 ana_pwr; sgtl5000 = devm_kzalloc(&client->dev, sizeof(*sgtl5000), GFP_KERNEL); if (!sgtl5000) @@ -1268,29 +1249,25 @@ static int sgtl5000_i2c_probe(struct i2c_client *client, sgtl5000->revision = rev; /* Follow section 2.2.1.1 of AN3663 */ + ana_pwr = SGTL5000_ANA_POWER_DEFAULT; if (sgtl5000->num_supplies <= VDDD) { /* internal VDDD at 1.2V */ regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_LINREG_CTRL, - SGTL5000_LINREG_VDDD_MASK, 8); - regmap_update_bits(sgtl5000->regmap, - SGTL5000_CHIP_ANA_POWER, - SGTL5000_LINEREG_D_POWERUP - | SGTL5000_LINREG_SIMPLE_POWERUP, - SGTL5000_LINEREG_D_POWERUP); + SGTL5000_LINREG_VDDD_MASK, + LINREG_VDDD); + ana_pwr |= SGTL5000_LINEREG_D_POWERUP; dev_info(&client->dev, "Using internal LDO instead of VDDD: check ER1\n"); } else { /* using external LDO for VDDD * Clear startup powerup and simple powerup * bits to save power */ - regmap_update_bits(sgtl5000->regmap, - SGTL5000_CHIP_ANA_POWER, - SGTL5000_STARTUP_POWERUP - | SGTL5000_LINREG_SIMPLE_POWERUP, - 0); + ana_pwr &= ~(SGTL5000_STARTUP_POWERUP + | SGTL5000_LINREG_SIMPLE_POWERUP); dev_dbg(&client->dev, "Using external VDDD\n"); } + regmap_write(sgtl5000->regmap, SGTL5000_CHIP_ANA_POWER, ana_pwr); if (np) { if (!of_property_read_u32(np, diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h index bd7a344..6fdc589 100644 --- a/sound/soc/codecs/sgtl5000.h +++ b/sound/soc/codecs/sgtl5000.h @@ -325,6 +325,7 @@ /* * SGTL5000_CHIP_ANA_POWER */ +#define SGTL5000_ANA_POWER_DEFAULT 0x7060 #define SGTL5000_DAC_STEREO 0x4000 #define SGTL5000_LINREG_SIMPLE_POWERUP 0x2000 #define SGTL5000_STARTUP_POWERUP 0x1000
Initialize CHIP_ANA_POWER to match power on defaults, which disables ADC, DAC, and charge pumps. In the process, remove references to the following register/bitfields from the sgttl5000_set_power_regs routine: CHIP_ANA_POWER/LINREG_SIMPLE_POWERUP and CHIP_LINREG_CTRL/LINREG_VDD_MASK And remove CHIP_ANA_POWER and CHIP_LINREG_CTRL from the set of default registers so they don't get clobbered by sgtl5000_fill_defaults(). Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com> --- sound/soc/codecs/sgtl5000.c | 41 +++++++++-------------------------------- sound/soc/codecs/sgtl5000.h | 1 + 2 files changed, 10 insertions(+), 32 deletions(-)