Re: [PATCH 2/3] media: mtk-jpegenc: use component framework to manage jpg HW - Tzung-Bi Shih

From: Tzung-Bi Shih <tzungbi@google.com>
To: "kyrie.wu" <kyrie.wu@mediatek.com>
Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Rick Chang <rick.chang@mediatek.com>,
	Bin Liu <bin.liu@mediatek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Tzung-Bi Shih <tzungbi@chromium.org>,
	Project_Global_Chrome_Upstream_Group@mediatek.com,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	Tomasz Figa <tfiga@chromium.org>,
	xia.jiang@mediatek.com, maoguang.meng@mediatek.com,
	srv_heupstream@mediatek.com
Subject: Re: [PATCH 2/3] media: mtk-jpegenc: use component framework to manage jpg HW
Date: Fri, 25 Jun 2021 17:18:30 +0800	[thread overview]
Message-ID: <CA+Px+wW89v3micrkgNDvxGAad4P+JfRHKnLdPN__qVrV3i-j+w@mail.gmail.com> (raw)
In-Reply-To: <1624428350-1424-3-git-send-email-kyrie.wu@mediatek.com>

On Wed, Jun 23, 2021 at 2:06 PM kyrie.wu <kyrie.wu@mediatek.com> wrote:
> Mtk jpeg encoder has several hardware, one HW may register a device
> node; use component framework to manage jpg HW device node,
> in this case, one device node could represent all jpg HW.
Can roughly understand.  But could you rephrase your sentences?

>  #include <media/videobuf2-core.h>
>  #include <media/videobuf2-dma-contig.h>
>  #include <soc/mediatek/smi.h>
> +#include <linux/component.h>
Maintain the alphabetical order.

