On Tue, Jul 27, 2021 at 06:10:40PM +0800, Yunfei Dong wrote:

+static struct component_match *mtk_vcodec_match_add(
+     struct mtk_vcodec_dev *vdec_dev)
+ {

Remove the extra space before {.

+     struct platform_device *pdev = vdec_dev->plat_dev;
+     struct component_match *match = NULL;
+     int i;
+
+     for (i = 0; i < ARRAY_SIZE(mtk_vdec_drv_ids); i++) {
+             struct device_node *comp_node;
+             enum mtk_vdec_hw_id comp_idx;
+             const struct of_device_id *of_id;

Failed to see benifits to declare here in the case.  To be neat, move
the variable declaration to the beginning of function.

+
+             comp_node = of_find_compatible_node(NULL, NULL,
+                     mtk_vdec_drv_ids[i].compatible);
+             if (!comp_node)
+                     continue;

If moving of_id assignment to the beginning of for-loop, use of_id->compatible.

+
+             if (!of_device_is_available(comp_node)) {
+                     of_node_put(comp_node);
+                     dev_err(&pdev->dev, "Fail to get MMSYS node\n");
+                     continue;
+             }
+
+             of_id = &mtk_vdec_drv_ids[i];
+             if (!of_id) {
+                     dev_err(&pdev->dev, "Failed to get match node\n");
+                     return ERR_PTR(-EINVAL);
+             }

Move the block to the beginning of for-loop.  Looking to other blocks
around, does it make more sense to use `continue` instead of `return`?

+
+             comp_idx = (enum mtk_vdec_hw_id)of_id->data;
+             mtk_v4l2_debug(4, "Get component:hw_id(%d),vdec_dev(0x%p),comp_node(0x%p)\n",
+                     comp_idx, vdec_dev, comp_node);
+             vdec_dev->component_node[comp_idx] = comp_node;
+
+             component_match_add_release(&pdev->dev, &match, mtk_vdec_release_of,
+                     mtk_vdec_compare_of, comp_node);
+             of_node_put(comp_node);

I thought it shouldn't call of_node_put(...) which is already
delegated to component framework (mtk_vdec_release_of).

+     }
+
+   return match;

Fix the indent.

+static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
+{
+     struct platform_device *pdev = dev->plat_dev;
+     int ret;
+
+     ret = mtk_vcodec_get_reg_bases(dev);
+     if (ret)
+             return ret;
+
+     if (!dev->is_comp_supported) {
+             dev->dec_irq = platform_get_irq(pdev, 0);
+             if (dev->dec_irq < 0) {
+                     dev_err(&pdev->dev, "failed to get irq number");
+                     return dev->dec_irq;
+             }
+
+             irq_set_status_flags(dev->dec_irq, IRQ_NOAUTOEN);
+             ret = devm_request_irq(&pdev->dev, dev->dec_irq,
+                             mtk_vcodec_dec_irq_handler, 0, pdev->name, dev);
+             if (ret) {
+                     dev_err(&pdev->dev, "failed to install dev->dec_irq %d (%d)",
+                             dev->dec_irq, ret);
+                     return ret;
+             }
+
+             ret = mtk_vcodec_init_dec_pm(pdev, &dev->pm);
+             if (ret < 0) {
+                     dev_err(&pdev->dev, "failed to get mt vcodec clock source");
+                     return ret;
+             }
+     }
+
+     return ret;

To be concise, return 0.

+     comp_node =
+             of_find_compatible_node(NULL, NULL, "mediatek,mtk-vcodec-core");
+     if (!comp_node)
+             dev->is_comp_supported = false;
+     else
+             dev->is_comp_supported = true;
+     of_node_put(comp_node);

Looking up "mediatek,mtk-vcodec-core" to determine if it uses
component framework sounds like a sub-optimal method.

Has asked the same question in [1].  Why it removes the
platform_set_drvdata() above?  mtk_vcodec_init_master() also calls
platform_set_drvdata().

[1]: https://patchwork.linuxtv.org/project/linux-media/patch/20210707062157.21176-4-yunfei.dong@mediatek.com/

+     return 0;
+err_component_match:
+     video_unregister_device(vfd_dec);

Why video_unregister_device()?  It is already called [2].

[2]: https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c#L344

Shouldn't remove it.  mtk_vcodec_init_dec_params() also needs to undo
in the path.

Refactoring them to mtk_vcodec_init_dec_params() makes the error
handling more complicated.

If moving to mtk_vcodec_dec_hw.h or mtk_vcodec_dec_hw.c makes more sense?

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