Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191025093345.22643-1-yhchuang@realtek.com> <20191025093345.22643-4-yhchuang@realtek.com>
In-Reply-To: <20191025093345.22643-4-yhchuang@realtek.com>
From:   Chris Chiu <chiu@endlessm.com>
Date:   Fri, 25 Oct 2019 18:51:34 +0800
Message-ID: <CAB4CAwf_Y0tMG37nZDtsU9FmACaOB=eALCNCazVk1kbAvOE26Q@mail.gmail.com>
Subject: Re: [PATCH 3/6] rtw88: use a module parameter to control LPS enter
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Brian Norris <briannorris@chromium.org>,
        g.schlmm@googlemail.com
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Fri, Oct 25, 2019 at 5:33 PM <yhchuang@realtek.com> wrote:
>
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> If the number of packets is less than the LPS threshold, driver
> can then enter LPS mode.
> And driver used to take RTW_LPS_THRESHOLD as the threshold. As
> the macro can not be changed after compiled, use a parameter
> instead.
>
> The larger of the threshold, the more traffic required to leave
> power save mode, responsive time could be longer, but also the
> power consumption could be lower.
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/main.c | 7 +++++--
>  drivers/net/wireless/realtek/rtw88/ps.h   | 2 --
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
> index 7c1b89c4fb6c..bff8a0b129d9 100644
> --- a/drivers/net/wireless/realtek/rtw88/main.c
> +++ b/drivers/net/wireless/realtek/rtw88/main.c

> @@ -199,8 +202,8 @@ static void rtw_watch_dog_work(struct work_struct *work)
>         if (busy_traffic != test_bit(RTW_FLAG_BUSY_TRAFFIC, rtwdev->flags))
>                 rtw_coex_wl_status_change_notify(rtwdev);
>
> -       if (stats->tx_cnt > RTW_LPS_THRESHOLD ||
> -           stats->rx_cnt > RTW_LPS_THRESHOLD)
> +       if (stats->tx_cnt > rtw_lps_threshold ||
> +           stats->rx_cnt > rtw_lps_threshold)
>                 ps_active = true;
>         else
>                 ps_active = false;

The naming of 'ps_active' is a bit confusing. Per the commit message,
it will leave LPS
it tx/rx count > threshold. But I'll be misled by the name ps_active.
Does it mean the
current condition is PS active and ready to power sleep? I'd like to
rename it to old-fashioned
'lps_enter' to represent the action that would be taken. It would be
easier for me to understand.

Chris

> --
> 2.17.1
>