RE: [PATCH v2 1/6] rtw88: use macro to check the current band

[Tony Chuang <yhchuang@realtek.com>]

> Subject: Re: [PATCH v2 1/6] rtw88: use macro to check the current band
> 
> A little late on this...
> 
> On Wed, Oct 16, 2019 at 7:39 PM Tony Chuang <yhchuang@realtek.com>
> wrote:
> > From: Brian Norris
> > > On Wed, Oct 16, 2019 at 5:33 AM <yhchuang@realtek.com> wrote:
> > > > index 4759d6a0ca6e..492a2bfc0d5a 100644
> > > > --- a/drivers/net/wireless/realtek/rtw88/main.h
> > > > +++ b/drivers/net/wireless/realtek/rtw88/main.h
> > > > @@ -58,6 +58,19 @@ struct rtw_hci {
> > > >         u8 bulkout_num;
> > > >  };
> > > >
> > > > +#define IS_CH_5G_BAND_1(channel) ((channel) >= 36 && (channel <=
> 48))
> > > > +#define IS_CH_5G_BAND_2(channel) ((channel) >= 52 && (channel <=
> 64))
> > > > +#define IS_CH_5G_BAND_3(channel) ((channel) >= 100 && (channel <=
> > > 144))
> > > > +#define IS_CH_5G_BAND_4(channel) ((channel) >= 149 && (channel <=
> > > 177))
> > >
> > > There are channels between 48 and 52, 64 and 100, and 144 and 149.
> > > What are you doing with those?
> >
> > These devices are not supporting those channels you mentioned.
> > So I hope if some unsupported channels are used, they should hit the
> > "else" case, or throw such a warn.
> 
> Maybe that argument makes sense on its own, but see below:
> 
> > > > +#define IS_CH_5G_BAND_MID(channel) \
> > > > +       (IS_CH_5G_BAND_2(channel) || IS_CH_5G_BAND_3(channel))
> > > > +
> > > > +#define IS_CH_2G_BAND(channel) ((channel) <= 14)
> > > > +#define IS_CH_5G_BAND(channel) \
> > > > +       (IS_CH_5G_BAND_1(channel) || IS_CH_5G_BAND_2(channel)
> || \
> > > > +        IS_CH_5G_BAND_3(channel) ||
> IS_CH_5G_BAND_4(channel))
> > >
> > > Given the above (you have major holes in 5G_BAND{1,2,3,4}), this macro
> > > seems like a regression.
> 
> I still think it's a terrible idea to write an intentionally
> misleading macro named "IS 5G BAND" that returns false for 5G
> channels. It just gives you a nice way to stub your toe if you ever
> have chips that do support these channels.
> 

You're probably right, but I really don't think those 802.11ac devices are
going to support these channels, unless rtw88 is going to add support
for 802.11ax devices. And I'll take care of it when it comes.

At least the current 802.11ac devices such as 8821C/8822B/8822C do
not support it.

Yan-Hsuan