RE: [PATCH 1/4] wifi: rtw88: 8821cu: Fix firmware upload fail

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

> -----Original Message-----
> From: Sascha Hauer <sha@pengutronix.de>
> Sent: Wednesday, February 28, 2024 4:56 PM
> To: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Cc: linux-wireless@vger.kernel.org; Ping-Ke Shih <pkshih@realtek.com>; Sean Mollet <sean@malmoset.com>
> Subject: Re: [PATCH 1/4] wifi: rtw88: 8821cu: Fix firmware upload fail
> 
> On Tue, Feb 27, 2024 at 06:27:51PM +0200, Bitterblue Smith wrote:
> > Adding Sean Mollet because I forgot earlier.
> >
> > On 27/02/2024 14:56, Sascha Hauer wrote:
> > > On Tue, Feb 27, 2024 at 02:18:20PM +0200, Bitterblue Smith wrote:
> > >> +  if (addr < 0xFE00) {
> > >> +          if (addr <= 0xff)
> > >> +                  current_reg_sec = REG_ON_SEC;
> > >> +          else if (0x1000 <= addr && addr <= 0x10ff)
> > >> +                  current_reg_sec = REG_ON_SEC;
> > >> +          else
> > >> +                  current_reg_sec = REG_OFF_SEC;
> > >> +  } else {
> > >> +          current_reg_sec = REG_LOCAL_SEC;
> > >> +  }
> > >> +
> > >> +  if (current_reg_sec != REG_ON_SEC)
> > >> +          return;
> > >
> > > Is there something we want to do with current_reg_sec == REG_LOCAL_SEC
> > > or current_reg_sec == REG_OFF_SEC later? If not the above could be
> > > rewritten as:
> > >
> > >     if (addr > 0xff && addr < 0x1000)
> > >             return;
> > >     if (addr > 0x10ff)
> > >             return;
> > >
> > >     ...
> >
> > Dunno, I just copied the code from the other drivers:
> >
> >
> https://github.com/morrownr/8821cu-20210916/blob/5b39398e2de146edeb76716420f3288f508bea61/os_dep/linux
> /usb_ops_linux.c#L171
> 
> Ok, nothing is done with current_reg_sec here as well, so I suggest
> rewriting the check like I suggested.

I also prefer rewriting the code, but we can add comments to describe there
are three sections: 
  1. on (0x00~0xFF;0x1000~0x10FF): this section is always powered on
  2. off (< 0xFE00; but not on section): this section could be powered off
  3. local (>= 0xFE00): usb specific registers section 

Since only on-section needs special deal, maybe positively listing register
ranges would be clear, like

	bool reg_on_sec = false;

	if ((addr >= 0x00 && addr <= 0xFF) ||
	    (addr >= 0x1000 && addr <= 0x10FF))
		reg_on_sec = true;

	if (!reg_on_sec)
		return;

Ping-Ke