Hi, Krzysztof.

-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Tuesday, February 11, 2025 2:41 AM
To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org;
hverkuil@xs4all.nl; sebastian.fricke@collabora.com; robh@kernel.org;
krzk+dt@kernel.org; conor+dt@kernel.org
Cc: linux-media@vger.kernel.org; devicetree@vger.kernel.org; linux-
kernel@vger.kernel.org; linux-imx@nxp.com; linux-arm-
kernel@lists.infradead.org; jackson.lee [off-list ref];
lafley.kim [off-list ref]
Subject: Re: [PATCH 4/8] media: chips-media: wave6: Add Wave6 codec driver

On 10/02/2025 10:07, Nas Chung wrote:

quoted

+
+	dev->debugfs = debugfs_lookup(WAVE6_VPU_DEBUGFS_DIR, NULL);
+	if (IS_ERR_OR_NULL(dev->debugfs))
+		dev->debugfs = debugfs_create_dir(WAVE6_VPU_DEBUGFS_DIR,

NULL);

quoted

+
+	pm_runtime_enable(&pdev->dev);
+
+	if (dev->res->codec_types & WAVE6_IS_DEC) {
+		ret = wave6_vpu_dec_register_device(dev);
+		if (ret) {
+			dev_err(&pdev->dev, "wave6_vpu_dec_register_device

fail: %d\n", ret);

quoted

+			goto err_temp_vbuf_free;
+		}
+	}
+	if (dev->res->codec_types & WAVE6_IS_ENC) {
+		ret = wave6_vpu_enc_register_device(dev);
+		if (ret) {
+			dev_err(&pdev->dev, "wave6_vpu_enc_register_device

fail: %d\n", ret);

quoted

+			goto err_dec_unreg;
+		}
+	}
+
+	if (dev->ctrl && wave6_vpu_ctrl_support_follower(dev->ctrl)) {
+		wave6_vpu_activate(dev);
+		ret = pm_runtime_resume_and_get(dev->dev);
+		if (ret)
+			goto err_enc_unreg;
+	}
+
+	dev_dbg(&pdev->dev, "Added wave6 driver with caps %s %s\n",
+		dev->res->codec_types & WAVE6_IS_ENC ? "'ENCODE'" : "",
+		dev->res->codec_types & WAVE6_IS_DEC ? "'DECODE'" : "");
+
+	return 0;
+
+err_enc_unreg:
+	if (dev->res->codec_types & WAVE6_IS_ENC)
+		wave6_vpu_enc_unregister_device(dev);
+err_dec_unreg:
+	if (dev->res->codec_types & WAVE6_IS_DEC)
+		wave6_vpu_dec_unregister_device(dev);
+err_temp_vbuf_free:
+	wave6_free_dma(&dev->temp_vbuf);
+err_kfifo_free:
+	kfifo_free(&dev->irq_status);
+err_m2m_dev_release:
+	wave6_vpu_release_m2m_dev(dev);
+err_v4l2_unregister:
+	v4l2_device_unregister(&dev->v4l2_dev);
+
+	return ret;
+}
+
+static void wave6_vpu_remove(struct platform_device *pdev)
+{
+	struct vpu_device *dev = dev_get_drvdata(&pdev->dev);
+
+	if (dev->ctrl && wave6_vpu_ctrl_support_follower(dev->ctrl)) {
+		if (!pm_runtime_suspended(&pdev->dev))
+			pm_runtime_put_sync(&pdev->dev);
+	}
+	pm_runtime_disable(&pdev->dev);
+
+	wave6_vpu_enc_unregister_device(dev);
+	wave6_vpu_dec_unregister_device(dev);
+	wave6_free_dma(&dev->temp_vbuf);
+	kfifo_free(&dev->irq_status);
+	wave6_vpu_release_m2m_dev(dev);
+	v4l2_device_unregister(&dev->v4l2_dev);
+}
+
+#ifdef CONFIG_PM
+static int wave6_vpu_runtime_suspend(struct device *dev)
+{
+	struct vpu_device *vpu_dev = dev_get_drvdata(dev);
+
+	if (!vpu_dev)
+		return -ENODEV;
+
+	dprintk(dev, "runtime suspend\n");

Drop

quoted

+	if (vpu_dev->ctrl && vpu_dev->active)
+		wave6_vpu_ctrl_put_sync(vpu_dev->ctrl, &vpu_dev->entity);
+	if (vpu_dev->num_clks)
+		clk_bulk_disable_unprepare(vpu_dev->num_clks, vpu_dev->clks);
+
+	return 0;
+}
+
+static int wave6_vpu_runtime_resume(struct device *dev)
+{
+	struct vpu_device *vpu_dev = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (!vpu_dev)
+		return -ENODEV;
+
+	dprintk(dev, "runtime resume\n");

Drop

quoted

+	if (vpu_dev->num_clks) {
+		ret = clk_bulk_prepare_enable(vpu_dev->num_clks, vpu_dev-
clks);
+		if (ret) {
+			dev_err(dev, "failed to enable clocks: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (vpu_dev->ctrl && vpu_dev->active) {
+		ret = wave6_vpu_ctrl_resume_and_get(vpu_dev->ctrl, &vpu_dev-
entity);
+		if (ret && vpu_dev->num_clks)
+			clk_bulk_disable_unprepare(vpu_dev->num_clks, vpu_dev-
clks);
+	} else {
+		wave6_vpu_check_state(vpu_dev);
+	}
+
+	return ret;
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int wave6_vpu_suspend(struct device *dev)
+{
+	int ret;
+
+	dprintk(dev, "suspend\n");

Drop. Don't re-implement existing tracing.

quoted

+	wave6_vpu_pause(dev, 0);
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		wave6_vpu_pause(dev, 1);
+
+	return ret;
+}
+
+static int wave6_vpu_resume(struct device *dev)
+{
+	int ret;
+
+	dprintk(dev, "resume\n");

Drop, useless.

Okay. I will remove the redundant debug message and dprintk in v2.

quoted

+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
+	wave6_vpu_pause(dev, 1);
+	return 0;
+}
+#endif
+static const struct dev_pm_ops wave6_vpu_pm_ops = {
+	SET_RUNTIME_PM_OPS(wave6_vpu_runtime_suspend,

wave6_vpu_runtime_resume, NULL)

quoted

+	SET_SYSTEM_SLEEP_PM_OPS(wave6_vpu_suspend, wave6_vpu_resume)
+};
+
+static const struct of_device_id wave6_dt_ids[] = {
+	{ .compatible = "nxp,imx95-wave633c", .data = &wave633c_data },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, wave6_dt_ids);
+
+static struct platform_driver wave6_vpu_driver = {
+	.driver = {
+		.name = VPU_PLATFORM_DEVICE_NAME,
+		.of_match_table = of_match_ptr(wave6_dt_ids),

Drop of_match_ptr, you have here warnings.

I will address this in v2.

Thanks.
Nas.

Best regards,
Krzysztof