Re: [PATCH v11, 01/19] media: mtk-vcodec: Get numbers of register bases from DT

[Steve Cho <stevecho@xxxxxxxxxx>]

Thanks,

Steve

On Wed, Dec 1, 2021 at 6:38 PM yunfei.dong@xxxxxxxxxxxx <yunfei.dong@xxxxxxxxxxxx> wrote:

Hi Steve,

Thanks for your suggestion.

On Wed, 2021-12-01 at 15:36 -0800, Steve Cho wrote:

LGTM with few nits. 

Thanks,

Steve

On Sun, Nov 28, 2021 at 7:44 PM Yunfei Dong <yunfei.dong@xxxxxxxxxxxx> wrote:

Different platform may has different numbers of register bases. Gets the
numbers of register bases from DT (sizeof(u32) * 4 bytes for each)

Few nits.

s/platform/platforms/

s/has/have/

Fix, DT is dts.

Btw, what is DT?

 
Reviewed-by: Tzung-Bi Shih<tzungbi@xxxxxxxxxx>
Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
---
 .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  | 37 ++++++++++++++-----
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index e6e6a8203eeb..59caf2163349 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -78,6 +78,30 @@ static irqreturn_t mtk_vcodec_dec_irq_handler(int irq, void *priv)
        return IRQ_HANDLED;
 }

+static int mtk_vcodec_get_reg_bases(struct mtk_vcodec_dev *dev)
+{

I see that dev is already checked before entering into this function, but null check for dev would still be nice. 

 

Dev is never null in this function, whether it looks not very reasonable?

That is correct with current code. In Chromium world, reviewers still suggest to null check in similar situations. 

Up to you. It is fine with me as it is. 

 

Best Regards,

Yunfei Dong

+       struct platform_device *pdev = dev->plat_dev;
+       int reg_num, i;
+
+       /* Sizeof(u32) * 4 bytes for each register base. */
+       reg_num = of_property_count_elems_of_size(pdev->dev.of_node, "reg",
+               sizeof(u32) * 4);
+       if (reg_num <= 0 || reg_num > NUM_MAX_VDEC_REG_BASE) {
+               dev_err(&pdev->dev, "Invalid register property size: %d\n", reg_num);
+               return -EINVAL;
+       }
+
+       for (i = 0; i < reg_num; i++) {
+               dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i);
+               if (IS_ERR(dev->reg_base[i]))
+                       return PTR_ERR(dev->reg_base[i]);
+
+               mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]);
+       }
+
+       return 0;
+}
+
 static int fops_vcodec_open(struct file *file)
 {
        struct mtk_vcodec_dev *dev = video_drvdata(file);
@@ -206,7 +230,7 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
        struct resource *res;
        phandle rproc_phandle;
        enum mtk_vcodec_fw_type fw_type;
-       int i, ret;
+       int ret;

        dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
        if (!dev)
@@ -238,14 +262,9 @@ static int mtk_vcodec_probe(struct platform_device *pdev)
                goto err_dec_pm;
        }

-       for (i = 0; i < NUM_MAX_VDEC_REG_BASE; i++) {
-               dev->reg_base[i] = devm_platform_ioremap_resource(pdev, i);
-               if (IS_ERR((__force void *)dev->reg_base[i])) {
-                       ret = PTR_ERR((__force void *)dev->reg_base[i]);
-                       goto err_res;
-               }
-               mtk_v4l2_debug(2, "reg[%d] base=%p", i, dev->reg_base[i]);
-       }
+       ret = mtk_vcodec_get_reg_bases(dev);
+       if (ret)
+               goto err_res;

        res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
        if (res == NULL) {