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: <20191107111603.12317-1-yhchuang@realtek.com> <20191107111603.12317-4-yhchuang@realtek.com>
In-Reply-To: <20191107111603.12317-4-yhchuang@realtek.com>
From:   Chris Chiu <chiu@endlessm.com>
Date:   Wed, 13 Nov 2019 14:13:36 +0800
Message-ID: <CAB4CAwd6MDSyPTVGr-3T6nNTpJiJy1jEfNWtyriqoMJyV83jdw@mail.gmail.com>
Subject: Re: [PATCH 3/4] rtw88: pci: enable CLKREQ function if host supports it
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Brian Norris <briannorris@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 7, 2019 at 7:16 PM <yhchuang@realtek.com> wrote:
>
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> By Realtek's design, there are two HW modules associated for CLKREQ,
> one is responsible to follow the PCIE host settings, and another
> is to actually working on it. But the module that is actually working
> on it is default disabled, and driver should enable that module if
> host and device have successfully sync'ed with each other.
>
> The module is default disabled because sometimes the host does not
> support it, and if there is any incorrect settings (ex. CLKREQ# is
> not Bi-Direction), device can be lost and disconnected to the host.
>
> So driver should first check after host and device are sync'ed, and
> the host does support the function and set it in configuration
> space, then driver can turn on the HW module to working on it.
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
> ---
>  drivers/net/wireless/realtek/rtw88/pci.c | 83 ++++++++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/pci.h |  5 ++
>  2 files changed, 88 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index 6d1aa6f41e84..4fcef8a6fc42 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -1081,6 +1081,33 @@ static void rtw_dbi_write8(struct rtw_dev *rtwdev, u16 addr, u8 data)
>         WARN(flag, "failed to write to DBI register, addr=0x%04x\n", addr);
>  }
>
> +static int rtw_dbi_read8(struct rtw_dev *rtwdev, u16 addr, u8 *value)
> +{
> +       u16 read_addr = addr & BITS_DBI_ADDR_MASK;
> +       u8 flag;
> +       u8 cnt;
> +
> +       rtw_write16(rtwdev, REG_DBI_FLAG_V1, read_addr);
> +       rtw_write8(rtwdev, REG_DBI_FLAG_V1 + 2, BIT_DBI_RFLAG >> 16);
> +
> +       for (cnt = 0; cnt < RTW_PCI_WR_RETRY_CNT; cnt++) {
> +               flag = rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2);
> +               if (flag == 0)
> +                       break;
Why not just doing ' rtw_read8(rtwdev, read_addr);' and return here?
Then you don't need to check the flag != 0 at the following. It would
make the code cleaner and same expressive.

> +
> +               udelay(10);
> +       }
> +
> +       if (flag != 0) {
> +               WARN(1, "failed to read DBI register, addr=0x%04x\n", addr);
> +               return -EIO;
> +       }
> +
> +       read_addr = REG_DBI_RDATA_V1 + (addr & 3);
> +       *value = rtw_read8(rtwdev, read_addr);
> +       return 0;
> +}
> +
> --
> 2.17.1
>