Messages in this thread | | | From | Kalle Valo <> | Subject | Re: [PATCH] rtw88: Add delay on polling h2c command status bit | Date | Mon, 06 Apr 2020 17:03:00 +0300 |
| |
Kai-Heng Feng <kai.heng.feng@canonical.com> writes:
>> On Apr 6, 2020, at 21:24, Kalle Valo <kvalo@codeaurora.org> wrote: >> >> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >> >>>> On Apr 6, 2020, at 20:17, Kalle Valo <kvalo@codeaurora.org> wrote: >>>> >>>> Kai-Heng Feng <kai.heng.feng@canonical.com> writes: >>>> >>>>> --- a/drivers/net/wireless/realtek/rtw88/hci.h >>>>> +++ b/drivers/net/wireless/realtek/rtw88/hci.h >>>>> @@ -253,6 +253,10 @@ rtw_write8_mask(struct rtw_dev *rtwdev, u32 >>>>> addr, u32 mask, u8 data) >>>>> rtw_write8(rtwdev, addr, set); >>>>> } >>>>> >>>>> +#define rr8(addr) rtw_read8(rtwdev, addr) >>>>> +#define rr16(addr) rtw_read16(rtwdev, addr) >>>>> +#define rr32(addr) rtw_read32(rtwdev, addr) >>>> >>>> For me these macros reduce code readability, not improve anything. They >>>> hide the use of rtwdev variable, which is evil, and a name like rr8() is >>>> just way too vague. Please keep the original function names as is. >>> >>> The inspiration is from another driver. >>> readx_poll_timeout macro only takes one argument for the op. >>> Some other drivers have their own poll_timeout implementation, >>> and I guess it makes sense to make one specific for rtw88. >> >> I'm not even understanding the problem you are tying to fix with these >> macros. The upstream philosopyhy is to have the source code readable and >> maintainable, not to use minimal number of characters. There's a reason >> why we don't name our functions a(), b(), c() and so on. > > The current h2c polling doesn't have delay between each interval, so > the polling is too fast and the following logic considers it's a > timeout. > The readx_poll_timeout() macro provides a generic mechanism to setup > an interval delay and timeout which is what we need here. > However readx_poll_timeout only accepts one parameter which usually is > memory address, while we need to pass both rtwdev and address. > > So if hiding rtwdev is evil, we can roll our own variant of > readx_poll_timeout() to make the polling readable.
Can't you do:
ret = read_poll_timeout(rtw_read8, box_state, !((box_state >> box) & 0x1), 100, 3000, false, rtw_dev, REG_HMETFR);
No ugly macros needed and it should function the same. But I did not test this in any way, so no idea if it even compiles.
-- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
| |