From:   Kalle Valo <kvalo@kernel.org>
To:     Carl Huang <cjhuang@codeaurora.org>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org
Subject: Re: [PATCH 1/6] ath11k: Add basic WoW functionalities
References: <20211011193750.4891-1-cjhuang@codeaurora.org>
        <20211011193750.4891-2-cjhuang@codeaurora.org>
Date:   Thu, 09 Dec 2021 16:48:18 +0200
In-Reply-To: <20211011193750.4891-2-cjhuang@codeaurora.org> (Carl Huang's
        message of "Mon, 11 Oct 2021 15:37:45 -0400")
Message-ID: <87czm525sd.fsf@codeaurora.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Carl Huang <cjhuang@codeaurora.org> writes:

> Implement basic WoW functionalities such as magic-packet, disconnect
> and pattern. The logic is very similar to ath10k.
>
> When WoW is configured, ath11k_core_suspend and ath11k_core_resume
> are skipped as WoW configuration and hif suspend/resume are done in
> ath11k_wow_op_suspend() and ath11k_wow_op_resume().
>
> Tested-on: QCA6390 hw2.0 PCI WLAN.HST.1.0.1-01740-QCAHSTSWPLZ_V2_TO_X86-1
>
> Signed-off-by: Carl Huang <cjhuang@codeaurora.org>
> Signed-off-by: Baochen Qiang <bqiang@codeaurora.org>

[...]

>  int ath11k_core_suspend(struct ath11k_base *ab)
>  {
>  	int ret;
> +	struct ath11k *ar;
>  
>  	if (!ab->hw_params.supports_suspend)
>  		return -EOPNOTSUPP;
>  
> +	/* so far single_pdev_only chips have supports_suspend as true
> +	 * and only the first pdev is valid.
> +	 */
> +	ar = ab->pdevs[0].ar;

I'm a bit worried about this assumption, it might change in the future.
What about a helper like this to add a test for it:

static inline struct ath11k_pdev *ath11k_core_get_single_pdev()
{
        WARN_ON(!ab->hw_params.single_pdev_only);

        return ab->pdevs[0];
}

>  int ath11k_core_resume(struct ath11k_base *ab)
>  {
>  	int ret;
> +	struct ath11k *ar;
>  
>  	if (!ab->hw_params.supports_suspend)
>  		return -EOPNOTSUPP;
>  
> +	/* so far signle_pdev_only chips have supports_suspend as true
> +	 * and only the first pdev is valid.
> +	 */
> +	ar = ab->pdevs[0].ar;
> +	if (!ar || ar->state != ATH11K_STATE_OFF)
> +		return 0;

The same helper could be used here.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches