Re: [PATCH v1, 03/14] media: mtk-vcodec: Use component framework to manage each hardware information

[Tzung-Bi Shih <tzungbi@google.com>]

On Wed, Jul 7, 2021 at 2:22 PM Yunfei Dong <yunfei.dong@mediatek.com> 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.

> @@ -319,7 +434,6 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>                 MTK_VCODEC_DEC_NAME);
>         video_set_drvdata(vfd_dec, dev);
>         dev->vfd_dec = vfd_dec;
> -       platform_set_drvdata(pdev, dev);
Why does it need to remove platform_set_drvdata()?

> @@ -370,8 +484,17 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
>         mtk_v4l2_debug(0, "decoder registered as /dev/video%d",
>                 vfd_dec->num);
>
> -       return 0;
> +       if (dev->is_support_comp) {
> +               ret = mtk_vcodec_init_master(dev);
> +               if (ret < 0)
> +                       goto err_component_match;
> +       } else {
> +               platform_set_drvdata(pdev, dev);
> +       }
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.

> @@ -404,6 +422,7 @@ struct mtk_vcodec_enc_pdata {
>   *
>   * @fw_handler: used to communicate with the firmware.
>   * @id_counter: used to identify current opened instance
> + * @is_support_comp: 1: using compoent framework, 0: not support
is_support_comp is a boolean.  Use true and false instead of 1 and 0.

> @@ -422,6 +441,10 @@ struct mtk_vcodec_enc_pdata {
>   * @pm: power management control
>   * @dec_capability: used to identify decode capability, ex: 4k
>   * @enc_capability: used to identify encode capability
> + *
> + * comp_dev: component hardware device
> + * component_node: component node
> + * comp_idx: component index
To be neat, missing "@" before each symbol name.