On Sun 25 Mar 22:40 PDT 2018, Govind Singh wrote:
> Add support for target capablity request
> qmi message for wcn3990 target.
>
> Signed-off-by: Govind Singh <govinds@xxxxxxxxxxxxxx>
> ---
> drivers/net/wireless/ath/ath10k/qmi.c | 91 +++++++++++++++++++++++++++++++++++
> drivers/net/wireless/ath/ath10k/qmi.h | 25 ++++++++++
> 2 files changed, 116 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
> index 763b812..65a43af 100644
> --- a/drivers/net/wireless/ath/ath10k/qmi.c
> +++ b/drivers/net/wireless/ath/ath10k/qmi.c
> @@ -270,6 +270,93 @@ static int ath10k_qmi_msa_ready_send_sync_msg(struct ath10k_qmi *qmi)
> return ret;
> }
>
> +static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi)
> +{
> + struct wlfw_cap_resp_msg_v01 *resp;
I think this is 207 bytes, so it makes sense to allocate this on the
heap.
> + struct wlfw_cap_req_msg_v01 *req;
But this is 1 bytes, so keep it on the stack.
> + struct qmi_txn txn;
> + int ret;
> +
> + req = kzalloc(sizeof(*req), GFP_KERNEL);
> + if (!req)
> + return -ENOMEM;
> +
> + resp = kzalloc(sizeof(*resp), GFP_KERNEL);
> + if (!resp) {
> + kfree(req);
> + return -ENOMEM;
> + }
> +
> + ret = qmi_txn_init(&qmi->qmi_hdl, &txn, wlfw_cap_resp_msg_v01_ei, resp);
> + if (ret < 0) {
> + pr_err("fail to init txn for capability resp %d\n", ret);
Same comments as in previous patches.
> + goto out;
> + }
> +
> + ret = qmi_send_request(&qmi->qmi_hdl, NULL, &txn,
> + QMI_WLFW_CAP_REQ_V01,
> + WLFW_CAP_REQ_MSG_V01_MAX_MSG_LEN,
> + wlfw_cap_req_msg_v01_ei, req);
> + if (ret < 0) {
> + qmi_txn_cancel(&txn);
> + pr_err("fail to send capability req %d\n", ret);
> + goto out;
> + }
> +
> + ret = qmi_txn_wait(&txn, WLFW_TIMEOUT * HZ);
> + if (ret < 0)
> + goto out;
> +
> + if (resp->resp.result != QMI_RESULT_SUCCESS_V01) {
> + pr_err("qmi capability request rejected, result:%d error:%d\n",
> + resp->resp.result, resp->resp.error);
> + ret = -resp->resp.result;
> + goto out;
> + }
> +
> + if (resp->chip_info_valid) {
> + qmi->chip_info.chip_id = resp->chip_info.chip_id;
> + qmi->chip_info.chip_family = resp->chip_info.chip_family;
> + }
> +
> + if (resp->board_info_valid)
> + qmi->board_info.board_id = resp->board_info.board_id;
> + else
> + qmi->board_info.board_id = 0xFF;
> +
> + if (resp->soc_info_valid)
> + qmi->soc_info.soc_id = resp->soc_info.soc_id;
> +
> + if (resp->fw_version_info_valid) {
> + qmi->fw_version_info.fw_version =
> + resp->fw_version_info.fw_version;
> + strlcpy(qmi->fw_version_info.fw_build_timestamp,
> + resp->fw_version_info.fw_build_timestamp,
> + MAX_TIMESTAMP_LEN + 1);
Use sizeof(qmi->fw_version_info.fw_build_timestamp) to remove the risk
of them getting out of sync.
> + }
> +
> + if (resp->fw_build_id_valid)
> + strlcpy(qmi->fw_build_id, resp->fw_build_id,
> + MAX_BUILD_ID_LEN + 1);
> +
> + pr_debug("chip_id: 0x%x, chip_family: 0x%x, board_id: 0x%x, soc_id: 0x%x, fw_version: 0x%x, fw_build_timestamp: %s, fw_build_id: %s",
> + qmi->chip_info.chip_id, qmi->chip_info.chip_family,
> + qmi->board_info.board_id, qmi->soc_info.soc_id,
> + qmi->fw_version_info.fw_version,
> + qmi->fw_version_info.fw_build_timestamp,
> + qmi->fw_build_id);
Use dev_dbg(), but perhaps this could even be a dev_info()?
> +
> + pr_debug("target cap request completed\n");
> + kfree(resp);
> + kfree(req);
> +
> + return 0;
> +out:
> + kfree(resp);
> + kfree(req);
> + return ret;
> +}
> +
> static int
> ath10k_qmi_ind_register_send_sync_msg(struct ath10k_qmi *qmi)
> {
> @@ -425,6 +512,10 @@ static void ath10k_qmi_event_server_arrive(struct work_struct *work)
> if (ret)
> goto err_setup_msa;
>
> + ret = ath10k_qmi_cap_send_sync_msg(qmi);
> + if (ret)
> + goto err_setup_msa;
> +
> return;
>
> err_setup_msa:
> diff --git a/drivers/net/wireless/ath/ath10k/qmi.h b/drivers/net/wireless/ath/ath10k/qmi.h
> index 47af020..09d20a0 100644
> --- a/drivers/net/wireless/ath/ath10k/qmi.h
> +++ b/drivers/net/wireless/ath/ath10k/qmi.h
> @@ -17,6 +17,8 @@
> #define _QMI_H_
>
> #define MAX_NUM_MEMORY_REGIONS 2
> +#define MAX_TIMESTAMP_LEN 32
> +#define MAX_BUILD_ID_LEN 128
>
> enum ath10k_qmi_driver_event_type {
> ATH10K_QMI_EVENT_SERVER_ARRIVE,
> @@ -31,6 +33,24 @@ struct ath10k_msa_mem_region_info {
> u8 secure_flag;
> };
>
> +struct ath10k_qmi_chip_info {
> + u32 chip_id;
> + u32 chip_family;
> +};
> +
> +struct ath10k_qmi_board_info {
> + u32 board_id;
> +};
> +
> +struct ath10k_qmi_soc_info {
> + u32 soc_id;
> +};
> +
> +struct ath10k_qmi_fw_version_info {
> + u32 fw_version;
> + char fw_build_timestamp[MAX_TIMESTAMP_LEN + 1];
> +};
I don't see the benefit off lugging these around as separate structs,
why not just put them in the ath10k_qmi struct directly.
But in particular, I don't see a reason for stashing the data that will
never be used, store the parts that you're actually using.
> +
> struct ath10k_qmi {
> struct platform_device *pdev;
> struct qmi_handle qmi_hdl;
> @@ -47,5 +67,10 @@ struct ath10k_qmi {
> phys_addr_t msa_pa;
> u32 msa_mem_size;
> void *msa_va;
> + struct ath10k_qmi_chip_info chip_info;
> + struct ath10k_qmi_board_info board_info;
> + struct ath10k_qmi_soc_info soc_info;
> + struct ath10k_qmi_fw_version_info fw_version_info;
> + char fw_build_id[MAX_BUILD_ID_LEN + 1];
> };
> #endif /* _QMI_H_ */
Regards,
Bjorn
