Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH v2 3/4] ALSA: hda: Separate runtime and system suspend
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <alpine.DEB.2.22.394.2010271317430.864696@eliteleevi.tm.intel.com>
Date:   Tue, 27 Oct 2020 20:04:16 +0800
Cc:     tiwai@suse.com, alsa-devel@alsa-project.org,
        linux-kernel@vger.kernel.org, hui.wang@canonical.com
Content-Transfer-Encoding: 8BIT
Message-Id: <468FD7B1-A6A6-4D0B-B27B-B66696424A48@canonical.com>
References: <20201027054001.1800-1-kai.heng.feng@canonical.com>
 <20201027054001.1800-4-kai.heng.feng@canonical.com>
 <alpine.DEB.2.22.394.2010271317430.864696@eliteleevi.tm.intel.com>
To:     Kai Vehmanen <kai.vehmanen@linux.intel.com>
Precedence: bulk

Hi Kai,

> On Oct 27, 2020, at 19:38, Kai Vehmanen <kai.vehmanen@linux.intel.com> wrote:
> 
> Hi,
> 
> thanks, this looks like a good improvement! Some minor notes:
> 
> On Tue, 27 Oct 2020, Kai-Heng Feng wrote:
> 
>> Both pm_runtime_force_suspend() and pm_runtime_force_resume() have
>> some implicit checks, so it can make code flow more straightfoward if we
>> separate runtime and systemd suspend callbacks.
> 
> straightforward -> straightforward
> 
> and systemd? Maybe just "system suspend"? :)

Typos :)
Will update them in v3.

> 
>> While at it, also remove AZX_DCAPS_SUSPEND_SPURIOUS_WAKEUP, as the
>> original bug commit a6630529aecb ("ALSA: hda: Workaround for spurious
>> wakeups on some Intel platforms") solves doesn't happen with this
>> patch.
> 
> Hmm, so this was gone already with the v1 version (so not related to 
> programming the WAKEEN when going to system suspend)?

Yes, I was worried that this cleanup may regress the user again.

> 
>> @@ -143,6 +143,7 @@ struct azx {
>> 	unsigned int align_buffer_size:1;
>> 	unsigned int region_requested:1;
>> 	unsigned int disabled:1; /* disabled by vga_switcheroo */
>> +	unsigned int prepared:1;
> 
> I wonder if "pm_prepared" would be better as ALSA API has a prepare method 
> as well and this is not related. OTOH, if ok to Takashi, ok for me as 
> well. 

Sure, I think we should use different terms.

> 
>> +	azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
>> +		   ~STATESTS_INT_MASK);
> 
> This would fit to one line now. 

Ok, will concat the lines.

Kai-Heng

> 
> Br, Kai