Received-SPF: pass (google.com: domain of linux-wireless+bounces-6184-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <823fe8a6-1e6c-49e9-a225-2012e77093ac@quicinc.com>
Date: Thu, 11 Apr 2024 08:20:38 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 4/4] wifi: ath12k: Refactor data path cmem init
Content-Language: en-US
To: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>,
        Kalle Valo
	<kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240409151415.1226109-1-quic_periyasa@quicinc.com>
 <20240409151415.1226109-5-quic_periyasa@quicinc.com>
 <877ch4z0z1.fsf@kernel.org>
 <6ac588c2-6206-c12e-e741-cbc507365bc3@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <6ac588c2-6206-c12e-e741-cbc507365bc3@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 4/11/2024 3:07 AM, Karthikeyan Periyasamy wrote:
> 
> 
> On 4/11/2024 3:15 PM, Kalle Valo wrote:
>> Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes:
>>
>>> Move the data path Tx and Rx descriptor primary page table CMEM
>>> configuration into a helper function. This will make the code more
>>> scalable for configuring partner device in support of multi-device MLO.
>>>
>>> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.1.1-00188-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: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
>>> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
>>
>> [...]
>>
>>> +static void ath12k_dp_cmem_init(struct ath12k_base *ab,
>>> +				struct ath12k_dp *dp,
>>> +				enum ath12k_dp_desc_type type)
>>> +{
>>> +	u32 cmem_base;
>>> +	int i, start, end;
>>> +
>>> +	cmem_base = ab->qmi.dev_mem[ATH12K_QMI_DEVMEM_CMEM_INDEX].start;
>>> +
>>> +	switch (type) {
>>> +	case ATH12K_DP_TX_DESC:
>>> +		start = ATH12K_TX_SPT_PAGE_OFFSET;
>>> +		end = start + ATH12K_NUM_TX_SPT_PAGES;
>>> +		break;
>>> +	case ATH12K_DP_RX_DESC:
>>> +		start = ATH12K_RX_SPT_PAGE_OFFSET;
>>> +		end = start + ATH12K_NUM_RX_SPT_PAGES;
>>> +		break;
>>> +	default:
>>> +		ath12k_err(ab, "invalid descriptor type %d in cmem init\n", type);
>>> +		return;
>>> +	}
>>> +
>>> +	/* Write to PPT in CMEM */
>>> +	for (i = start; i < end; i++)
>>> +		ath12k_hif_write32(ab, cmem_base + ATH12K_PPT_ADDR_OFFSET(i),
>>> +				   dp->spt_info[i].paddr >> ATH12K_SPT_4K_ALIGN_OFFSET);
>>> +}
>>
>> Here's a good example why I don't like functions returning void. How do
>> we handle the errors in this case?
>>
> 
> sure, will handle the error case in the caller.
> 

this is a static function with one caller. the only error is the default case
which will never be hit. adding logic to return an error and then check it in
the caller seems like overkill. why not just WARN() in the default case since
this would be a logic error with newly added code?