Ping-Ke Shih <pkshih@xxxxxxxxxxx> 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
