> -----Original Message-----
> From: Kalle Valo <kvalo@xxxxxxxxxx>
> Sent: Wednesday, March 15, 2023 5:52 PM
> To: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
> Cc: linux-wireless@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] wifi: rtw89: add counters of register-based H2C/C2H
>
> Ping-Ke Shih <pkshih@xxxxxxxxxxx> writes:
>
> > The register-based H2C/C2H are used to exchange information between driver
> > and firmware, but only apply to narrow area because its data size is
> > smaller than regular packet-based H2C/C2H.
> >
> > This kind of H2C/C2H must be paired. To identify if any H2C/C2H is missing,
> > update counters to help diagnose this kind of problems.
> >
> > Signed-off-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>
>
> [...]
>
> > +static inline void
> > +rtw89_write8_mask_add(struct rtw89_dev *rtwdev, u32 addr, u32 mask, u8 add)
> > +{
> > + u32 shift;
> > + u8 orig, set;
> > + u8 data;
> > +
> > + mask &= 0xff;
> > + shift = __ffs(mask);
> > +
> > + 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.
Ping-Ke
