A little late on this...
On Wed, Oct 16, 2019 at 7:39 PM Tony Chuang <yhchuang@xxxxxxxxxxx> wrote:
> From: Brian Norris
> > On Wed, Oct 16, 2019 at 5:33 AM <yhchuang@xxxxxxxxxxx> 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.
Brian
