[lvc-project] [PATCH rtw-next v3 5/9] wifi: rtw89: implement C2H TX report handler

[Fedor Pchelkin]

On Wed, 22. Oct 06:21, Ping-Ke Shih wrote:
> Fedor Pchelkin <pchelkin at ispras.ru> wrote:
> > @@ -5457,6 +5457,35 @@ rtw89_mac_c2h_mcc_status_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32
> >         rtw89_complete_cond(&rtwdev->mcc.wait, cond, &data);
> >  }
> > 
> > +static void
> > +rtw89_mac_c2h_tx_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> > +{
> > +       u8 sw_define, tx_status, data_txcnt;
> > +
> > +       if (rtwdev->chip->chip_id == RTL8922A) {
> 
> Add a chip_ops c2h_tx_rpt? Then, no need chip_id checking, and reduce line
> length (normally we prefer shorter than 80 or 90 characters; over 100 characters
> isn't a good idea).
> 
> Maybe this is because you want to store the status into local variables.
> With a chip_ops, you should define another struct to store them.
> 
> Or, you just keep it as was, but wrap lines to be shorter, and give shorter
> naming. For example,
>   - rpt_v2 -> v2
> 
>   - data_txcnt -> txcnt
> 
> 		if (rtwdev->chip->chip_id == RTL8852C)
> 			txcnt = le32_get_bits(rpt->w5,
> 					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
> 		else
> 			txcnt = le32_get_bits(rpt->w5,
> 					              RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> 

The chip_ops variant is more abstract (and good in such a way) but I don't
think it's worth mangling with modifying struct chip_ops of each chip and
adding new structs as we probably won't need to scale or expand
rtw89_mac_c2h_tx_rpt() further unless some another V3 format is supported.
Also other mac C2H functions tend to check chip_id in place if they need.

I'll try to make the lines shorter - the longest one will be 86 symbols.

> 
> > +               const struct rtw89_c2h_mac_tx_rpt_v2 *rpt_v2;
> > +
> > +               rpt_v2 = (const struct rtw89_c2h_mac_tx_rpt_v2 *)c2h->data;
> > +               sw_define = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_SW_DEFINE_V2);
> > +               tx_status = le32_get_bits(rpt_v2->w12, RTW89_C2H_MAC_TX_RPT_W12_TX_STATE_V2);
> > +               data_txcnt = le32_get_bits(rpt_v2->w14, RTW89_C2H_MAC_TX_RPT_W14_DATA_TX_CNT_V2);
> > +       } else {
> > +               const struct rtw89_c2h_mac_tx_rpt *rpt;
> > +
> > +               rpt = (const struct rtw89_c2h_mac_tx_rpt *)c2h->data;
> > +               sw_define = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_SW_DEFINE);
> > +               tx_status = le32_get_bits(rpt->w2, RTW89_C2H_MAC_TX_RPT_W2_TX_STATE);
> > +               if (rtwdev->chip->chip_id == RTL8852C)
> > +                       data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT_V1);
> > +               else
> > +                       data_txcnt = le32_get_bits(rpt->w5, RTW89_C2H_MAC_TX_RPT_W5_DATA_TX_CNT);
> > +       }
> > +
> > +       rtw89_debug(rtwdev, RTW89_DBG_TXRX,
> > +                   "C2H TX RPT: sn %d, tx_status %d, data_txcnt %d\n",
> > +                   sw_define, tx_status, data_txcnt);
> > +}
> > +
> >  static void
> >  rtw89_mac_c2h_mrc_tsf_rpt(struct rtw89_dev *rtwdev, struct sk_buff *c2h, u32 len)
> >  {
> 
> [...]
>