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:

quoted

Add MERGE engine file:

[snip]

quoted

+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.

quoted

+	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 .

quoted

+	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

-- 
Regards,
Jason-JH Lin [off-list ref]


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel