Re: [PATCH v2 6/6] ath10k: Add QMI message handshake for wcn3990 client

[Govind Singh <govinds@codeaurora.org>]

On 2018-07-03 20:45, Kalle Valo wrote:
> Niklas Cassel <niklas.cassel@linaro.org> writes:
> 
>> On Tue, Jun 05, 2018 at 06:07:32PM +0530, Govind Singh wrote:
>>> Add WCN3990 QMI client handshakes for Q6 integrated WLAN connectivity
>>> subsystem. This layer is responsible for communicating qmi control
>>> messages to wifi fw QMI service using QMI messaging protocol.
>>> 
>>> Qualcomm MSM Interface(QMI) is a messaging format used to communicate
>>> between components running between application processor and remote
>>> processors with underlying transport layer based on integrated
>>> chipset(shared memory) or discrete chipset(PCI/USB/SDIO/UART).
>>> 
>>> With this patch-set basic functionality(STA/SAP) can be tested
>>> with WCN3990 chipset. The changes are verified with the firmware
>>> WLAN.HL.2.0-01192-QCAHLSWMTPLZ-1 and SDM845 MTP device.
>>> 
>>> Signed-off-by: Govind Singh <govinds@codeaurora.org>
> 
> [...]
> 
>>> --- a/drivers/net/wireless/ath/ath10k/Kconfig
>>> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
>>> @@ -41,12 +41,13 @@ config ATH10K_USB
>>>  	  work in progress and will not fully work.
>>> 
>>>  config ATH10K_SNOC
>>> -        tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
>>> -        depends on ATH10K && ARCH_QCOM
>>> -        ---help---
>>> -          This module adds support for integrated WCN3990 chip 
>>> connected
>>> -          to system NOC(SNOC). Currently work in progress and will 
>>> not
>>> -          fully work.
>>> +	tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
>>> +	depends on ATH10K && ARCH_QCOM
>>> +	select QCOM_QMI_HELPERS
>>> +	---help---
>>> +	This module adds support for integrated WCN3990 chip connected
>>> +	to system NOC(SNOC). Currently work in progress and will not
>>> +	fully work.
>> 
>> Hello Govind,
>> 
>> Please do clean ups in separate commits.
>> That way is would be easier to see that the only
>> functional change here is that you added
>> select QCOM_QMI_HELPERS.
>> 
>> (Also help text should normally be indented by two extra spaces.)
>> 
>> I've sent a fix for the mixed tabs/spaces when I tried to
>> add COMPILE_TEST for this, and Kalle has already picked it up
>> in his master branch:
>> https://marc.info/?l=linux-wireless&m=152880359200364
>> 
>> So in your next version of this series, this can be reduced to simply
>> select QCOM_QMI_HELPERS.
> 
> BTW, I actually already did this on the pending branch:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=0caef5beefb85e6a5aa2a4b60d78253bbe453d7c
> 
>> There are some checkpatch warnings on this patch:
>> 
>> drivers/net/wireless/ath/ath10k/qmi.c and
>> drivers/net/wireless/ath/ath10k/qmi.h
>> is missing SPDX-License-Identifier tag.
>> 
>> Several lines in drivers/net/wireless/ath/ath10k/qmi.c
>> and one line in drivers/net/wireless/ath/ath10k/snoc.c
>> is over 80 characters.
> 
> Yeah, but these can be ignored.
> 
>> This patch is quite big, I think that it makes sense to split your 
>> patch in two.
>> One that adds the ath10k_qmi_* functions, and a follow up patch
>> that actually adds the function calls to snoc.c
> 
> Yeah, it's big but IMHO not too big. And splitting it up makes
> functinonal review harder, that's why I prefer it like this.
> 
>>> +int ath10k_qmi_wlan_enable(struct ath10k *ar,
>>> +			   struct ath10k_qmi_wlan_enable_cfg *config,
>>> +			   enum ath10k_qmi_driver_mode mode,
>>> +			   const char *version)
>>> +{
>>> +	int ret;
>>> +
>>> +	ath10k_dbg(ar, ATH10K_DBG_QMI, "mode: %d, config: %p:\n",
>>> +		   mode, config);
>>> +
>>> +	ret = ath10k_qmi_cfg_send_sync_msg(ar, config, version);
>>> +	if (ret) {
>>> +		ath10k_err(ar, "wlan qmi config send failed\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = ath10k_qmi_mode_send_sync_msg(ar, mode);
>> 
>> Sparse tells me that you are mixing enum types.
>> If this is really what you want, do an explicit cast.
>> 
>> drivers/net/wireless/ath/ath10k//qmi.c:504:49: warning: mixing 
>> different enum types
>> drivers/net/wireless/ath/ath10k//qmi.c:504:49:     int enum 
>> ath10k_qmi_driver_mode  versus
>> drivers/net/wireless/ath/ath10k//qmi.c:504:49:     int enum 
>> wlfw_driver_mode_enum_v01
> 
> Good catch, that can cause subtle bugs. If you really want this, I
> prefer having a separate helper function with a switch statement making
> the conversion. This way it's super clear what's happening.
> 

Addressed in v3 version.

>>> +int ath10k_snoc_fw_indication(struct ath10k *ar, u64 type)
>>> +{
>>> +	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
>>> +	int ret;
>>> +
>>> +	switch (type) {
>>> +	case ATH10K_QMI_EVENT_FW_READY_IND:
>>> +		ret = ath10k_core_register(ar,
>>> +					   ar_snoc->target_info.soc_version);
>>> +		if (ret) {
>>> +			ath10k_err(ar, "Failed to register driver core: %d\n",
>>> +				   ret);
>>> +		}
>>> +		break;
>>> +	case ATH10K_QMI_EVENT_FW_DOWN_IND:
>>> +		break;
>> 
>> Perhaps this switch statement should have a default label?
> 
> Good idea. And add a warning so that we know there was an unknown event
> which should be added to ath10k.

Addressed in v3 version.

Thanks,
Govind

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k