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
--
Regards,
Jason-JH Lin <jason-jh.lin@xxxxxxxxxxxx>
