[PATCH] ASoC: cs42l42: Implement system suspend
Add system suspend functions to handle clean power-down on suspend and restoring state on resume.
The jack state could change during suspend. Plug->unplug and unplug->plug are straightforward because this looks no different from any other plug state change. However, the jack could be unplugged and a different type of jack plugged, and on resume the plug state would not have changed. Some code changes are needed to the jack handling so that on resume a plugged state will be re-evaluated instead of filtered out. In this case there would not have been a previous unplug event to clear the reported jack state so the full state of all jack type and button flags must be reported.
Note that during system suspend any jack plug/unplug and button events will not be reported or generate a system wakeup. If the plug state or headset type has changed it will be reported after resume.
Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/codecs/cs42l42.c | 212 ++++++++++++++++++++++++++++++++++++++++++--- sound/soc/codecs/cs42l42.h | 8 +- 2 files changed, 209 insertions(+), 11 deletions(-)
diff --git a/sound/soc/codecs/cs42l42.c b/sound/soc/codecs/cs42l42.c index 43d98bdb5b5b..5abac0dc8e8b 100644 --- a/sound/soc/codecs/cs42l42.c +++ b/sound/soc/codecs/cs42l42.c @@ -47,7 +47,6 @@ static const struct reg_default cs42l42_reg_defaults[] = { { CS42L42_I2C_STRETCH, 0x03 }, { CS42L42_I2C_TIMEOUT, 0xB7 }, { CS42L42_PWR_CTL1, 0xFF }, - { CS42L42_PWR_CTL2, 0x84 }, { CS42L42_PWR_CTL3, 0x20 }, { CS42L42_RSENSE_CTL1, 0x40 }, { CS42L42_RSENSE_CTL2, 0x00 }, @@ -331,6 +330,11 @@ static bool cs42l42_volatile_register(struct device *dev, unsigned int reg) case CS42L42_DEVID_CD: case CS42L42_DEVID_E: case CS42L42_MCLK_STATUS: + /* + * PWR_CTL2 must be volatile so it can be used as a canary bit to + * detect a reset during system suspend. + */ + case CS42L42_PWR_CTL2: case CS42L42_OSC_SWITCH_STATUS: case CS42L42_TRSENSE_STATUS: case CS42L42_HS_DET_STATUS: @@ -550,7 +554,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_ struct cs42l42_private *cs42l42 = snd_soc_component_get_drvdata(component);
/* Prevent race with interrupt handler */ - mutex_lock(&cs42l42->jack_detect_mutex); + mutex_lock(&cs42l42->irq_lock); cs42l42->jack = jk;
if (jk) { @@ -566,7 +570,7 @@ static int cs42l42_set_jack(struct snd_soc_component *component, struct snd_soc_ break; } } - mutex_unlock(&cs42l42->jack_detect_mutex); + mutex_unlock(&cs42l42->irq_lock);
return 0; } @@ -1613,6 +1617,12 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data) unsigned int i; int report = 0;
+ mutex_lock(&cs42l42->irq_lock); + + if (cs42l42->suspended) { + mutex_unlock(&cs42l42->irq_lock); + return IRQ_NONE; + }
/* Read sticky registers to clear interurpt */ for (i = 0; i < ARRAY_SIZE(stickies); i++) { @@ -1635,9 +1645,12 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data) CS42L42_M_DETECT_FT_MASK | CS42L42_M_HSBIAS_HIZ_MASK);
- mutex_lock(&cs42l42->jack_detect_mutex); - - /* Check auto-detect status */ + /* + * Check auto-detect status. During system suspend the jack could be + * unplugged and a different type plugged. On resume there will not + * have been a previous unplug event to clear the reported flags, so + * here the full state of all flags must be reported. + */ if ((~masks[5]) & irq_params_table[5].mask) { if (stickies[5] & CS42L42_HSDET_AUTO_DONE_MASK) { cs42l42_process_hs_type_detect(cs42l42); @@ -1645,11 +1658,15 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data) case CS42L42_PLUG_CTIA: case CS42L42_PLUG_OMTP: snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET, - SND_JACK_HEADSET); + SND_JACK_HEADSET | + SND_JACK_BTN_0 | SND_JACK_BTN_1 | + SND_JACK_BTN_2 | SND_JACK_BTN_3); break; case CS42L42_PLUG_HEADPHONE: snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE, - SND_JACK_HEADPHONE); + SND_JACK_HEADSET | + SND_JACK_BTN_0 | SND_JACK_BTN_1 | + SND_JACK_BTN_2 | SND_JACK_BTN_3); break; default: break; @@ -1705,7 +1722,7 @@ static irqreturn_t cs42l42_irq_thread(int irq, void *data) } }
- mutex_unlock(&cs42l42->jack_detect_mutex); + mutex_unlock(&cs42l42->irq_lock);
return IRQ_HANDLED; } @@ -2053,8 +2070,9 @@ static int cs42l42_i2c_probe(struct i2c_client *i2c_client, return -ENOMEM;
cs42l42->dev = &i2c_client->dev; + cs42l42->irq = i2c_client->irq; i2c_set_clientdata(i2c_client, cs42l42); - mutex_init(&cs42l42->jack_detect_mutex); + mutex_init(&cs42l42->irq_lock);
cs42l42->regmap = devm_regmap_init_i2c(i2c_client, &cs42l42_regmap); if (IS_ERR(cs42l42->regmap)) { @@ -2210,6 +2228,179 @@ static int cs42l42_i2c_remove(struct i2c_client *i2c_client) return 0; }
+/* Suspend sequence from datasheet */ +static const struct reg_sequence __maybe_unused cs42l42_suspend_seq[] = { + REG_SEQ0(CS42L42_MIC_DET_CTL1, 0x9f), + REG_SEQ0(CS42L42_ADC_OVFL_INT_MASK, 0x01), + REG_SEQ0(CS42L42_MIXER_INT_MASK, 0x0F), + REG_SEQ0(CS42L42_SRC_INT_MASK, 0x0F), + REG_SEQ0(CS42L42_ASP_RX_INT_MASK, 0x1F), + REG_SEQ0(CS42L42_ASP_TX_INT_MASK, 0x0F), + REG_SEQ0(CS42L42_CODEC_INT_MASK, 0x03), + REG_SEQ0(CS42L42_SRCPL_INT_MASK, 0x7F), + REG_SEQ0(CS42L42_VPMON_INT_MASK, 0x01), + REG_SEQ0(CS42L42_PLL_LOCK_INT_MASK, 0x01), + REG_SEQ0(CS42L42_TSRS_PLUG_INT_MASK, 0x0F), + REG_SEQ0(CS42L42_WAKE_CTL, 0xE1), + REG_SEQ0(CS42L42_DET_INT1_MASK, 0xE0), + REG_SEQ0(CS42L42_DET_INT2_MASK, 0xFF), + REG_SEQ0(CS42L42_MIXER_CHA_VOL, 0x3f), + REG_SEQ0(CS42L42_MIXER_ADC_VOL, 0x3f), + REG_SEQ0(CS42L42_MIXER_CHB_VOL, 0x3f), + REG_SEQ0(CS42L42_HP_CTL, 0x0f), + REG_SEQ0(CS42L42_ASP_RX_DAI0_EN, 0x00), + REG_SEQ0(CS42L42_ASP_CLK_CFG, 0x00), + REG_SEQ0(CS42L42_HSDET_CTL2, 0x00), + REG_SEQ0(CS42L42_PWR_CTL1, 0xfe), + REG_SEQ0(CS42L42_PWR_CTL2, 0x8c), + REG_SEQ0(CS42L42_DAC_CTL2, 0x02), + REG_SEQ0(CS42L42_HS_CLAMP_DISABLE, 0x00), + REG_SEQ0(CS42L42_MISC_DET_CTL, 0x03), + REG_SEQ0(CS42L42_TIPSENSE_CTL, 0x02), + REG_SEQ0(CS42L42_HSBIAS_SC_AUTOCTL, 0x03), + REG_SEQ0(CS42L42_PWR_CTL1, 0xff), +}; + +static const struct reg_sequence __maybe_unused cs42l42_suspend_post_seq[] = { + REG_SEQ0(CS42L42_PWR_CTL2, 0x9c), +}; + +static int __maybe_unused cs42l42_suspend(struct device *dev) +{ + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev); + unsigned int reg; + int ret; + + if (cs42l42->irq >= 0) + disable_irq(cs42l42->irq); + + /* + * Wait for threaded irq handler to be idle and stop it processing + * future interrupts. This ensures a safe disable if the interrupt + * is shared. + */ + mutex_lock(&cs42l42->irq_lock); + cs42l42->suspended = true; + + /* Clear any previous PDN_DONE */ + regmap_read(cs42l42->regmap, CS42L42_CODEC_STATUS, ®); + + /* Preserve cached values so they can be restored. */ + regmap_multi_reg_write_bypassed(cs42l42->regmap, + cs42l42_suspend_seq, + ARRAY_SIZE(cs42l42_suspend_seq)); + + /* The cached address page register value is now stale */ + regcache_drop_region(cs42l42->regmap, CS42L42_PAGE_REGISTER, CS42L42_PAGE_REGISTER); + + /* Wait for power-down complete */ + msleep(CS42L42_PDN_DONE_TIME_MS); + ret = regmap_read_poll_timeout(cs42l42->regmap, CS42L42_CODEC_STATUS, reg, + (reg & CS42L42_PDN_DONE_MASK), + CS42L42_PDN_DONE_POLL_US, + CS42L42_PDN_DONE_TIMEOUT_US); + if (ret) + dev_warn(dev, "Failed to get PDN_DONE: %d\n", ret); + + regmap_multi_reg_write_bypassed(cs42l42->regmap, + cs42l42_suspend_post_seq, + ARRAY_SIZE(cs42l42_suspend_post_seq)); + msleep(CS42L42_FILT_DISCHARGE_TIME_MS); + + /* The cached address page register value is now stale */ + regcache_drop_region(cs42l42->regmap, CS42L42_PAGE_REGISTER, CS42L42_PAGE_REGISTER); + + regcache_cache_only(cs42l42->regmap, true); + + gpiod_set_value_cansleep(cs42l42->reset_gpio, 0); + regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies); + + mutex_unlock(&cs42l42->irq_lock); + + dev_dbg(dev, "System suspended\n"); + + return 0; +} + +static int __maybe_unused cs42l42_resume(struct device *dev) +{ + struct cs42l42_private *cs42l42 = dev_get_drvdata(dev); + unsigned int val; + int ret; + + mutex_lock(&cs42l42->irq_lock); + + /* + * If jack was unplugged and re-plugged during suspend it could have + * changed type but the tip-sense state hasn't changed. Force a + * plugged state to be re-evaluated if it is still plugged. + */ + if (cs42l42->plug_state != CS42L42_TS_UNPLUG) + cs42l42->plug_state = CS42L42_TS_TRANS; + + ret = regulator_bulk_enable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies); + if (ret != 0) { + dev_err(dev, "Failed to enable supplies: %d\n", ret); + return ret; + } + + gpiod_set_value_cansleep(cs42l42->reset_gpio, 1); + usleep_range(CS42L42_BOOT_TIME_US, CS42L42_BOOT_TIME_US * 2); + + regcache_cache_only(cs42l42->regmap, false); + + if (cs42l42->reset_gpio) { + /* Registers have reset to defaults */ + regcache_mark_dirty(cs42l42->regmap); + } else { + /* + * Only call regcache_mark_dirty() if cs42l42 reset, so + * regcache_sync() writes all non-default cached values. + * If it didn't reset we don't want to filter out syncing + * dirty cache entries that have default value. + * DISCHARGE_FILT==1 after suspend. If the cs42l42 reset + * it will now be 0. + */ + ret = regmap_read(cs42l42->regmap, CS42L42_PWR_CTL2, &val); + if (ret) { + dev_err(dev, "failed to read PWR_CTL2: %d\n", ret); + goto err; + } + + if ((val & CS42L42_DISCHARGE_FILT_MASK) == 0) { + dev_dbg(dev, "Has reset in suspend\n"); + regcache_mark_dirty(cs42l42->regmap); + } else { + dev_dbg(dev, "Did not reset in suspend\n"); + } + } + + /* Sync LATCH_TO_VP first so the VP domain registers sync correctly */ + regcache_sync_region(cs42l42->regmap, CS42L42_MIC_DET_CTL1, CS42L42_MIC_DET_CTL1); + regcache_sync(cs42l42->regmap); + + cs42l42->suspended = false; + mutex_unlock(&cs42l42->irq_lock); + + if (cs42l42->irq >= 0) + enable_irq(cs42l42->irq); + + dev_dbg(dev, "System resumed\n"); + + return 0; +err: + gpiod_set_value_cansleep(cs42l42->reset_gpio, 0); + regulator_bulk_disable(ARRAY_SIZE(cs42l42->supplies), cs42l42->supplies); + + mutex_unlock(&cs42l42->irq_lock); + + return ret; +} + +static const struct dev_pm_ops cs42l42_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(cs42l42_suspend, cs42l42_resume) +}; + #ifdef CONFIG_OF static const struct of_device_id cs42l42_of_match[] = { { .compatible = "cirrus,cs42l42", }, @@ -2236,6 +2427,7 @@ MODULE_DEVICE_TABLE(i2c, cs42l42_id); static struct i2c_driver cs42l42_i2c_driver = { .driver = { .name = "cs42l42", + .pm = &cs42l42_pm_ops, .of_match_table = of_match_ptr(cs42l42_of_match), .acpi_match_table = ACPI_PTR(cs42l42_acpi_match), }, diff --git a/sound/soc/codecs/cs42l42.h b/sound/soc/codecs/cs42l42.h index 9fff183dce8e..b497ca1618c2 100644 --- a/sound/soc/codecs/cs42l42.h +++ b/sound/soc/codecs/cs42l42.h @@ -826,6 +826,10 @@ #define CS42L42_PLL_LOCK_POLL_US 250 #define CS42L42_PLL_LOCK_TIMEOUT_US 1250 #define CS42L42_HP_ADC_EN_TIME_US 20000 +#define CS42L42_PDN_DONE_POLL_US 1000 +#define CS42L42_PDN_DONE_TIMEOUT_US 200000 +#define CS42L42_PDN_DONE_TIME_MS 100 +#define CS42L42_FILT_DISCHARGE_TIME_MS 46
static const char *const cs42l42_supply_names[CS42L42_NUM_SUPPLIES] = { "VA", @@ -842,7 +846,8 @@ struct cs42l42_private { struct gpio_desc *reset_gpio; struct completion pdn_done; struct snd_soc_jack *jack; - struct mutex jack_detect_mutex; + int irq; + struct mutex irq_lock; int pll_config; int bclk; u32 sclk; @@ -860,6 +865,7 @@ struct cs42l42_private { u8 hs_bias_sense_en; u8 stream_use; bool hp_adc_up_pending; + bool suspended; };
#endif /* __CS42L42_H__ */
On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
Add system suspend functions to handle clean power-down on suspend and restoring state on resume.
The jack state could change during suspend. Plug->unplug and unplug->plug are straightforward because this looks no different from any other plug state change. However, the jack could be unplugged and a different type
This fiddling about with the jack detect feels like it should be at least one separate change, possibly multiple changes - the reporting of button states feels like it should be a good cleanup/fix separately for example.
of jack plugged, and on resume the plug state would not have changed. Some code changes are needed to the jack handling so that on resume a plugged state will be re-evaluated instead of filtered out. In this case
It would be helpful to elaborate on what these code changes might be.
- /*
* PWR_CTL2 must be volatile so it can be used as a canary bit to* detect a reset during system suspend.*/- case CS42L42_PWR_CTL2:
This feels a bit hackish - is the cost of doing the cache sync really so expensive that it's worth the effort of trying to skip it?
- if (cs42l42->suspended) {
mutex_unlock(&cs42l42->irq_lock);return IRQ_NONE;- }
Given that you're using disable_irq() to force the interrupt off (which is a bit rude but often the best one can do) how might we be getting an interrupt while suspended? This seems to indicate an error condition.
case CS42L42_PLUG_OMTP: snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
SND_JACK_HEADSET);
SND_JACK_HEADSET |SND_JACK_BTN_0 | SND_JACK_BTN_1 |SND_JACK_BTN_2 | SND_JACK_BTN_3); break; case CS42L42_PLUG_HEADPHONE: snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
SND_JACK_HEADPHONE);
SND_JACK_HEADSET |SND_JACK_BTN_0 | SND_JACK_BTN_1 |SND_JACK_BTN_2 | SND_JACK_BTN_3);
This unconditionally clears the button status - will something notice if the buttons are pressed?
- } else {
/** Only call regcache_mark_dirty() if cs42l42 reset, so* regcache_sync() writes all non-default cached values.* If it didn't reset we don't want to filter out syncing* dirty cache entries that have default value.* DISCHARGE_FILT==1 after suspend. If the cs42l42 reset* it will now be 0.*/
Something needs to tell regmap that the cache is dirty otherwise it won't sync anything, including the non-default register values? There's currently an assumption coded in there that the cache is dirty because the device was reset and everything has default values.
On 01/12/2021 16:32, Mark Brown wrote:
On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
Add system suspend functions to handle clean power-down on suspend and restoring state on resume.
The jack state could change during suspend. Plug->unplug and unplug->plug are straightforward because this looks no different from any other plug state change. However, the jack could be unplugged and a different type
This fiddling about with the jack detect feels like it should be at least one separate change, possibly multiple changes - the reporting of button states feels like it should be a good cleanup/fix separately for example.
I'll split them out.
of jack plugged, and on resume the plug state would not have changed. Some code changes are needed to the jack handling so that on resume a plugged state will be re-evaluated instead of filtered out. In this case
It would be helpful to elaborate on what these code changes might be.
- /*
* PWR_CTL2 must be volatile so it can be used as a canary bit to* detect a reset during system suspend.*/- case CS42L42_PWR_CTL2:
This feels a bit hackish
I can't think of a better way of detecting whether the chip reset. If we don't have control of the reset GPIO we can't force a reset. So it depends whether power turned off (also whether it dropped below the Vmin voltage at which a reset is triggered). The regulator framework can't tell us if it really turned off power and on ACPI systems the power is all controlled magically.
- is the cost of doing the cache sync really so> expensive that
it's worth the effort of trying to skip it?
It's not cost, it's about getting the correct values in the registers. If we call regcache_mark_dirty() it tells regmap that all the hardware registers have reset to default. Then regcache_sync() will skip writing cache values that are the register default value, assuming that the register already has this value. If the chip didn't reset, the registers could retain non-default values while the cache contains a dirty default value, in that case the register needs updating to match the cache.
I tried to persuade myself that a cache value couldn't possibly change at any time from suspend() being called to resume disabling cache-only so I could safely accept default values being skipped. But that assumption made me very uncomfortable - I don't like leaving potential bugs in when its simple enough to prevent them.
- if (cs42l42->suspended) {
mutex_unlock(&cs42l42->irq_lock);return IRQ_NONE;- }
Given that you're using disable_irq() to force the interrupt off (which is a bit rude but often the best one can do) how might we be getting an interrupt while suspended? This seems to indicate an error condition.
I may have misunderstood here, but the documentation says that enables/disables are nested. The interrupt starts out enabled right after calling request_threaded_irq(), so I expected that all users of the shared interrupt would have to call disable_irq() for it to actually get disabled (I put the call in on that basis that it does no harm). If disable_irq() forces the irq off even if it's shared then I'll remove it because as you say that would be rude.
case CS42L42_PLUG_OMTP: snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADSET,
SND_JACK_HEADSET);
SND_JACK_HEADSET |SND_JACK_BTN_0 | SND_JACK_BTN_1 |SND_JACK_BTN_2 | SND_JACK_BTN_3); break; case CS42L42_PLUG_HEADPHONE: snd_soc_jack_report(cs42l42->jack, SND_JACK_HEADPHONE,
SND_JACK_HEADPHONE);
SND_JACK_HEADSET |SND_JACK_BTN_0 | SND_JACK_BTN_1 |SND_JACK_BTN_2 | SND_JACK_BTN_3);This unconditionally clears the button status - will something notice if the buttons are pressed?
- } else {
/** Only call regcache_mark_dirty() if cs42l42 reset, so* regcache_sync() writes all non-default cached values.* If it didn't reset we don't want to filter out syncing* dirty cache entries that have default value.* DISCHARGE_FILT==1 after suspend. If the cs42l42 reset* it will now be 0.*/Something needs to tell regmap that the cache is dirty otherwise it
Regmap knows if it has dirty values that it hasn't written out to the hardware.
won't sync anything, including the non-default register values? There's
That's fine. If the registers have not been reset they still have the value of every clean cache entry. Every dirty cache entry will be written out by regcache_sync().
currently an assumption coded in there that the cache is dirty because the device was reset and everything has default values.
Regmap only assumes that if regcache_mark_dirty() is called.
On Wed, Dec 01, 2021 at 06:04:00PM +0000, Richard Fitzgerald wrote:
On 01/12/2021 16:32, Mark Brown wrote:
On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
- /*
* PWR_CTL2 must be volatile so it can be used as a canary bit to* detect a reset during system suspend.*/- case CS42L42_PWR_CTL2:
This feels a bit hackish
I can't think of a better way of detecting whether the chip reset. If we don't have control of the reset GPIO we can't force a reset. So it depends whether power turned off (also whether it dropped below the Vmin voltage at which a reset is triggered). The regulator framework can't tell us if it really turned off power and on ACPI systems the power is all controlled magically.
Right, hence the second part of the question - the general thing for drivers has just been to assume that the power might've gone out over suspend and behave as though it did unless we were using the device as a wake source. We could if required extend the existing regulator API notification stuff to generate notifications, though they'd not work when it's compiled out.
- is the cost of doing the cache sync really so> expensive that it'sworth the effort of trying to skip it?
It's not cost, it's about getting the correct values in the registers. If we call regcache_mark_dirty() it tells regmap that all the hardware registers have reset to default. Then regcache_sync() will skip writing cache values that are the register default value, assuming that the register already has this value. If the chip didn't reset, the registers could retain non-default values while the cache contains a dirty default value, in that case the register needs updating to match the cache.
(note BTW that at the point I queried the performance thing I'd not got as far as the magic bypassed write sequences)
So this is clearly a being done at the wrong level - it is needlessly complex, non-idiomatic and making fragile and non-obvious assumptions about how the cache code operates. The issue you've got is that the cache code is presenting interfaces for the common case where you'd only want to resync the cache in cases where the device has been reset but you've added code which bypasses the cache and pulls the device out of sync with the cache. You need to teach regmap about that, not try to hack around things in the driver code. But...
I tried to persuade myself that a cache value couldn't possibly change at any time from suspend() being called to resume disabling cache-only so I could safely accept default values being skipped. But that assumption made me very uncomfortable - I don't like leaving potential bugs in when its simple enough to prevent them.
...that's based on the assumption that this is all about the magic write sequences you're using for shutdown potentially conflicting with default values you've got in the cache? If it's not those then the assumption is that either the device was reset in which case it has reset values or the device was not reset and held it's previous value, in which case the cache sync is redundant. If we don't trust the device to reset cleanly for some reason then it's probably a bad idea to tell regmap about default values in the first place since even on initial boot we might actually be doing a soft reboot and the device could be holding values from whatever was running beforehand.
This clearly isn't simple, and other than those shutdown sequences or potential issues with the device not resetting cleanly this should be done by extending regmap so it explicitly knows what's going on. If it is those shutdown sequences then you're probably looking for something like doing a _sync_region() on the registers the sequences touch. Possibly a _sync_region() variant that writes everything requested, or just make sync_region() not skip defaults. Or just remove all the defaults from the driver, the sync will be a bit more expensive but that's hopefully fine. If it's not those shutdown sequences it should still be handled genericly but I'd really like to understand the scenario you're concerned about here, especially the fact that any issue will show up in this single register that the code is checking.
- if (cs42l42->suspended) {
mutex_unlock(&cs42l42->irq_lock);return IRQ_NONE;- }
Given that you're using disable_irq() to force the interrupt off (which is a bit rude but often the best one can do) how might we be getting an interrupt while suspended? This seems to indicate an error condition.
I may have misunderstood here, but the documentation says that enables/disables are nested. The interrupt starts out enabled right after calling request_threaded_irq(), so I expected that all users of the shared interrupt would have to call disable_irq() for it to actually get disabled (I put the call in on that basis that it does no harm). If disable_irq() forces the irq off even if it's shared then I'll remove it because as you say that would be rude.
Hrm, that may have changed since I last looked - if that's the case then I guess it's best not to warn which was what I was thinking here.
/** Only call regcache_mark_dirty() if cs42l42 reset, so* regcache_sync() writes all non-default cached values.* If it didn't reset we don't want to filter out syncing* dirty cache entries that have default value.* DISCHARGE_FILT==1 after suspend. If the cs42l42 reset* it will now be 0.*/
Something needs to tell regmap that the cache is dirty otherwise it
Regmap knows if it has dirty values that it hasn't written out to the hardware.
Well, apparently it doesn't since otherwise we could just do a resync.
won't sync anything, including the non-default register values? There's
That's fine. If the registers have not been reset they still have the value of every clean cache entry. Every dirty cache entry will be written out by regcache_sync().
What about the shutdown sequence - does that not touch cached registers?
currently an assumption coded in there that the cache is dirty because the device was reset and everything has default values.
Regmap only assumes that if regcache_mark_dirty() is called.
Right, but the point here is that coming out of suspend the driver is expected to be unconditionally marking the cache as dirty and doing a resync since we probably lost power. The code is trying to avoid that for reasons that are not at all apparent but I *think* are due to the bypassed writes in shutdown(), especially if you say it's not a performance thing. We shouldn't need to worry about there being non default states in the hardware.
The whole thing is just really confusing as it stands. Like I say I think this needs to express what it's trying to do clearly in both the regmap operations it's doing and the code in general, at the minute it's not clear looking at things what the goal is.
On Wed, Dec 01, 2021 at 10:08:26PM +0000, Mark Brown wrote:
On Wed, Dec 01, 2021 at 06:04:00PM +0000, Richard Fitzgerald wrote:
On 01/12/2021 16:32, Mark Brown wrote:
On Wed, Dec 01, 2021 at 03:36:48PM +0000, Richard Fitzgerald wrote:
...that's based on the assumption that this is all about the magic write sequences you're using for shutdown potentially conflicting with default values you've got in the cache? If it's not those then the assumption is that either the device was reset in which case it has reset values or the device was not reset and held it's previous value, in which case the cache sync is redundant.
This isn't quite accurate though, as whilst the device was suspended the user may have touched ALSA controls which will have updated the state of the cache. Thus the cache requires a sync regardless of if the hardware turned off.
I suspect we do need to have a think about the write sequences, but there is also a more general issue here. The sequence looks like this:
1) Device enters cache only mode. 2) User writes an ALSA control causing a register to return to its default value. 3) Device exits cache only and does a cache sync.
This flow works if the device was reset but not if the registers retained their value since the register written by the user will be at default in the cache, but not on the hardware. The cache sync code assumes the device returned to defaults.
It is often tricky to know if the device reset since the regulator could be shared and even if they arn't capacitance can play a part if the off time is very small. Usually we mandate a hard reset or use a soft reset if the hard isn't available. I think the soft reset has some issues on this chip but perhaps we should look into that more.
Given that you're using disable_irq() to force the interrupt off (which is a bit rude but often the best one can do) how might we be getting an interrupt while suspended? This seems to indicate an error condition.
I may have misunderstood here, but the documentation says that enables/disables are nested. The interrupt starts out enabled right after calling request_threaded_irq(), so I expected that all users of the shared interrupt would have to call disable_irq() for it to actually get disabled (I put the call in on that basis that it does no harm). If disable_irq() forces the irq off even if it's shared then I'll remove it because as you say that would be rude.
Hrm, that may have changed since I last looked - if that's the case then I guess it's best not to warn which was what I was thinking here.
No I am pretty sure you are correct here calling disable_irq will immediately disable the IRQ for everyone using it. The reference counting is on the other side, ie. if I call disable_irq 5 times I have to call enable_irq 5 times before it turns back on.
Thanks, Charles
On Thu, Dec 02, 2021 at 09:53:33AM +0000, Charles Keepax wrote:
On Wed, Dec 01, 2021 at 10:08:26PM +0000, Mark Brown wrote:
...that's based on the assumption that this is all about the magic write sequences you're using for shutdown potentially conflicting with default values you've got in the cache? If it's not those then the assumption is that either the device was reset in which case it has reset values or the device was not reset and held it's previous value, in which case the cache sync is redundant.
This isn't quite accurate though, as whilst the device was suspended the user may have touched ALSA controls which will have
updated the state of the cache. Thus the cache requires a sync regardless of if the hardware turned off.
Right, an actual description of an actual issue. Though how would they have touched the ALSA controls during system suspend? Userspace should halted before we start suspending devices so there shouldn't be anything new coming in from the application layer while the device is in cache only mode. The sound card as a whole should've been suspended first so nothing should be coming in from there either.
I suspect we do need to have a think about the write sequences, but there is also a more general issue here. The sequence looks like this:
- Device enters cache only mode.
- User writes an ALSA control causing a register to return to
its default value. 3) Device exits cache only and does a cache sync.
This is a thing that happens for runtime suspend but for runtime suspend we have good tracking of if the device lost power so we shouldn't just be marking the cache as dirty unconditionally. For system suspend there shouldn't be a need to worry about userspace changing anything, and anything coming in from the rest of the kernel should be very limited.
In any case this isn't something that should be hacked around in an individual driver, like I say whatever the driver is trying to do needs to be written in a clear and obvious fashion.
participants (3)
-
Charles Keepax -
Mark Brown -
Richard Fitzgerald