RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()

[Ping-Ke Shih <pkshih@realtek.com>]

Hi Geert,

Ping-Ke Shih wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Hi Ping-Ke,
> >
> > On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > > > The power value and enable bit fields can be not consecutive mask, but
> > > > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
> > > > > consecutive bit mask. Therefore, change the code accordingly.
> > > > >
> > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >
> > > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h
> > > > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> > > > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
> > > > >         return 0;
> > > > >  }
> > > > >
> > > > > +static inline
> > > > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
> > > > > +                               u32 mask_en, bool cond)
> > > > > +{
> > > > > +       u32 wrt = u32_encode_bits(val, mask);
> > > >
> > > > Nit: you could do without this variable...

Can you change u32_encode_bits() to field_prep() you are going to add?
And take this patch (v1) with your suggestion into your patchset.
Then the code looks better and thing can be smooth.

I will drop my v2 that looks not good. 

> > > >
> > > > > +       u32 val32;
> > > > > +       int ret;
> > > > > +
> > > > > +       if (cond)
> > > > > +               wrt |= mask_en;
> > > > > +
> > > > > +       ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
> > > > > +       if (ret)
> > > > > +               return;
> > > > > +
> > > > > +       val32 &= ~(mask | mask_en);
> > > > > +       val32 |= wrt;
> > > >
> > > > val32 |= u32_encode_bits(val, mask);
> > > > if (cond)
> > > >         cal32 |= mask_en;
> > >
> > > With this change, ARCH arm is failed to build (x86 is well):
> > >
> > > In file included from /build/rtw89/core.h:9,
> > >                  from /build/rtw89/coex.h:8,
> > >                  from /build/rtw89/rtw8851b.c:5:
> > > In function 'field_multiplier',
> > >     inlined from 'field_mask' at ./include/linux/bitfield.h:170:17,
> > >     inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1,
> > >     inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11:
> > > ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad
> > bitfield mask
> > >   165 |                 __bad_mask();
> > >       |                 ^~~~~~~~~~~~
> > > In function 'field_multiplier',
> >
> > Hmm...
> >
> > Note that u32_encode_bits() really requires a constant mask, just
> > like FIELD_PREP().  So probably the compiler can no longer deduce it
> > is called with a constant after restructuring the code...
> 
> Do you think I need to add a macro to generate mask with u32_encode_bits()
> and then call this inline function?
> 
> Or, compiler can always deduce it in the inline function, and current version
> is okay?
>