[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1475204778.13398.28.camel@mtksdaap41>
Date: Fri, 30 Sep 2016 11:06:18 +0800
From: CK Hu <ck.hu@...iatek.com>
To: HS Liao <hs.liao@...iatek.com>
CC: Rob Herring <robh+dt@...nel.org>,
Matthias Brugger <matthias.bgg@...il.com>,
Jassi Brar <jassisinghbrar@...il.com>,
"Daniel Kurtz" <djkurtz@...omium.org>,
Sascha Hauer <s.hauer@...gutronix.de>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-mediatek@...ts.infradead.org>,
<srv_heupstream@...iatek.com>,
Sascha Hauer <kernel@...gutronix.de>,
"Philipp Zabel" <p.zabel@...gutronix.de>,
Nicolas Boichat <drinkcat@...omium.org>,
cawa cheng <cawa.cheng@...iatek.com>,
Bibby Hsieh <bibby.hsieh@...iatek.com>,
YT Shen <yt.shen@...iatek.com>,
Daoyuan Huang <daoyuan.huang@...iatek.com>,
Damon Chu <damon.chu@...iatek.com>,
"Josh-YC Liu" <josh-yc.liu@...iatek.com>,
Glory Hung <glory.hung@...iatek.com>,
Jiaguang Zhang <jiaguang.zhang@...iatek.com>,
Dennis-YC Hsieh <dennis-yc.hsieh@...iatek.com>,
Monica Wang <monica.wang@...iatek.com>
Subject: Re: [PATCH v14 2/4] CMDQ: Mediatek CMDQ driver
Hi, HS:
On Mon, 2016-09-05 at 09:44 +0800, HS Liao wrote:
> This patch is first version of Mediatek Command Queue(CMDQ) driver. The
> CMDQ is used to help write registers with critical time limitation,
> such as updating display configuration during the vblank. It controls
> Global Command Engine (GCE) hardware to achieve this requirement.
> Currently, CMDQ only supports display related hardwares, but we expect
> it can be extended to other hardwares for future requirements.
>
> Signed-off-by: HS Liao <hs.liao@...iatek.com>
> Signed-off-by: CK Hu <ck.hu@...iatek.com>
> ---
[snip...]
> +
> +struct cmdq_task {
> + struct cmdq *cmdq;
> + struct list_head list_entry;
> + void *va_base;
> + dma_addr_t pa_base;
> + size_t cmd_buf_size; /* command occupied size */
> + size_t buf_size; /* real buffer size */
> + bool finalized;
> + struct cmdq_thread *thread;
I think thread info could be removed from cmdq_task. Only
cmdq_task_handle_error() and cmdq_task_insert_into_thread() use
task->thread and caller of both function has the thread info. So you
could just pass thread info into these two function and remove thread
info in cmdq_task.
> + struct cmdq_task_cb cb;
I think this callback function is equal to mailbox client tx_done
callback. It's better to use already-defined interface rather than
creating your own.
> +};
> +
[snip...]
> +
> +static int cmdq_suspend(struct device *dev)
> +{
> + struct cmdq *cmdq = dev_get_drvdata(dev);
> + struct cmdq_thread *thread;
> + int i;
> + bool task_running = false;
> +
> + mutex_lock(&cmdq->task_mutex);
> + cmdq->suspended = true;
> + mutex_unlock(&cmdq->task_mutex);
> +
> + for (i = 0; i < ARRAY_SIZE(cmdq->thread); i++) {
> + thread = &cmdq->thread[i];
> + if (!list_empty(&thread->task_busy_list)) {
> + mod_timer(&thread->timeout, jiffies + 1);
> + task_running = true;
> + }
> + }
> +
> + if (task_running) {
> + dev_warn(dev, "exist running task(s) in suspend\n");
> + msleep(20);
Why sleep here? It looks like a recovery but could 20ms recovery
something? I think warning message is enough because you see the warning
message, and you fix the bug, so no need to recovery anything.
> + }
> +
> + clk_unprepare(cmdq->clock);
> + return 0;
> +}
> +
Regards,
CK
Powered by blists - more mailing lists