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.
Attachment:
signature.asc
Description: PGP signature