Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> writes: >> On Apr 6, 2020, at 21:24, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote: >> >> Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> writes: >> >>>> On Apr 6, 2020, at 20:17, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote: >>>> >>>> Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx> 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