> -----Original Message-----
> From: Kalle Valo <kvalo@xxxxxxxxxx>
> Sent: Friday, February 16, 2024 10:06 PM
> To: Rameshkumar Sundaram (QUIC) <quic_ramess@xxxxxxxxxxx>
> Cc: ath12k@xxxxxxxxxxxxxxxxxxx; linux-wireless@xxxxxxxxxxxxxxx; Sriram
> R <quic_srirrama@xxxxxxxxxxx>
> Subject: Re: [PATCH v2 03/12] wifi: ath12k: modify ath12k mac
> start/stop ops for single wiphy
>
> Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx> writes:
>
> > From: Sriram R <quic_srirrama@xxxxxxxxxxx>
> >
> > When mac80211 does drv start/stop, apply the state change for all
> the
> > radios within the wiphy in ath12k.
> >
> > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-
> QCAHKSWPL_SILICONZ-1
> > Tested-on: WCN7850 hw2.0 PCI
> > WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> >
> > Signed-off-by: Sriram R <quic_srirrama@xxxxxxxxxxx>
> > Signed-off-by: Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx>
>
> I haven't reviewed the patchset fully yet, first quick impressions:
>
> > +static void ath12k_drain_tx(struct ath12k_hw *ah) {
> > + int i;
> > + struct ath12k *ar;
>
> We usually try to have reverse xmas style, of course not always possible
> but here it is.
>
Sure, Will fix it v3.
> > + ar = ah->radio;
> > +
> > + for (i = 0; i < ah->num_radio; i++) {
> > + ath12k_mac_drain_tx(ar);
> > + ar++;
> > + }
> > +}
>
> Please avoid pointer arithmetic (in this case 'ar++') as much as possible,
> it's just so easy to shoot yourself in the foot. In patch 1 you add
> ath12k_ah_to_ar(), why not use it?
Sure, will avoid this and take reference from ah->radio[]
>
> And I see this pattern quite a lot, should we have
> ath12k_for_each_radio() or something like that? Or did I see that macro
> in some patch already?
Sure, makes sense, guess it was in v1 of patch [01/12] in this series, but was removed and used inline in subsequent patch sets.
will introduced it back.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submitti
> ngpatches
