On Wed, Jul 7, 2021 at 2:22 PM Yunfei Dong [off-list ref] wrote:

+#include "mtk_vcodec_util.h"
+
 #include <media/videobuf2-core.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-mem2mem.h>

The changes look like independent ones.  If any .c files need the
headers, include them in the .c files instead of here.

+               comp_node = of_find_compatible_node(NULL, NULL,
+                       mtk_vdec_drv_ids[i].compatible);
+               if (!comp_node)
+                       continue;
+
+               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 = of_match_node(mtk_vdec_drv_ids, comp_node);
+               if (!of_id) {

Doesn't it need to call of_node_put(comp_node)?

+static int mtk_vcodec_init_master(struct mtk_vcodec_dev *dev)
+{
+       struct platform_device *pdev = dev->plat_dev;
+       struct component_match *match;
+       int ret = 0;

ret doesn't need to be initialized.

+       match = mtk_vcodec_match_add(dev);
+       if (IS_ERR_OR_NULL(match))
+               return -EINVAL;
+
+       platform_set_drvdata(pdev, dev);

Why does platform_set_drvdata() need to be here?  The function neither
creates pdev nor dev.

+static int mtk_vcodec_init_dec_params(struct mtk_vcodec_dev *dev)
+{
+       struct platform_device *pdev = dev->plat_dev;
+       struct resource *res;
+       int ret = 0;

ret doesn't need to be initialized.

+       if (!dev->is_support_comp) {
+               res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+               if (res == NULL) {

!res, res is not used BTW.

+               dev->dec_irq = platform_get_irq(dev->plat_dev, 0);

Check return value.

+               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);

Can join to previous line.

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

Need a DT binding document patch for the attribute.

Does it really need to call of_find_compatible_node() for parsing an
attribute?  If so, it needs to call of_node_put() afterward.

Why does it need to remove platform_set_drvdata()?

mtk_vcodec_init_master() also calls platform_set_drvdata().  What is
the difference?

+       /* clear interrupt */
+       writel((readl(vdec_misc_addr) | VDEC_IRQ_CFG), vdec_misc_addr);
+       writel((readl(vdec_misc_addr) & ~VDEC_IRQ_CLR), vdec_misc_addr);

Can remove 1 parenthese pair.

+static int mtk_vdec_comp_probe(struct platform_device *pdev)
+{
+       struct mtk_vdec_comp_dev *dev;
+       int ret;
+
+       dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+       if (!dev)
+               return -ENOMEM;
+
+       dev->plat_dev = pdev;
+       spin_lock_init(&dev->irqlock);
+
+       ret = mtk_vcodec_init_dec_pm(dev->plat_dev, &dev->pm);

To be concise, use pdev.

+       dev->reg_base[VDEC_COMP_MISC] =
+               devm_platform_ioremap_resource(pdev, 0);

Confusing about the index 0, where:
VDEC_COMP_SYS = 0
VDEC_COMP_MISC = 1

+#ifndef _MTK_VCODEC_DEC_HW_H_
+#define _MTK_VCODEC_DEC_HW_H_
+
+#include <linux/component.h>

Does it really need to include component.h?

+/**
+ * enum mtk_comp_hw_reg_idx - component register base index
+ */
+enum mtk_comp_hw_reg_idx {
+       VDEC_COMP_SYS,
+       VDEC_COMP_MISC,
+       NUM_MAX_COMP_VCODEC_REG_BASE

The name is suboptimal.  How about VDEC_COMP_MAX or VDEC_COMP_LAST?

+#include <linux/component.h>
+#include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/videodev2.h>
 #include <media/v4l2-ctrls.h>

The newly added code in the file doesn't look like it needs anything
from component.h and io.h.

is_support_comp is a boolean.  Use true and false instead of 1 and 0.

To be neat, missing "@" before each symbol name.

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