From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> To: Vinod <vkoul@kernel.org> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, alsa-devel@alsa-project.org, girishm@quicinc.com, bgoswami@codeaurora.org, gregkh@linuxfoundation.org, broonie@kernel.org, linux-kernel@vger.kernel.org, robh+dt@kernel.org, kramasub@codeaurora.org, linux-arm-msm@vger.kernel.org, sdharia@quicinc.com Subject: Re: [PATCH 2/2] slimbus: ngd: Add qcom SLIMBus NGD driver Date: Mon, 21 May 2018 12:55:51 +0100 [thread overview] Message-ID: <b29bf75f-4846-ef73-b1da-509b62488944@linaro.org> (raw) In-Reply-To: <20180521113314.GC14654@vkoul-mobl> Thanks Vinod for the review! On 21/05/18 12:33, Vinod wrote: > On 16-05-18, 17:51, Srinivas Kandagatla wrote: >> This patch adds suppor to Qualcomm SLIMBus Non-Generic Device (NGD) > > /s/suppor/support > Yep, Will fix this in next version. >> +/* NGD (Non-ported Generic Device) registers */ >> +#define NGD_CFG 0x0 >> +#define NGD_CFG_ENABLE BIT(0) >> +#define NGD_CFG_RX_MSGQ_EN BIT(1) >> +#define NGD_CFG_TX_MSGQ_EN BIT(2) >> +#define NGD_STATUS 0x4 >> +#define NGD_LADDR BIT(1) >> +#define NGD_RX_MSGQ_CFG 0x8 >> +#define NGD_INT_EN 0x10 >> +#define SLIMBUS_QMI_POWER_RESP_MAX_MSG_LEN 7 >> +#define SLIMBUS_QMI_SELECT_INSTANCE_REQ_MAX_MSG_LEN 14 >> +#define SLIMBUS_QMI_SELECT_INSTANCE_RESP_MAX_MSG_LEN 7 >> +#define SLIMBUS_QMI_CHECK_FRAMER_STAT_RESP_MAX_MSG_LEN 7 >> +/* QMI response timeout of 500ms */ >> +#define SLIMBUS_QMI_RESP_TOUT 1000 > > Tabs or spaces, take your pick and use one, not both... I will check for such instances once again before sending v2. > >> +static void qcom_slim_qmi_power_resp_cb(struct qmi_handle *handle, >> + struct sockaddr_qrtr *sq, >> + struct qmi_txn *txn, const void *data) >> +{ >> + struct slimbus_power_resp_msg_v01 *resp; >> + >> + resp = (struct slimbus_power_resp_msg_v01 *)data; > > you dont need cast away from void It's a const void * so the compiler keeps complaining about this without cast. > >> + if (resp->resp.result != QMI_RESULT_SUCCESS_V01) >> + pr_err("%s: QMI power request failed 0x%x\n", __func__, >> + resp->resp.result); > > cant we use dev_err? I will have a look! > >> +static int qcom_slim_qmi_send_power_request(struct qcom_slim_ngd_ctrl *ctrl, >> + struct slimbus_power_req_msg_v01 *req) >> +{ >> + struct slimbus_power_resp_msg_v01 resp = { { 0, 0 } }; >> + struct qmi_txn txn; >> + int rc; >> + >> + rc = qmi_txn_init(ctrl->qmi.handle, &txn, >> + slimbus_power_resp_msg_v01_ei, &resp); >> + >> + rc = qmi_send_request(ctrl->qmi.handle, NULL, &txn, >> + SLIMBUS_QMI_POWER_REQ_V01, >> + SLIMBUS_QMI_POWER_REQ_MAX_MSG_LEN, >> + slimbus_power_req_msg_v01_ei, req); >> + if (rc < 0) { >> + dev_err(ctrl->dev, "%s: QMI send req fail %d\n", __func__, rc); >> + qmi_txn_cancel(&txn); >> + } >> + >> + if (rc < 0) >> + return rc; > > why not add this is prev error check? Yes, we should move this to previous check, Will fix this in v2. > >> +static int qcom_slim_qmi_init(struct qcom_slim_ngd_ctrl *ctrl, >> + bool apps_is_master) >> +{ >> + int rc = 0; > > superfluous init > >> +static u32 *qcom_slim_ngd_tx_msg_get(struct qcom_slim_ngd_ctrl *ctrl, int len, >> + struct completion *comp) >> +{ >> + struct qcom_slim_ngd_dma_desc *desc; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&ctrl->tx_buf_lock, flags); >> + >> + if ((ctrl->tx_tail + 1) % QCOM_SLIM_NGD_DESC_NUM == ctrl->tx_head) { >> + spin_unlock_irqrestore(&ctrl->tx_buf_lock, flags); >> + return NULL; >> + } >> + desc = &ctrl->txdesc[ctrl->tx_tail]; >> + desc->base = (u32 *)((u8 *)ctrl->tx_base + >> + (ctrl->tx_tail * SLIM_MSGQ_BUF_LEN)); > > too many casts > >> +static int qcom_slim_ngd_post_rx_msgq(struct qcom_slim_ngd_ctrl *ctrl) >> +{ >> + struct qcom_slim_ngd_dma_desc *desc; >> + struct dma_slave_config conf = { >> + .direction = DMA_DEV_TO_MEM, >> + }; >> + int ret, i; >> + >> + ret = dmaengine_slave_config(ctrl->dma_rx_channel, &conf); > > so you are only setting direction for conf, not any other parameters? If not why > bother setting direction Nice hint, I will remove this! > >> + if (ret) >> + dev_err(ctrl->dev, "Error Configuring rx dma\n"); >> + >> + for (i = 0; i < QCOM_SLIM_NGD_DESC_NUM; i++) { >> + desc = &ctrl->rx_desc[i]; >> + desc->phys = ctrl->rx_phys_base + i * SLIM_MSGQ_BUF_LEN; >> + desc->ctrl = ctrl; >> + desc->base = ctrl->rx_base + i * SLIM_MSGQ_BUF_LEN; >> + desc->desc = dmaengine_prep_slave_single(ctrl->dma_rx_channel, >> + desc->phys, SLIM_MSGQ_BUF_LEN, >> + DMA_DEV_TO_MEM, >> + DMA_PREP_INTERRUPT); > > why issue multiple slave_single to dmaengine, you can bundle them and issue > dmaengine_prep_slave_sg().. Am reusing the descriptors here, My plan is to issue multiple descriptors with callback for each, and in each callback queue it back. > >> +static int qcom_slim_ngd_qmi_svc_event_init(struct qcom_slim_ngd_qmi *qmi) >> +{ >> + int ret = 0; > > superfluous init here too > Yep. >> +static int qcom_slim_ngd_runtime_idle(struct device *dev) >> +{ >> + struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(dev); >> + >> + if (ctrl->state == QCOM_SLIM_NGD_CTRL_AWAKE) >> + ctrl->state = QCOM_SLIM_NGD_CTRL_IDLE; >> + pm_request_autosuspend(dev); >> + return -EAGAIN; >> +} >> + >> + > > double empty lines, here Sure, will fix this in v2. > thanks, srini
WARNING: multiple messages have this Message-ID (diff)
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> To: Vinod <vkoul@kernel.org> Cc: gregkh@linuxfoundation.org, robh+dt@kernel.org, kramasub@codeaurora.org, sdharia@quicinc.com, girishm@quicinc.com, linux-kernel@vger.kernel.org, mark.rutland@arm.com, bgoswami@codeaurora.org, devicetree@vger.kernel.org, broonie@kernel.org, linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org Subject: Re: [PATCH 2/2] slimbus: ngd: Add qcom SLIMBus NGD driver Date: Mon, 21 May 2018 12:55:51 +0100 [thread overview] Message-ID: <b29bf75f-4846-ef73-b1da-509b62488944@linaro.org> (raw) In-Reply-To: <20180521113314.GC14654@vkoul-mobl> Thanks Vinod for the review! On 21/05/18 12:33, Vinod wrote: > On 16-05-18, 17:51, Srinivas Kandagatla wrote: >> This patch adds suppor to Qualcomm SLIMBus Non-Generic Device (NGD) > > /s/suppor/support > Yep, Will fix this in next version. >> +/* NGD (Non-ported Generic Device) registers */ >> +#define NGD_CFG 0x0 >> +#define NGD_CFG_ENABLE BIT(0) >> +#define NGD_CFG_RX_MSGQ_EN BIT(1) >> +#define NGD_CFG_TX_MSGQ_EN BIT(2) >> +#define NGD_STATUS 0x4 >> +#define NGD_LADDR BIT(1) >> +#define NGD_RX_MSGQ_CFG 0x8 >> +#define NGD_INT_EN 0x10 >> +#define SLIMBUS_QMI_POWER_RESP_MAX_MSG_LEN 7 >> +#define SLIMBUS_QMI_SELECT_INSTANCE_REQ_MAX_MSG_LEN 14 >> +#define SLIMBUS_QMI_SELECT_INSTANCE_RESP_MAX_MSG_LEN 7 >> +#define SLIMBUS_QMI_CHECK_FRAMER_STAT_RESP_MAX_MSG_LEN 7 >> +/* QMI response timeout of 500ms */ >> +#define SLIMBUS_QMI_RESP_TOUT 1000 > > Tabs or spaces, take your pick and use one, not both... I will check for such instances once again before sending v2. > >> +static void qcom_slim_qmi_power_resp_cb(struct qmi_handle *handle, >> + struct sockaddr_qrtr *sq, >> + struct qmi_txn *txn, const void *data) >> +{ >> + struct slimbus_power_resp_msg_v01 *resp; >> + >> + resp = (struct slimbus_power_resp_msg_v01 *)data; > > you dont need cast away from void It's a const void * so the compiler keeps complaining about this without cast. > >> + if (resp->resp.result != QMI_RESULT_SUCCESS_V01) >> + pr_err("%s: QMI power request failed 0x%x\n", __func__, >> + resp->resp.result); > > cant we use dev_err? I will have a look! > >> +static int qcom_slim_qmi_send_power_request(struct qcom_slim_ngd_ctrl *ctrl, >> + struct slimbus_power_req_msg_v01 *req) >> +{ >> + struct slimbus_power_resp_msg_v01 resp = { { 0, 0 } }; >> + struct qmi_txn txn; >> + int rc; >> + >> + rc = qmi_txn_init(ctrl->qmi.handle, &txn, >> + slimbus_power_resp_msg_v01_ei, &resp); >> + >> + rc = qmi_send_request(ctrl->qmi.handle, NULL, &txn, >> + SLIMBUS_QMI_POWER_REQ_V01, >> + SLIMBUS_QMI_POWER_REQ_MAX_MSG_LEN, >> + slimbus_power_req_msg_v01_ei, req); >> + if (rc < 0) { >> + dev_err(ctrl->dev, "%s: QMI send req fail %d\n", __func__, rc); >> + qmi_txn_cancel(&txn); >> + } >> + >> + if (rc < 0) >> + return rc; > > why not add this is prev error check? Yes, we should move this to previous check, Will fix this in v2. > >> +static int qcom_slim_qmi_init(struct qcom_slim_ngd_ctrl *ctrl, >> + bool apps_is_master) >> +{ >> + int rc = 0; > > superfluous init > >> +static u32 *qcom_slim_ngd_tx_msg_get(struct qcom_slim_ngd_ctrl *ctrl, int len, >> + struct completion *comp) >> +{ >> + struct qcom_slim_ngd_dma_desc *desc; >> + unsigned long flags; >> + >> + spin_lock_irqsave(&ctrl->tx_buf_lock, flags); >> + >> + if ((ctrl->tx_tail + 1) % QCOM_SLIM_NGD_DESC_NUM == ctrl->tx_head) { >> + spin_unlock_irqrestore(&ctrl->tx_buf_lock, flags); >> + return NULL; >> + } >> + desc = &ctrl->txdesc[ctrl->tx_tail]; >> + desc->base = (u32 *)((u8 *)ctrl->tx_base + >> + (ctrl->tx_tail * SLIM_MSGQ_BUF_LEN)); > > too many casts > >> +static int qcom_slim_ngd_post_rx_msgq(struct qcom_slim_ngd_ctrl *ctrl) >> +{ >> + struct qcom_slim_ngd_dma_desc *desc; >> + struct dma_slave_config conf = { >> + .direction = DMA_DEV_TO_MEM, >> + }; >> + int ret, i; >> + >> + ret = dmaengine_slave_config(ctrl->dma_rx_channel, &conf); > > so you are only setting direction for conf, not any other parameters? If not why > bother setting direction Nice hint, I will remove this! > >> + if (ret) >> + dev_err(ctrl->dev, "Error Configuring rx dma\n"); >> + >> + for (i = 0; i < QCOM_SLIM_NGD_DESC_NUM; i++) { >> + desc = &ctrl->rx_desc[i]; >> + desc->phys = ctrl->rx_phys_base + i * SLIM_MSGQ_BUF_LEN; >> + desc->ctrl = ctrl; >> + desc->base = ctrl->rx_base + i * SLIM_MSGQ_BUF_LEN; >> + desc->desc = dmaengine_prep_slave_single(ctrl->dma_rx_channel, >> + desc->phys, SLIM_MSGQ_BUF_LEN, >> + DMA_DEV_TO_MEM, >> + DMA_PREP_INTERRUPT); > > why issue multiple slave_single to dmaengine, you can bundle them and issue > dmaengine_prep_slave_sg().. Am reusing the descriptors here, My plan is to issue multiple descriptors with callback for each, and in each callback queue it back. > >> +static int qcom_slim_ngd_qmi_svc_event_init(struct qcom_slim_ngd_qmi *qmi) >> +{ >> + int ret = 0; > > superfluous init here too > Yep. >> +static int qcom_slim_ngd_runtime_idle(struct device *dev) >> +{ >> + struct qcom_slim_ngd_ctrl *ctrl = dev_get_drvdata(dev); >> + >> + if (ctrl->state == QCOM_SLIM_NGD_CTRL_AWAKE) >> + ctrl->state = QCOM_SLIM_NGD_CTRL_IDLE; >> + pm_request_autosuspend(dev); >> + return -EAGAIN; >> +} >> + >> + > > double empty lines, here Sure, will fix this in v2. > thanks, srini
next prev parent reply other threads:[~2018-05-21 11:55 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-05-16 16:51 [PATCH 0/2] slimbus: Add QCOM SLIMBus NGD driver Srinivas Kandagatla 2018-05-16 16:51 ` Srinivas Kandagatla 2018-05-16 16:51 ` [PATCH 1/2] slimbus: ngd: dt-bindings: Add slim ngd dt bindings Srinivas Kandagatla 2018-05-16 16:51 ` Srinivas Kandagatla 2018-05-18 20:47 ` Trilok Soni 2018-05-21 8:49 ` Srinivas Kandagatla 2018-05-21 8:49 ` Srinivas Kandagatla 2018-05-23 16:40 ` Rob Herring 2018-05-23 16:40 ` Rob Herring 2018-05-23 17:17 ` Srinivas Kandagatla 2018-05-23 18:11 ` Srinivas Kandagatla 2018-05-23 18:11 ` Srinivas Kandagatla 2018-05-23 19:28 ` Rob Herring 2018-05-23 20:01 ` Srinivas Kandagatla 2018-05-23 20:01 ` Srinivas Kandagatla 2018-05-16 16:51 ` [PATCH 2/2] slimbus: ngd: Add qcom SLIMBus NGD driver Srinivas Kandagatla 2018-05-18 21:39 ` kbuild test robot 2018-05-18 21:39 ` kbuild test robot 2018-05-21 8:54 ` Srinivas Kandagatla 2018-05-21 8:54 ` Srinivas Kandagatla 2018-05-21 11:33 ` Vinod 2018-05-21 11:55 ` Srinivas Kandagatla [this message] 2018-05-21 11:55 ` Srinivas Kandagatla
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=b29bf75f-4846-ef73-b1da-509b62488944@linaro.org \ --to=srinivas.kandagatla@linaro.org \ --cc=alsa-devel@alsa-project.org \ --cc=bgoswami@codeaurora.org \ --cc=broonie@kernel.org \ --cc=devicetree@vger.kernel.org \ --cc=girishm@quicinc.com \ --cc=gregkh@linuxfoundation.org \ --cc=kramasub@codeaurora.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=robh+dt@kernel.org \ --cc=sdharia@quicinc.com \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.