RE: [PATCH 1/2] wifi: rtl8xxxu: Clean up some messy ifs

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

> -----Original Message-----
> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> Sent: Sunday, April 9, 2023 10:11 PM
> To: Ping-Ke Shih <pkshih@realtek.com>; linux-wireless@vger.kernel.org
> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>
> Subject: Re: [PATCH 1/2] wifi: rtl8xxxu: Clean up some messy ifs
> 
> On 06/04/2023 04:16, Ping-Ke Shih wrote:
> >
> >
> >> -----Original Message-----
> >> From: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >> Sent: Saturday, April 1, 2023 4:17 AM
> >> To: linux-wireless@vger.kernel.org
> >> Cc: Jes Sorensen <Jes.Sorensen@gmail.com>; Ping-Ke Shih <pkshih@realtek.com>
> >> Subject: [PATCH 1/2] wifi: rtl8xxxu: Clean up some messy ifs
> >>
> >> Add some new members to rtl8xxxu_fileops and use them instead of
> >> checking priv->rtl_chip.
> >>
> >> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> >> ---
> >
> > [...]
> >
> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> index c152b228606f..62dd53a57659 100644
> >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> >> @@ -1916,7 +1916,7 @@ static int rtl8xxxu_start_firmware(struct rtl8xxxu_priv *priv)
> >>         /*
> >>          * Init H2C command
> >>          */
> >> -       if (priv->rtl_chip == RTL8723B || priv->rtl_chip == RTL8188F || priv->rtl_chip == RTL8710B)
> >> +       if (priv->fops->init_reg_hmtfr)
> >>                 rtl8xxxu_write8(priv, REG_HMTFR, 0x0f);
> >>  exit:
> >>         return ret;
> >> @@ -3864,11 +3864,8 @@ void rtl8xxxu_init_burst(struct rtl8xxxu_priv *priv)
> >>         rtl8xxxu_write8(priv, REG_HT_SINGLE_AMPDU_8723B, val8);
> >>
> >>         rtl8xxxu_write16(priv, REG_MAX_AGGR_NUM, 0x0c14);
> >> -       if (priv->rtl_chip == RTL8723B || priv->rtl_chip == RTL8710B)
> >> -               val8 = 0x5e;
> >> -       else if (priv->rtl_chip == RTL8188F)
> >> -               val8 = 0x70; /* 0x5e would make it very slow */
> >> -       rtl8xxxu_write8(priv, REG_AMPDU_MAX_TIME_8723B, val8);
> >> +       rtl8xxxu_write8(priv, REG_AMPDU_MAX_TIME_8723B,
> >> +                       priv->fops->ampdu_max_time);
> >
> > Should it be
> >
> > if (priv->fops->ampdu_max_time)
> >     val8 = priv->fops->ampdu_max_time;>
> > rtl8xxxu_write8(priv, REG_AMPDU_MAX_TIME_8723B, val8); // this line doesn't change?
> >
> > Because originally val8 is read from REG_HT_SINGLE_AMPDU_8723B and add
> > HT_SINGLE_AMPDU_ENABLE bit.
> 
> No, the value read from REG_HT_SINGLE_AMPDU_8723B is not supposed to be
> written to REG_AMPDU_MAX_TIME_8723B. And it never was, because only
> RTL8723B, RTL8710B, and RTL8188F use this function. This was clearer in
> the original version of the code, when it was used only by RTL8723B:
> 
>                 val8 = rtl8xxxu_read8(priv, REG_HT_SINGLE_AMPDU_8723B);
>                 val8 |= BIT(7);
>                 rtl8xxxu_write8(priv, REG_HT_SINGLE_AMPDU_8723B, val8);
> 
>                 rtl8xxxu_write16(priv, REG_MAX_AGGR_NUM, 0x0c14);
>                 rtl8xxxu_write8(priv, REG_AMPDU_MAX_TIME_8723B, 0x5e);
> 

Oops. Somehow I misunderstood the code. Sorry for the noise.