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"? :)
> 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)?
> @@ -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.
> + azx_writew(chip, WAKEEN, azx_readw(chip, WAKEEN) &
> + ~STATESTS_INT_MASK);
This would fit to one line now.
Br, Kai
Hi Kai,
> On Oct 27, 2020, at 19:38, Kai Vehmanen <[email protected]> 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