Received-SPF: pass (google.com: domain of linux-wireless+bounces-3686-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: Kalle Valo <kvalo@kernel.org>
To: Rameshkumar Sundaram <quic_ramess@quicinc.com>
Cc: <ath12k@lists.infradead.org>,  <linux-wireless@vger.kernel.org>,  Sriram
 R <quic_srirrama@quicinc.com>
Subject: Re: [PATCH v2 03/12] wifi: ath12k: modify ath12k mac start/stop ops
 for single wiphy
References: <20240206091954.4144454-1-quic_ramess@quicinc.com>
	<20240206091954.4144454-4-quic_ramess@quicinc.com>
Date: Fri, 16 Feb 2024 18:35:41 +0200
In-Reply-To: <20240206091954.4144454-4-quic_ramess@quicinc.com> (Rameshkumar
	Sundaram's message of "Tue, 6 Feb 2024 14:49:45 +0530")
Message-ID: <8734tsnz8i.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Rameshkumar Sundaram <quic_ramess@quicinc.com> writes:

> From: Sriram R <quic_srirrama@quicinc.com>
>
> 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@quicinc.com>
> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>

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.

> +	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?

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches