Re: [PATCH v2,7/9] media: mtk-jpegenc: Use component framework to manage each hardware information

On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu [off-list ref] wrote:

+static  const struct of_device_id mtk_jpegenc_drv_ids[] = {

Remove the extra space between "static" and "const".

+       {
+               .compatible = "mediatek,mt8195-jpgenc0",
+               .data = (void *)MTK_JPEGENC_HW0,
+       },
+       {
+               .compatible = "mediatek,mt8195-jpgenc1",
+               .data = (void *)MTK_JPEGENC_HW1,
+       },
+       {},
+};

Should be guarded by CONFIG_OF.

+static struct component_match *mtk_jpegenc_match_add(struct mtk_jpeg_dev *jpeg)
+{
+       struct device *dev = jpeg->dev;
+       struct component_match *match = NULL;
+       int i;
+       char compatible[128] = {0};

It doesn't need to be initialized.

+
+       for (i = 0; i < ARRAY_SIZE(mtk_jpegenc_drv_ids); i++) {
+               struct device_node *comp_node;
+               enum mtk_jpegenc_hw_id comp_idx;
+               const struct of_device_id *of_id;
+
+               memcpy(compatible, mtk_jpegenc_drv_ids[i].compatible,
+                      sizeof(mtk_jpegenc_drv_ids[i].compatible));

Shouldn't rely on the source length.  Also needs to use strcpy-family
for better handling the NULL terminator.

+               if (!of_device_is_available(comp_node)) {
+                       of_node_put(comp_node);
+                       v4l2_err(&jpeg->v4l2_dev, "Fail to get jpeg enc HW node\n");

To be consistent, use "Failed".

+               of_id = of_match_node(mtk_jpegenc_drv_ids, comp_node);
+               if (!of_id) {
+                       v4l2_err(&jpeg->v4l2_dev, "Failed to get match node\n");
+                       return ERR_PTR(-EINVAL);

Shouldn't it call of_node_put() before returning?

+               comp_idx = (enum mtk_jpegenc_hw_id)of_id->data;
+               v4l2_info(&jpeg->v4l2_dev, "Get component:hw_id(%d),jpeg_dev(0x%p),comp_node(0x%p)\n",
+                         comp_idx, jpeg, comp_node);
+
+               jpeg->component_node[comp_idx] = comp_node;
+
+               component_match_add_release(dev, &match, mtk_vdec_release_of,
+                                           mtk_vdec_compare_of, comp_node);

Shouldn't it need to break if it already found?

+       if (!jpeg->variant->is_encoder) {
+               res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+               jpeg->reg_base[MTK_JPEGENC_HW0] =
+                       devm_ioremap_resource(&pdev->dev, res);

If !is_encoder, why is it still using MTK_JPEGENC_HW0?

+               if (IS_ERR(jpeg->reg_base[MTK_JPEGENC_HW0])) {
+                       ret = PTR_ERR(jpeg->reg_base[MTK_JPEGENC_HW0]);
+                       return ret;

Just return the PTR_ERR if it doesn't need to goto.

-       pm_runtime_enable(&pdev->dev);
+       if (jpeg->variant->is_encoder) {
+               match = mtk_jpegenc_match_add(jpeg);
+               if (IS_ERR_OR_NULL(match))
+                       goto err_vfd_jpeg_register;
+
+               video_set_drvdata(jpeg->vdev, jpeg);
+               platform_set_drvdata(pdev, jpeg);
+               ret = component_master_add_with_match(&pdev->dev,
+                                                     &mtk_jpegenc_ops, match);
+               if (ret < 0)
+                       goto err_vfd_jpeg_register;

Shouldn't it call of_node_put() for un-doing mtk_jpegenc_match_add()?

The patch adds more than 2 fields in the struct.  They also need short
descriptions here.

It is not used.

Probably put in the wrong patch?

+int mtk_jpegenc_init_pm(struct mtk_jpeg_dev *mtkdev)
+{
+       struct platform_device *pdev;
+       struct mtk_jpegenc_pm *pm;
+       struct mtk_jpegenc_clk *jpegenc_clk;
+       struct mtk_jpegenc_clk_info *clk_info;
+       int i, ret;
+
+       pdev = mtkdev->plat_dev;
+       pm->dev = &pdev->dev;
+       pm = &mtkdev->pm;
+       pm->mtkdev = mtkdev;
+       jpegenc_clk = &pm->venc_clk;

Could they be inlined to above where the variables are declared.