Re: [PATCH v11 15/16] drm/mediatek: add MERGE support for mediatek-drm

[Jason-JH Lin <jason-jh.lin@mediatek.com>]

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@mediatek.com>


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek