thanks Pierre for taking time to review the patches.
On 03/09/2021 16:57, Pierre-Louis Bossart wrote:
+struct prm_cmd_request_rsc {
- struct apm_module_param_data param_data;
- uint32_t num_clk_id;
- struct audio_hw_clk_cfg clock_ids[1];
+} __packed;
+struct prm_cmd_release_rsc {
- struct apm_module_param_data param_data;
- uint32_t num_clk_id;
- struct audio_hw_clk_cfg clock_ids[1];
why do you need arrays of one element? I thought this was also frowned upon if not already deprecated?
These instances are fixed across the patches now.
+} __packed;
+static int q6prm_send_cmd_sync(struct q6prm *prm, struct gpr_pkt *pkt,
uint32_t rsp_opcode)+{
- gpr_device_t *gdev = prm->gdev;
- struct gpr_hdr *hdr = &pkt->hdr;
- int rc;
- mutex_lock(&prm->lock);
- prm->result.opcode = 0;
- prm->result.status = 0;
- rc = gpr_send_pkt(prm->gdev, pkt);
- if (rc < 0)
goto err;- if (rsp_opcode)
rc = wait_event_timeout(prm->wait,(prm->result.opcode == hdr->opcode) ||(prm->result.opcode == rsp_opcode),5 * HZ);- else
rc = wait_event_timeout(prm->wait,(prm->result.opcode == hdr->opcode),5 * HZ);- if (!rc) {
dev_err(&gdev->dev, "CMD timeout for [%x] opcode\n",hdr->opcode);rc = -ETIMEDOUT;- } else if (prm->result.status > 0) {
dev_err(&gdev->dev, "DSP returned error[%x] %x\n", hdr->opcode,prm->result.status);rc = -EINVAL;- } else {
dev_err(&gdev->dev, "DSP returned [%x]\n",prm->result.status);rc = 0;- }
+err:
- mutex_unlock(&prm->lock);
- return rc;
+}
this looks again like the same code we've seen twice already?
I remember your comments last time in the same area, Not sure why I missed this one. Its now fixed we have one library function for this.
+static int q6prm_set_hw_core_req(struct device *dev, uint32_t hw_block_id, bool enable) +{
- struct prm_cmd_request_hw_core *req;
- struct apm_module_param_data *param_data;
- struct gpr_pkt *pkt;
- struct q6prm *prm = dev_get_drvdata(dev->parent);
- gpr_device_t *gdev = prm->gdev;
- void *p;
- int rc = 0;
- uint32_t opcode, rsp_opcode;
- if (enable) {
opcode = PRM_CMD_REQUEST_HW_RSC;rsp_opcode = PRM_CMD_RSP_REQUEST_HW_RSC;- } else {
opcode = PRM_CMD_RELEASE_HW_RSC;rsp_opcode = PRM_CMD_RSP_RELEASE_HW_RSC;- }
- p = audioreach_alloc_cmd_pkt(sizeof(*req), opcode, 0, gdev->svc.id,
GPR_PRM_MODULE_IID);- if (IS_ERR(p))
return -ENOMEM;- pkt = p;
same comment as before for the rest of this file:
pkt = audioreach_alloc_cmd_pkt(sizeof(*req), opcode, 0, gdev->svc.id, GPR_PRM_MODULE_IID); kfree(pkt);
This is now fixed.
- req = p + GPR_HDR_SIZE + APM_CMD_HDR_SIZE;
- param_data = &req->param_data;
- param_data->module_instance_id = GPR_PRM_MODULE_IID;
- param_data->error_code = 0;
- param_data->param_id = PARAM_ID_RSC_HW_CORE;
- param_data->param_size = sizeof(*req) - APM_MODULE_PARAM_DATA_SIZE;
- req->hw_clk_id = hw_block_id;
- q6prm_send_cmd_sync(prm, pkt, rsp_opcode);
- kfree(pkt);
- return rc;
+}