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
prev parent reply other threads:[~2018-05-21 11:55 UTC|newest]
Thread overview: 14+ 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 ` [PATCH 1/2] slimbus: ngd: dt-bindings: Add slim ngd dt bindings Srinivas Kandagatla
2018-05-18 20:47 ` Trilok Soni
2018-05-21 8:49 ` Srinivas Kandagatla
2018-05-23 16:40 ` Rob Herring
2018-05-23 17:17 ` 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-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-21 8:54 ` Srinivas Kandagatla
2018-05-21 11:33 ` Vinod
2018-05-21 11:55 ` Srinivas Kandagatla [this message]
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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).