On 2018-07-03 20:45, Kalle Valo wrote:
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.
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
