Hi Angelo,
thanks for the review.
On Thu, 2021-10-14 at 16:27 +0200, AngeloGioacchino Del Regno wrote:
Il 21/09/21 17:52, jason-jh.lin ha scritto:
Add MERGE engine file:
[snip]
+int mtk_merge_clk_enable(struct device *dev) +{
- int ret = 0;
- struct mtk_disp_merge *priv = dev_get_drvdata(dev);
- ret = clk_prepare_enable(priv->clk);
- if (ret)
pr_err("merge clk prepare enable failed\n");If you failed to enable this clock, I take it as the hardware won't work or won't work as expected, hence you should return a failure before trying to call prepare_enable for async_clk.
OK I'll fix it.
- ret = clk_prepare_enable(priv->async_clk);
- if (ret)
pr_err("async clk prepare enable failed\n");You should also return a failure here but, before that, you should clean up the state by calling clk_disable_unprepare(priv->clk), or you will leave it enabled, eventually getting a hardware fault later on (which may or may not result in a board reboot), or other sorts of unexpected states.
At least, you will get issues with the refcount for "clk" and/or "async_clk".
Please fix that.
Also, please use dev_err or, more appropriately, DRM_ERROR instead or pr_err.
OK I'll fix it .
- return ret;
+}
+void mtk_merge_clk_disable(struct device *dev) +{
- struct mtk_disp_merge *priv = dev_get_drvdata(dev);
- clk_disable_unprepare(priv->async_clk); > + clk_disable_unprepa
re(priv->clk); +}
+static int mtk_disp_merge_bind(struct device *dev, struct device *master,
void *data)+{
- return 0;
+}
+static void mtk_disp_merge_unbind(struct device *dev, struct device *master,
void *data)+{ +}
+static const struct component_ops mtk_disp_merge_component_ops = {
- .bind = mtk_disp_merge_bind,
- .unbind = mtk_disp_merge_unbind,
+};
+static int mtk_disp_merge_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev;
- struct resource *res;
- struct mtk_disp_merge *priv;
- int ret;
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
return -ENOMEM;- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- priv->regs = devm_ioremap_resource(dev, res);
- if (IS_ERR(priv->regs)) {
dev_err(dev, "failed to ioremap merge\n");return PTR_ERR(priv->regs);- }
- priv->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(priv->clk)) {
dev_err(dev, "failed to get merge clk\n");return PTR_ERR(priv->clk);- }
- priv->async_clk = of_clk_get(dev->of_node, 1);
- if (IS_ERR(priv->async_clk)) {
ret = PTR_ERR(priv->async_clk);dev_dbg(dev, "No merge async clock: %d\n", ret);priv->async_clk = NULL;- }
You are using devm_clk_get for the first clock, of_clk_get for the second one: what's the reason for that?
Also, async_clk seems to be optional... and there's the right API for you! If you use devm_clk_get_optional(), you won't have to manually assign NULL to priv->async_clk, as that's API handled... and you'll get a failure if the return value is an error that's not -ENOENT (so, it'll fail if the clock was declared in DT, but there was an error acquiring it).
Please use devm_clk_get_optional() here.
Yes, async_clk is optional. Thanks for your suggestion. I'll try it.
Regards,
- Angelo