From:   Kalle Valo <kvalo@kernel.org>
To:     Ping-Ke Shih <pkshih@realtek.com>
Cc:     "linux-wireless\@vger.kernel.org" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH] wifi: rtw89: add counters of register-based H2C/C2H
References: <20230310035943.49020-1-pkshih@realtek.com>
        <87r0tqmje1.fsf@kernel.org>
        <485422aa276d49878278592861a103f8@realtek.com>
Date:   Fri, 17 Mar 2023 09:17:50 +0200
In-Reply-To: <485422aa276d49878278592861a103f8@realtek.com> (Ping-Ke Shih's
        message of "Wed, 15 Mar 2023 11:56:03 +0000")
Message-ID: <87ilezn8wx.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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