RE: [PATCH v5 03/11] wifi: rtlwifi: Add rtl8192du/hw.{c,h}

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

Bitterblue Smith <rtl8821cerfe2@gmail.com> wrote:
> These contain mostly hardware init/deinit routines for RTL8192DU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>

[...]

> ---
>  .../wireless/realtek/rtlwifi/rtl8192du/hw.c   | 1232 +++++++++++++++++
>  .../wireless/realtek/rtlwifi/rtl8192du/hw.h   |   24 +
>  2 files changed, 1256 insertions(+)
>  create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.c
>  create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.h
> 
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.c
> b/drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.c
> new file mode 100644
> index 000000000000..14a3e4685dee
> --- /dev/null
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192du/hw.c
> @@ -0,0 +1,1232 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright(c) 2009-2012  Realtek Corporation.*/

Please change year to 2024 across this patchset. 

> +
> +#include "../wifi.h"
> +#include "../efuse.h"
> +#include "../base.h"
> +#include "../regd.h"
> +#include "../cam.h"
> +#include "../ps.h"
> +#include "../pci.h"
> +#include "../usb.h"
> +#include "../rtl8192d/reg.h"
> +#include "../rtl8192d/def.h"
> +#include "../rtl8192d/dm_common.h"
> +#include "../rtl8192d/fw_common.h"
> +#include "../rtl8192d/hw_common.h"
> +#include "../rtl8192d/phy_common.h"
> +#include "phy.h"
> +#include "dm.h"
> +#include "fw.h"
> +#include "led.h"
> +#include "hw.h"
> +#include "trx.h"
> +
> +static void _rtl92de_set_bcn_ctrl_reg(struct ieee80211_hw *hw,
> +                                     u8 set_bits, u8 clear_bits)

Here is 8192du, so prefix should not 8192de. Also rtl92d_linked_set_reg() should
be used in common file, so we should use rtl92du_linked_set_reg() instead.
Please apply this rule across patchset. 

> +{
> +       struct rtl_usb *rtlusb = rtl_usbdev(rtl_usbpriv(hw));
> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> +
> +       rtlusb->reg_bcn_ctrl_val |= set_bits;
> +       rtlusb->reg_bcn_ctrl_val &= ~clear_bits;
> +       rtl_write_byte(rtlpriv, REG_BCN_CTRL, (u8)rtlusb->reg_bcn_ctrl_val);
> +}
> +

[...]

> +
> +void rtl92du_set_hw_reg(struct ieee80211_hw *hw, u8 variable, u8 *val)
> +{
> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> +       struct rtl_mac *mac = rtl_mac(rtlpriv);
> +
> +       switch (variable) {
> +       case HW_VAR_AC_PARAM: {
> +               rtl92d_dm_init_edca_turbo(hw);
> +               break;
> +       }

No need parenthesis for this single statement. 

[...]


> +
> +static bool _rtl92de_llt_table_init(struct ieee80211_hw *hw, u8 txpktbuf_bndy)
> +{
> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> +       unsigned short i;
> +       bool status;
> +       u8 maxpage;
> +
> +       if (rtlpriv->rtlhal.macphymode == SINGLEMAC_SINGLEPHY)
> +               maxpage = 255;
> +       else
> +               maxpage = 127;
> +
> +       for (i = 0; i < (txpktbuf_bndy - 1); i++) {
> +               status = rtl92de_llt_write(hw, i, i + 1);
> +               if (!status)
> +                       return status;
> +       }
> +
> +       /* end of list */
> +       status = rtl92de_llt_write(hw, txpktbuf_bndy - 1, 0xFF);
> +       if (!status)
> +               return status;
> +
> +       /* Make the other pages as ring buffer */
> +       /* This ring buffer is used as beacon buffer if we */
> +       /* config this MAC as two MAC transfer. */
> +       /* Otherwise used as local loopback buffer.  */

Be single one block comment. 

"... loopback buffer.  " contains two trailing spaces. 

[...]

> +
> +int rtl92du_hw_init(struct ieee80211_hw *hw)
> +{
> +       struct rtl_usb_priv *usb_priv = rtl_usbpriv(hw);
> +       struct rtl_usb *rtlusb = rtl_usbdev(usb_priv);
> +       struct rtl_priv *rtlpriv = rtl_priv(hw);
> +       struct rtl_ps_ctl *ppsc = rtl_psc(rtlpriv);
> +       struct rtl_hal *rtlhal = rtl_hal(rtlpriv);
> +       struct rtl_mac *mac = rtl_mac(rtlpriv);
> +       struct rtl_phy *rtlphy = &rtlpriv->phy;
> +       u8 val8, txpktbuf_bndy;
> +       unsigned long flags;
> +       int err, i;
> +       u32 val32;
> +       u16 val16;
> +
> +       /* As this function can take a very long time
> +        * and can be called with irqs disabled, reenable the irqs
> +        * to let the other devices continue being serviced.
> +        *
> +        * It is safe doing so since our own interrupts will only be enabled
> +        * in a subsequent step.
> +        */
> +       local_save_flags(flags);
> +       local_irq_enable();

I think you just copy and paste this chunk. I wonder if USB has the same behavior?
Driver force to enable IRQ is weird to me. Please check I this is really called
with irq disabled? If not, we can remove them.

[...]

> +       local_irq_disable();
> +       local_irq_restore(flags);
> +
> +       return err;
> +}
> +