> +void mtk_jpeg_put_buf(struct mtk_jpeg_dev *jpeg)
> +{
> +       struct mtk_jpeg_ctx *ctx = NULL;
> +       struct vb2_v4l2_buffer *dst_buffer = NULL;
> +       struct list_head *temp_entry = NULL;
> +       struct list_head *pos = NULL;
> +       struct mtk_jpeg_src_buf *dst_done_buf = NULL, *tmp_dst_done_buf = NULL;
Remove the initialization if they don't need to.

> +       unsigned long flags;
> +
> +       ctx = jpeg->hw_param.curr_ctx;
> +       if (!ctx) {
> +               pr_err("%s : %d, comp_jpeg ctx fail !!!\n", __func__, __LINE__);
Use dev_err().

> +               return;
> +       }
> +
> +       dst_buffer = jpeg->hw_param.dst_buffer;
> +       if (!dst_buffer) {
> +               pr_err("%s : %d, comp_jpeg dst_buffer fail !!!\n",
> +                      __func__, __LINE__);
Use dev_err().

> +       if (!(irq_status & JPEG_ENC_INT_STATUS_DONE)) {
> +               pr_err("%s : %d, jpeg encode failed\n", __func__, __LINE__);
Use dev_err().

> +void mtk_jpegenc_timeout_work(struct work_struct *work)
> +{
> +       struct mtk_jpeg_dev *jpeg = container_of(work, struct mtk_jpeg_dev,
> +               job_timeout_work.work);
> +       struct mtk_jpeg_ctx *ctx = NULL;
It doesn't need to initialize.

> +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,
> +       },
Using compatible strings to separate them doesn't sound like a scalable method.

>  #include <linux/kernel.h>
>  #include <media/videobuf2-core.h>
>  #include <media/videobuf2-dma-contig.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/component.h>
> +#include <linux/clk.h>
> +#include <linux/pm_runtime.h>
Maintain the alphabetical order.

>  #include "mtk_jpeg_enc_hw.h"
> +#include "mtk_jpeg_core.h"
Maintain the alphabetical order.

> +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 = 0, ret = 0;
They don't need to initialize.

> +       pdev = mtkdev->plat_dev;
> +       pm = &mtkdev->pm;
To be concise, can inline to above when declaring the variables.

> +       jpegenc_clk->clk_num =
> +               of_property_count_strings(pdev->dev.of_node, "clock-names");
> +       if (jpegenc_clk->clk_num > 0) {
> +               jpegenc_clk->clk_info = devm_kcalloc(&pdev->dev,
> +                                                    jpegenc_clk->clk_num,
> +                                                    sizeof(*clk_info),
> +                                                    GFP_KERNEL);
> +               if (!jpegenc_clk->clk_info)
> +                       return -ENOMEM;
> +       } else {
> +               pr_err("Failed to get jpegenc clock count\n");
Use dev_err().
> +               return -EINVAL;
> +       }
Would prefer the block turn to be:

if (... <= 0) {
    ...
    return -EINVAL;
}

... = devm_kcalloc(...);
if (!...)
    return -ENOMEM;

> +       for (i = 0; i < jpegenc_clk->clk_num; i++) {
> +               clk_info = &jpegenc_clk->clk_info[i];
> +               ret = of_property_read_string_index(pdev->dev.of_node,
> +                                                   "clock-names", i,
> +                                                   &clk_info->clk_name);
> +               if (ret) {
> +                       pr_err("Failed to get jpegenc clock name id = %d", i);
Use dev_err().

> +                       return ret;
> +               }
> +
> +               clk_info->jpegenc_clk = devm_clk_get(&pdev->dev,
> +                                                    clk_info->clk_name);
> +               if (IS_ERR(clk_info->jpegenc_clk)) {
> +                       pr_err("devm_clk_get (%d)%s fail",
> +                              i, clk_info->clk_name);
Use dev_err().

> +       pm_runtime_enable(&pdev->dev);
> +       return ret;
return 0;

> +void mtk_jpegenc_release_pm(struct mtk_jpeg_dev *dev)
> +{
> +       pm_runtime_disable(dev->pm.dev);
> +}
Would prefer this function to be more "symmetric" to mtk_jpegenc_init_pm().

For example:

void mtk_jpegenc_release_pm(struct mtk_jpeg_dev *mtkdev)
{
    struct platform_device *pdev = mtkdev->plat_dev;
    pm_runtime_disable(&pdev->dev);
}

That way, it doesn't rely on whether mtkdev->pm is set or not.

> +       ret = devm_request_irq(&pdev->dev, dev->jpegenc_irq,
> +                              irq_handler, 0, pdev->name, dev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to install dev->jpegenc_irq %d (%d)",
> +                       dev->jpegenc_irq, ret);
> +
> +               return -ENOENT;
How about just returning ret?

> +       }
> +
> +       //disable_irq(dev->jpegenc_irq);
Remove it.

> +       ret = component_add(&pdev->dev, &mtk_jpegenc_hw_component_ops);
> +       if (ret) {
> +               dev_err(&pdev->dev, "Failed to component_add: %d\n", ret);
> +               goto err;
> +       }
How about just returning component_add(...)?

> +err:
> +       mtk_jpegenc_release_pm(dev);
Would expect the platform driver to have a .remove() callback and
invoke the mtk_jpegenc_release_pm() too.

> +static const struct of_device_id mtk_jpegenc_hw_ids[] = {
> +       {
> +               .compatible = "mediatek,mt8195-jpgenc0",
> +               .data = mtk_jpegenc_hw_irq_handler,
> +       },
> +       {       .compatible = "mediatek,mt8195-jpgenc1",
> +               .data = mtk_jpegenc_hw_irq_handler,
> +       },
> +       {},
> +};
> +MODULE_DEVICE_TABLE(of, mtk_jpegenc_hw_ids);
Had the same concern in dt-bindings patch.  Does it really need
multiple compatible strings to separate?

Also, the block should guard by using CONFIG_OF.

> +struct platform_driver mtk_jpegenc_hw_driver = {
> +       .probe  = mtk_jpegenc_hw_probe,
> +       .driver = {
> +               .name   = "mtk-jpegenc-hw",
> +               .of_match_table = mtk_jpegenc_hw_ids,
Should guard by using of_match_ptr().

Hi, after reading the patch for a while, I realized it is way too big
to me so that I didn't go through too much detail (especially the
component framework part).  Could you further divide the series into
smaller pieces?  For example:
- part i. refactor to make modifying code easier
- part ii. leverage component framework
- part iii. add new code for MT8195
I would expect part i and ii don't change the original behavior.