On Wed, Jun 30, 2021 at 3:28 PM kyrie.wu <kyrie.wu@xxxxxxxxxxxx> 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()?
> --- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> +++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> @@ -125,6 +125,8 @@ struct mtk_jpegenc_pm {
> * @larb: SMI device
> * @job_timeout_work: IRQ timeout structure
> * @variant: driver variant to be used
> + * @irqlock: spinlock protecting for irq
> + * @dev_mutex: the mutex protecting for device
The patch adds more than 2 fields in the struct. They also need short
descriptions here.
> */
> struct mtk_jpeg_dev {
> struct mutex lock;
> @@ -136,12 +138,18 @@ struct mtk_jpeg_dev {
> void *alloc_ctx;
> struct video_device *vdev;
> struct device *larb;
> - struct delayed_work job_timeout_work;
> const struct mtk_jpeg_variant *variant;
>
> + struct clk *clk_jpeg;
It is not used.
> /**
> * struct mtk_jpeg_fmt - driver's internal color format data
> * @fourcc: the fourcc code, 0 if not applicable
> @@ -194,6 +204,7 @@ struct mtk_jpeg_q_data {
> * @enc_quality: jpeg encoder quality
> * @restart_interval: jpeg encoder restart interval
> * @ctrl_hdl: controls handler
> + * @done_queue_lock: spinlock protecting for buffer done queue
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.
