lkml.org 
[lkml]   [2021]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ASoC: cs42l42: Implement system suspend
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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2021-12-01 17:32    [W:0.068 / U:0.332 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site