Dear Sylwester Nawrocki
Thank you for your update. I have some question of your patch.
please give your information to me.
Thank's
BR
Eunchul Kim.
On 04/17/2013 06:53 PM, Sylwester Nawrocki wrote:
The clocks handling is refactored and a "mux" clock handling is
added to account for changes in the clocks driver. After switching
to the common clock framework the sclk_fimc clock is now split
into two clocks: a gate and a mux clock. In order to retain the
exisiting functionality two additional consumer clocks are passed
to the driver from device tree: "mux" and "parent". Then the driver
sets "parent" clock as a parent clock of the "mux" clock. These two
additional clocks are optional, and should go away when there is a
standard way of setting up parent clocks on DT platforms.
Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
---
drivers/gpu/drm/exynos/exynos_drm_fimc.c | 167 +++++++++++++++++-------------
1 file changed, 97 insertions(+), 70 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index d812c57..bc8411a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -76,6 +76,27 @@ enum fimc_wb {
FIMC_WB_B,
};
+enum {
+ FIMC_CLK_LCLK,
+ FIMC_CLK_GATE,
+ FIMC_CLK_WB_A,
+ FIMC_CLK_WB_B,
+ FIMC_CLK_MUX,
+ FIMC_CLK_PARENT,
- What is MUX, PARENT ?
+ FIMC_CLKS_MAX
+};
+
+static const char * fimc_clock_names[] = {
+ [FIMC_CLK_LCLK] = "sclk_fimc",
+ [FIMC_CLK_GATE] = "fimc",
+ [FIMC_CLK_WB_A] = "pxl_async0",
+ [FIMC_CLK_WB_B] = "pxl_async1",
+ [FIMC_CLK_MUX] = "mux",
+ [FIMC_CLK_PARENT] = "parent",
- How can I get "mux", "parent" clock information ?
Normally we are using "mout_mpll" in exynos4210, "mout_mpll_user" in
exynos 4412. I want to get this two kind of clock name information.
+};
+
+#define FIMC_DEFAULT_LCLK_FREQUENCY 133000000UL
- When do I use this value in the patch ?
If not use. please remove this macro. If you want to use this value.
please use platform data instead of macro.
+
/*
* A structure of scaler.
*
@@ -132,15 +153,12 @@ struct fimc_driverdata {
*
* @ippdrv: prepare initialization using ippdrv.
* @regs_res: register resources.
+ * @dev: pointer to the fimc device structure.
- We already set the dev information in ippdrv structure.
I think this value is duplicated value.
* @regs: memory mapped io registers.
* @lock: locking of operations.
- * @sclk_fimc_clk: fimc source clock.
- * @fimc_clk: fimc clock.
- * @wb_clk: writeback a clock.
- * @wb_b_clk: writeback b clock.
+ * @clocks: fimc clocks.
+ * @clk_frequency: LCLK clock frequency.
* @sc: scaler infomations.
- * @odr: ordering of YUV.
- * @ver: fimc version.
* @pol: porarity of writeback.
* @id: fimc id.
* @irq: irq number.
@@ -148,13 +166,12 @@ struct fimc_driverdata {
*/
struct fimc_context {
struct exynos_drm_ippdrv ippdrv;
+ struct device *dev;
- please check this value about really need ?
struct resource *regs_res;
void __iomem *regs;
struct mutex lock;
- struct clk *sclk_fimc_clk;
- struct clk *fimc_clk;
- struct clk *wb_clk;
- struct clk *wb_b_clk;
+ struct clk *clocks[FIMC_CLKS_MAX];
+ u32 clk_frequency;
struct fimc_scaler sc;
struct fimc_driverdata *ddata;
struct exynos_drm_ipp_pol pol;
@@ -1301,14 +1318,12 @@ static int fimc_clk_ctrl(struct fimc_context *ctx, bool enable)
DRM_DEBUG_KMS("%s:enable[%d]\n", __func__, enable);
if (enable) {
- clk_enable(ctx->sclk_fimc_clk);
- clk_enable(ctx->fimc_clk);
- clk_enable(ctx->wb_clk);
+ clk_prepare_enable(ctx->clocks[FIMC_CLK_GATE]);
+ clk_prepare_enable(ctx->clocks[FIMC_CLK_WB_A]);
ctx->suspended = false;
} else {
- clk_disable(ctx->sclk_fimc_clk);
- clk_disable(ctx->fimc_clk);
- clk_disable(ctx->wb_clk);
+ clk_disable_unprepare(ctx->clocks[FIMC_CLK_GATE]);
+ clk_disable_unprepare(ctx->clocks[FIMC_CLK_WB_A]);
ctx->suspended = true;
}
@@ -1713,11 +1728,66 @@ static void fimc_ippdrv_stop(struct device *dev, enum drm_exynos_ipp_cmd cmd)
fimc_write(cfg, EXYNOS_CIGCTRL);
}
+static void fimc_put_clocks(struct fimc_context *ctx)
+{
+ int i;
+
+ for (i = 0; i < FIMC_CLKS_MAX; i++) {
+ if (IS_ERR(ctx->clocks[i]))
+ continue;
+ clk_put(ctx->clocks[i]);
+ ctx->clocks[i] = ERR_PTR(-EINVAL);
+ }
+}
+
+static int fimc_setup_clocks(struct fimc_context *ctx)
+{
+ struct device *dev;
+ int ret, i;
+
+ for (i = 0; i < FIMC_CLKS_MAX; i++)
+ ctx->clocks[i] = ERR_PTR(-EINVAL);
+
+ for (i = 0; i < FIMC_CLKS_MAX; i++) {
+ if (i == FIMC_CLK_WB_A || i == FIMC_CLK_WB_B)
+ dev = ctx->dev->parent;
+ else
+ dev = ctx->dev;
+
+ ctx->clocks[i] = clk_get(dev, fimc_clock_names[i]);
+ if (IS_ERR(ctx->clocks[i])) {
+ if (i >= FIMC_CLK_MUX)
+ break;
+ ret = PTR_ERR(ctx->clocks[i]);
+ goto e_clk_free;
+ }
+ }
+
+ if (!IS_ERR(ctx->clocks[FIMC_CLK_PARENT])) {
+ ret = clk_set_parent(ctx->clocks[FIMC_CLK_MUX],
+ ctx->clocks[FIMC_CLK_PARENT]);
- I can't review this code.
+ if (ret < 0) {
+ dev_err(ctx->dev, "failed to set parent: %d\n", ret);
+ return ret;
+ }
+ }
+
+ ret = clk_set_rate(ctx->clocks[FIMC_CLK_LCLK], ctx->clk_frequency);
- When do I set clk_frequency value ? and why doesn't use pdata->clk_rate ?
+ if (ret < 0)
+ return ret;
+
+ return clk_prepare_enable(ctx->clocks[FIMC_CLK_LCLK]);
+
+e_clk_free:
+ dev_err(ctx->dev, "failed to get clock: %s\n", fimc_clock_names[i]);
+ fimc_put_clocks(ctx);
+ return ret;
+}
+
static int fimc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct fimc_context *ctx;
- struct clk *parent_clk;
struct resource *res;
struct exynos_drm_ippdrv *ippdrv;
struct exynos_drm_fimc_pdata *pdata;
@@ -1734,55 +1804,6 @@ static int fimc_probe(struct platform_device *pdev)
if (!ctx)
return -ENOMEM;
- ddata = (struct fimc_driverdata *)
- platform_get_device_id(pdev)->driver_data;
-
- /* clock control */
- ctx->sclk_fimc_clk = devm_clk_get(dev, "sclk_fimc");
- if (IS_ERR(ctx->sclk_fimc_clk)) {
- dev_err(dev, "failed to get src fimc clock.\n");
- return PTR_ERR(ctx->sclk_fimc_clk);
- }
- clk_enable(ctx->sclk_fimc_clk);
-
- ctx->fimc_clk = devm_clk_get(dev, "fimc");
- if (IS_ERR(ctx->fimc_clk)) {
- dev_err(dev, "failed to get fimc clock.\n");
- clk_disable(ctx->sclk_fimc_clk);
- return PTR_ERR(ctx->fimc_clk);
- }
-
- ctx->wb_clk = devm_clk_get(dev, "pxl_async0");
- if (IS_ERR(ctx->wb_clk)) {
- dev_err(dev, "failed to get writeback a clock.\n");
- clk_disable(ctx->sclk_fimc_clk);
- return PTR_ERR(ctx->wb_clk);
- }
-
- ctx->wb_b_clk = devm_clk_get(dev, "pxl_async1");
- if (IS_ERR(ctx->wb_b_clk)) {
- dev_err(dev, "failed to get writeback b clock.\n");
- clk_disable(ctx->sclk_fimc_clk);
- return PTR_ERR(ctx->wb_b_clk);
- }
-
- parent_clk = devm_clk_get(dev, ddata->parent_clk);
-
- if (IS_ERR(parent_clk)) {
- dev_err(dev, "failed to get parent clock.\n");
- clk_disable(ctx->sclk_fimc_clk);
- return PTR_ERR(parent_clk);
- }
-
- if (clk_set_parent(ctx->sclk_fimc_clk, parent_clk)) {
- dev_err(dev, "failed to set parent.\n");
- clk_disable(ctx->sclk_fimc_clk);
- return -EINVAL;
- }
-
- devm_clk_put(dev, parent_clk);
- clk_set_rate(ctx->sclk_fimc_clk, pdata->clk_rate);
-
/* resource memory */
ctx->regs_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
ctx->regs = devm_ioremap_resource(dev, ctx->regs_res);
@@ -1804,6 +1825,9 @@ static int fimc_probe(struct platform_device *pdev)
return ret;
}
+ ret = fimc_setup_clocks(ctx);
+ if (ret < 0)
+ goto err_free_irq;
- please blank line and error log.
/* context initailization */
ctx->id = pdev->id;
ctx->pol = pdata->pol;
@@ -1820,7 +1844,7 @@ static int fimc_probe(struct platform_device *pdev)
ret = fimc_init_prop_list(ippdrv);
if (ret < 0) {
dev_err(dev, "failed to init property list.\n");
- goto err_get_irq;
+ goto err_put_clk;
}
DRM_DEBUG_KMS("%s:id[%d]ippdrv[0x%x]\n", __func__, ctx->id,
@@ -1835,16 +1859,18 @@ static int fimc_probe(struct platform_device *pdev)
ret = exynos_drm_ippdrv_register(ippdrv);
if (ret < 0) {
dev_err(dev, "failed to register drm fimc device.\n");
- goto err_ippdrv_register;
+ goto err_pm_dis;
}
dev_info(&pdev->dev, "drm fimc registered successfully.\n");
return 0;
-err_ippdrv_register:
+err_pm_dis:
pm_runtime_disable(dev);
-err_get_irq:
+err_put_clk:
+ fimc_put_clocks(ctx);
+err_free_irq:
free_irq(ctx->irq, ctx);
return ret;
@@ -1859,6 +1885,7 @@ static int fimc_remove(struct platform_device *pdev)
exynos_drm_ippdrv_unregister(ippdrv);
mutex_destroy(&ctx->lock);
+ fimc_put_clocks(ctx);
pm_runtime_set_suspended(dev);
pm_runtime_disable(dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
