Re: [PATCH 03/12] ath10k: Add ath10k QMI client driver

[Govind Singh]

Hi Bjorn,
Thanks for the review.

On 2018-05-11 22:55, Bjorn Andersson wrote:

On Sun 25 Mar 22:39 PDT 2018, Govind Singh wrote:


Add QMI client driver for Q6 integrated WLAN connectivity
subsystem. This module is responsible for communicating WLAN
control messages to FW over QUALCOMM MSM Interface (QMI).

Signed-off-by: Govind Singh 
---
 drivers/net/wireless/ath/ath10k/Kconfig  |   2 +-
 drivers/net/wireless/ath/ath10k/Makefile |   4 +
 drivers/net/wireless/ath/ath10k/qmi.c| 121 
+++

 drivers/net/wireless/ath/ath10k/qmi.h|  24 ++
 4 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
 create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h

diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
b/drivers/net/wireless/ath/ath10k/Kconfig

index 84f071a..9978ad5e 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -42,7 +42,7 @@ config ATH10K_USB

 config ATH10K_SNOC
 tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
-depends on ATH10K && ARCH_QCOM
+depends on ATH10K && ARCH_QCOM && QCOM_QMI_HELPERS


QCOM_QMI_HELPERS is expected to be selected by the clients, so this
would be:

select QCOM_QMI_HELPERS


Sure, will do in next version.


+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE 
LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY 
DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN 
AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING 
OUT OF

+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */


SPDX headers for new files please.



Sure, will do in next version.



+static struct ath10k_qmi *qmi;


Please design your code so that you don't depend on a global state
pointer.



Actually i thought about this, i can save this in platform drvdata and 
get this at

run time by saving the pdev in qmi_service->priv.
But qmi_service is available only in new_server/del_server, but not in 
the qmi indication callbacks.

Qmi handle also does not have the reference to the qmi_service.
Can you pls suggest something here.


