Date:   Thu, 2 Dec 2021 09:53:33 +0000
From:   Charles Keepax <ckeepax@opensource.cirrus.com>
To:     Mark Brown <broonie@kernel.org>
CC:     Richard Fitzgerald <rf@opensource.cirrus.com>,
        <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>,
        <patches@opensource.cirrus.com>
Subject: Re: [PATCH] ASoC: cs42l42: Implement system suspend
Message-ID: <20211202095333.GK18506@ediswmail.ad.cirrus.com>
References: <20211201153648.17259-1-rf@opensource.cirrus.com>
 <YaejghraYE6lD7FD@sirena.org.uk>
 <87930df9-6220-807e-a655-1c7d7ec020ab@opensource.cirrus.com>
 <YafyWnVA1J1rgCf1@sirena.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <YafyWnVA1J1rgCf1@sirena.org.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Precedence: bulk

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