Re: [PATCH] wifi: rtw89: add counters of register-based H2C/C2H

[Kalle Valo <kvalo@kernel.org>]

Ping-Ke Shih <pkshih@realtek.com> writes:

>> > +     orig = rtw89_read8(rtwdev, addr);
>> > +     data = ((orig & mask) >> shift) + add;
>> > +     set = (orig & ~mask) | ((data << shift) & mask);
>> > +     rtw89_write8(rtwdev, addr, set);
>> > +}
>> 
>> This function has a lot of shifting etc which feels like reinventing the
>> wheel, doesn't linux/bitfield.h contain what you need? For example,
>> u32_get_bits() and u32_replace_bits()?
>> 
>
> The mask argument of u32_get_bits() and his friends should be const, but our
> usage could be a variable. For now, we have only one use case that the mask is
> definitely const, but I remember it could lead some warning if we don't define
> this as 'static __always_inline'.
>
>
> My original thought to implement this function is 
>
> rtw89_write8_mask_add(struct rtw89_dev *rtwdev, u32 addr, u32 mask, u8 add)
> {
> 	u8 tmp;
>
> 	tmp = rtw89_read8_mask(rtwdev, addr, mask);
> 	tmp += add;
> 	rtw89_write8_mask(rtwdev, addr, mask, tmp);
> }
>
> But, this needs three IO (two reading and one writing IO), so I implement this
> a little odd patch. 
>
>
> I'm thinking I can have another implementation that adds variables to maintain
> counters by driver, and then I only need existing rtw89_write8_mask() to update
> counters instead. Therefore, no need rtw89_write8_mask_add(). I will use this
> method by v2.

Sounds good, thanks!

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches