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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH 3/4] rtw88: pci: enable CLKREQ function if host supports it
From:   Chris Chiu <chiu@endlessm.com>
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D1927EEB@RTITMBSVM04.realtek.com.tw>
Date:   Wed, 13 Nov 2019 15:46:41 +0800
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Brian Norris <briannorris@chromium.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A25CD5DF-9227-46DB-BBFF-0153F37723E9@endlessm.com>
References: <20191107111603.12317-1-yhchuang@realtek.com> <20191107111603.12317-4-yhchuang@realtek.com> <CAB4CAwd6MDSyPTVGr-3T6nNTpJiJy1jEfNWtyriqoMJyV83jdw@mail.gmail.com> <F7CD281DE3E379468C6D07993EA72F84D1927EEB@RTITMBSVM04.realtek.com.tw>
To:     Tony Chuang <yhchuang@realtek.com>
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk


Tony Chuang <yhchuang@realtek.com> =E6=96=BC 2019=E5=B9=B411=E6=9C=8813=E6=97=
=A5 =E4=B8=8B=E5=8D=883:16 =E5=AF=AB=E9=81=93=EF=BC=9A

>>> On Thu, Nov 7, 2019 at 7:16 PM <yhchuang@realtek.com> wrote:
>>>=20
>>> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>>>=20
>>> 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.
>>>=20
>>> 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.
>>>=20
>>> 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.
>>>=20
>>> 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(+)
>>>=20
>>> 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=3D0x%04x\n",
>> addr);
>>> }
>>>=20
>>> +static int rtw_dbi_read8(struct rtw_dev *rtwdev, u16 addr, u8 *value)
>>> +{
>>> +       u16 read_addr =3D 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 =3D 0; cnt < RTW_PCI_WR_RETRY_CNT; cnt++) {
>>> +               flag =3D rtw_read8(rtwdev, REG_DBI_FLAG_V1 + 2);
>>> +               if (flag =3D=3D 0)
>>> +                       break;
>> Why not just doing ' rtw_read8(rtwdev, read_addr);' and return here?
>> Then you don't need to check the flag !=3D 0 at the following. It would
>> make the code cleaner and same expressive.
>=20
> Maybe it would look cleaner, but you need to add statements when
> 'if (flag =3D=3D 0)', then the indents will be deeper.
> Also you will need to return 0 at the end if 'flag =3D=3D 0'.
>=20

Yup. And I believe it=E2=80=99s still under 80 characters so I think I=E2=80=
=99m cool with it.

> So you still think it's cleaner to write it that way? If so, I can try to s=
end
> v2 for it.
>>=20
>>> +
>>> +               udelay(10);
>>> +       }
>>> +
>>> +       if (flag !=3D 0) {
>>> +               WARN(1, "failed to read DBI register, addr=3D0x%04x\n",
>> addr);
>>> +               return -EIO;
>>> +       }
>>> +
>>> +       read_addr =3D REG_DBI_RDATA_V1 + (addr & 3);
>>> +       *value =3D rtw_read8(rtwdev, read_addr);
>>> +       return 0;
>>> +}
>>> +
>>> --
>>> 2.17.1
>>>=20
>>=20
>=20
> Yan-Hsuan