Niklas Cassel <niklas.cassel@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> [...] >> --- 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. >> +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. -- Kalle Valo
