> On Apr 6, 2020, at 22:03, Kalle Valo <kvalo@xxxxxxxxxxxxxx> wrote: > > 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. Yes that will do. Didn't notice the recently added macro. Will send v2. Kai-Heng > > -- > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