+
+static int ath10k_qmi_new_server(struct qmi_handle *qmi_hdl,
+struct qmi_service *service)
+{
+   return 0;
+}
+
+static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
+ struct qmi_service *service)
+{
+}
+
+static struct qmi_ops ath10k_qmi_ops = {
+   .new_server = ath10k_qmi_new_server,
+   .del_server = ath10k_qmi_del_server,
+};
+
+static int ath10k_qmi_probe(struct platform_device *pdev)
+{
+   int ret;
+
+   qmi = devm_kzalloc(>dev, sizeof(*qmi),
+  GFP_KERNEL);


This doesn't need to be line wrapped.



Sure, will do in next version.


+   if (!qmi)
+   return -ENOMEM;
+
+   qmi->pdev = pdev;


The only place you use this is to access pdev->dev, so carry the struct
device * instead.



Sure, will do in next version.


+   platform_set_drvdata(pdev, qmi);
+   ret = qmi_handle_init(>qmi_hdl,
+ WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
+ _qmi_ops, NULL);
+   if (ret < 0)
+   goto err;
+
+   ret = qmi_add_lookup(>qmi_hdl, WLFW_SERVICE_ID_V01,
+WLFW_SERVICE_VERS_V01, 0);
+   if (ret < 0)
+   goto err;
+
+   pr_debug("qmi client driver probed successfully\n");


Rather than printing a line to indicate that your driver probed you can
check /sys/bus/platform/drivers/ath10k_QMI_client for for devices,
regardless of debug level.



Sure, will do in next version.


+
+   return 0;


qmi_add_lookup() returns 0 on success, so you can have a common return,
preferably after renaming "err" to "out" or something that indicates
that it covers both cases.


+
+err:
+   return ret;
+}
+
+static int ath10k_qmi_remove(struct platform_device *pdev)
+{
+   struct ath10k_qmi *qmi = platform_get_drvdata(pdev);
+
+   qmi_handle_release(>qmi_hdl);
+
+   return 0;
+}
+
+static const struct of_device_id ath10k_qmi_dt_match[] = {
+   {.compatible = "qcom,ath10k-qmi"},
+   {}
+};
+
+MODULE_DEVICE_TABLE(of, ath10k_qmi_dt_match);
+
+static struct platform_driver ath10k_qmi_clinet = {


Spelling of "client".



Sure, will do in next version.


+   .probe  = ath10k_qmi_probe,
+   .remove = ath10k_qmi_remove,
+   .driver = {
+   .name = "ath10k QMI client",
+   .owner = THIS_MODULE,


platform_driver_register() will fill out .owner for you, so skip this.


+   .of_match_table = ath10k_qmi_dt_match,
+   },
+};
+
+static int __init ath10k_qmi_init(void)
+{
+   return platform_driver_register(_qmi_clinet);
+}
+
+static void __exit ath10k_qmi_exit(void)
+{
+   

Re: [PATCH 03/12] ath10k: Add ath10k QMI client driver

[Bjorn Andersson]

On Sun 25 Mar 22:39 PDT 2018, Govind Singh wrote:

> Add QMI client driver for Q6 integrated WLAN connectivity
> subsystem. This module is responsible for communicating WLAN
> control messages to FW over QUALCOMM MSM Interface (QMI).
> 
> Signed-off-by: Govind Singh 
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig  |   2 +-
>  drivers/net/wireless/ath/ath10k/Makefile |   4 +
>  drivers/net/wireless/ath/ath10k/qmi.c| 121 
> +++
>  drivers/net/wireless/ath/ath10k/qmi.h|  24 ++
>  4 files changed, 150 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
>  create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h
> 
> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
> b/drivers/net/wireless/ath/ath10k/Kconfig
> index 84f071a..9978ad5e 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -42,7 +42,7 @@ config ATH10K_USB
>  
>  config ATH10K_SNOC
>  tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
> -depends on ATH10K && ARCH_QCOM
> +depends on ATH10K && ARCH_QCOM && QCOM_QMI_HELPERS

QCOM_QMI_HELPERS is expected to be selected by the clients, so this
would be:

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
> diff --git a/drivers/net/wireless/ath/ath10k/Makefile 
> b/drivers/net/wireless/ath/ath10k/Makefile
> index 44d60a6..1730d1d 100644
> --- a/drivers/net/wireless/ath/ath10k/Makefile
> +++ b/drivers/net/wireless/ath/ath10k/Makefile
> @@ -38,5 +38,9 @@ ath10k_usb-y += usb.o
>  obj-$(CONFIG_ATH10K_SNOC) += ath10k_snoc.o
>  ath10k_snoc-y += snoc.o
>  
> +obj-$(CONFIG_ATH10K_SNOC) += ath10k_qmi.o
> +ath10k_qmi-y += qmi.o \
> + qmi_svc_v01.o
> +
>  # for tracing framework to find trace.h
>  CFLAGS_trace.o := -I$(src)
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c 
> b/drivers/net/wireless/ath/ath10k/qmi.c
> new file mode 100644
> index 000..2235182
> --- /dev/null
> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
> @@ -0,0 +1,121 @@
> +/*
> + * Copyright (c) 2018 The Linux Foundation. All rights reserved.
> + *
> + * Permission to use, copy, modify, and/or distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */

SPDX headers for new files please.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "qmi.h"
> +#include "qmi_svc_v01.h"
> +
> +static struct ath10k_qmi *qmi;

Please design your code so that you don't depend on a global state
pointer.

> +
> +static int ath10k_qmi_new_server(struct qmi_handle *qmi_hdl,
> +  struct qmi_service *service)
> +{
> + return 0;
> +}
> +
> +static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
> +   struct qmi_service *service)
> +{
> +}
> +
> +static struct qmi_ops ath10k_qmi_ops = {
> + .new_server = ath10k_qmi_new_server,
> + .del_server = ath10k_qmi_del_server,
> +};
> +
> +static int ath10k_qmi_probe(struct platform_device *pdev)
> +{
> + int ret;
> +
> + qmi = devm_kzalloc(>dev, sizeof(*qmi),
> +GFP_KERNEL);

This doesn't need to be line wrapped.

> + if (!qmi)
> + return -ENOMEM;
> +
> + qmi->pdev = pdev;

The only place you use this is to access pdev->dev, so carry the struct
device * instead.

> + platform_set_drvdata(pdev, qmi);
> + ret = qmi_handle_init(>qmi_hdl,
> +   WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
> +   _qmi_ops, NULL);
> + if (ret < 0)
> + goto err;
> +
> + ret = qmi_add_lookup(>qmi_hdl, WLFW_SERVICE_ID_V01,
> +  WLFW_SERVICE_VERS_V01, 0);
> + if (ret < 0)
> + goto err;
> +
> + pr_debug("qmi client driver probed successfully\n");

Rather than printing a line to indicate that your driver probed you can
check /sys/bus/platform/drivers/ath10k_QMI_client for for devices,
regardless of debug level.

> +
> + return 0;

qmi_add_lookup() returns 0 on success, so you can have a 

[PATCH 03/12] ath10k: Add ath10k QMI client driver

[Govind Singh]

Add QMI client driver for Q6 integrated WLAN connectivity
subsystem. This module is responsible for communicating WLAN
control messages to FW over QUALCOMM MSM Interface (QMI).

Signed-off-by: Govind Singh 
---
 drivers/net/wireless/ath/ath10k/Kconfig  |   2 +-
 drivers/net/wireless/ath/ath10k/Makefile |   4 +
 drivers/net/wireless/ath/ath10k/qmi.c| 121 +++
 drivers/net/wireless/ath/ath10k/qmi.h|  24 ++
 4 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
 create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h

diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
b/drivers/net/wireless/ath/ath10k/Kconfig
index 84f071a..9978ad5e 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -42,7 +42,7 @@ config ATH10K_USB
 
 config ATH10K_SNOC
 tristate "Qualcomm ath10k SNOC support (EXPERIMENTAL)"
-depends on ATH10K && ARCH_QCOM
+depends on ATH10K && ARCH_QCOM && QCOM_QMI_HELPERS
 ---help---
   This module adds support for integrated WCN3990 chip connected
   to system NOC(SNOC). Currently work in progress and will not
diff --git a/drivers/net/wireless/ath/ath10k/Makefile 
b/drivers/net/wireless/ath/ath10k/Makefile
index 44d60a6..1730d1d 100644
--- a/drivers/net/wireless/ath/ath10k/Makefile
+++ b/drivers/net/wireless/ath/ath10k/Makefile
@@ -38,5 +38,9 @@ ath10k_usb-y += usb.o
 obj-$(CONFIG_ATH10K_SNOC) += ath10k_snoc.o
 ath10k_snoc-y += snoc.o
 
+obj-$(CONFIG_ATH10K_SNOC) += ath10k_qmi.o
+ath10k_qmi-y += qmi.o \
+   qmi_svc_v01.o
+
 # for tracing framework to find trace.h
 CFLAGS_trace.o := -I$(src)
diff --git a/drivers/net/wireless/ath/ath10k/qmi.c 
b/drivers/net/wireless/ath/ath10k/qmi.c
new file mode 100644
index 000..2235182
--- /dev/null
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -0,0 +1,121 @@
+/*
+ * Copyright (c) 2018 The Linux Foundation. All rights reserved.
+ *
+ * Permission to use, copy, modify, and/or distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include "qmi.h"
+#include "qmi_svc_v01.h"
+
+static struct ath10k_qmi *qmi;
+
+static int ath10k_qmi_new_server(struct qmi_handle *qmi_hdl,
+struct qmi_service *service)
+{
+   return 0;
+}
+
+static void ath10k_qmi_del_server(struct qmi_handle *qmi_hdl,
+ struct qmi_service *service)
+{
+}
+
+static struct qmi_ops ath10k_qmi_ops = {
+   .new_server = ath10k_qmi_new_server,
+   .del_server = ath10k_qmi_del_server,
+};
+
+static int ath10k_qmi_probe(struct platform_device *pdev)
+{
+   int ret;
+
+   qmi = devm_kzalloc(>dev, sizeof(*qmi),
+  GFP_KERNEL);
+   if (!qmi)
+   return -ENOMEM;
+
+   qmi->pdev = pdev;
+   platform_set_drvdata(pdev, qmi);
+   ret = qmi_handle_init(>qmi_hdl,
+ WLFW_BDF_DOWNLOAD_REQ_MSG_V01_MAX_MSG_LEN,
+ _qmi_ops, NULL);
+   if (ret < 0)
+   goto err;
+
+   ret = qmi_add_lookup(>qmi_hdl, WLFW_SERVICE_ID_V01,
+WLFW_SERVICE_VERS_V01, 0);
+   if (ret < 0)
+   goto err;
+
+   pr_debug("qmi client driver probed successfully\n");
+
+   return 0;
+
+err:
+   return ret;
+}
+
+static int ath10k_qmi_remove(struct platform_device *pdev)
+{
+   struct ath10k_qmi *qmi = platform_get_drvdata(pdev);
+
+   qmi_handle_release(>qmi_hdl);
+
+   return 0;
+}
+
+static const struct of_device_id ath10k_qmi_dt_match[] = {
+   {.compatible = "qcom,ath10k-qmi"},
+   {}
+};
+
+MODULE_DEVICE_TABLE(of, ath10k_qmi_dt_match);
+
+static struct platform_driver ath10k_qmi_clinet = {
+   .probe  = ath10k_qmi_probe,
+   .remove = ath10k_qmi_remove,
+   .driver = {
+   .name = "ath10k QMI client",
+   .owner = THIS_MODULE,
+   .of_match_table = ath10k_qmi_dt_match,
+   },
+};
+
+static int __init ath10k_qmi_init(void)
+{
+   return platform_driver_register(_qmi_clinet);
+}
+
+static void __exit ath10k_qmi_exit(void)
+{
+