RE: [PATCH v3 5/9] media: chips-media: wave6: Add Wave6 core driver

Hi, Krzysztof.

-----Original Message-----
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Friday, August 29, 2025 11:03 PM
To: Nas Chung <nas.chung@chipsnmedia.com>; mchehab@kernel.org;
hverkuil@xs4all.nl; robh@kernel.org; krzk+dt@kernel.org;
conor+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de
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]; Ming Qian [off-list ref]
Subject: Re: [PATCH v3 5/9] media: chips-media: wave6: Add Wave6 core
driver

On 29/08/2025 10:46, Nas Chung wrote:

quoted

This adds the core driver for the Chips&Media Wave6 video codec IP.

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v6.16/source/Documentation/process/submitt
ing-patches.rst#L94

Understood, I'll fix the commit message in v4.
Thanks for sharing it.

quoted

The core region provides the encoding and decoding capabilities of
the VPU and depends on the control region for firmware and shared
resource management.

...

quoted

+
+static int wave6_vpu_core_probe(struct platform_device *pdev)
+{
+	struct vpu_core_device *core;
+	const struct wave6_vpu_core_resource *res;
+	int ret;
+	int irq;
+
+	res = device_get_match_data(&pdev->dev);
+	if (!res) {
+		dev_err(&pdev->dev, "missing resource\n");

This is almost impossible condition. Not worth printing errors.

I'll remove it in v4.

quoted

+		return -EINVAL;
+	}
+
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (ret < 0) {
+		dev_err(&pdev->dev, "dma_set_mask_and_coherent failed: %d\n",

ret);

quoted

+		return ret;
+	}
+
+	core = devm_kzalloc(&pdev->dev, sizeof(*core), GFP_KERNEL);
+	if (!core)
+		return -ENOMEM;
+
+	ret = devm_mutex_init(&pdev->dev, &core->dev_lock);
+	if (ret)
+		return ret;
+
+	ret = devm_mutex_init(&pdev->dev, &core->hw_lock);
+	if (ret)
+		return ret;
+
+	init_completion(&core->irq_done);
+	dev_set_drvdata(&pdev->dev, core);
+	core->dev = &pdev->dev;
+	core->res = res;
+
+	if (pdev->dev.parent->driver && pdev->dev.parent->driver->name &&
+	    !strcmp(pdev->dev.parent->driver->name,

WAVE6_VPU_PLATFORM_DRIVER_NAME))

quoted

+		core->vpu = dev_get_drvdata(pdev->dev.parent);
+
+	core->reg_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(core->reg_base))
+		return PTR_ERR(core->reg_base);
+
+	ret = devm_clk_bulk_get_all(&pdev->dev, &core->clks);
+	if (ret < 0) {
+		dev_warn(&pdev->dev, "unable to get clocks: %d\n", ret);

You should handle deferred probe instead.

Okay, I'll address this in v4 using dev_err_probe().

quoted

+		ret = 0;
+	}
+	core->num_clks = ret;
+
+	ret = v4l2_device_register(&pdev->dev, &core->v4l2_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "v4l2_device_register fail: %d\n", ret);
+		return ret;
+	}
+
+	ret = wave6_vpu_init_m2m_dev(core);
+	if (ret)
+		goto err_v4l2_unregister;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "failed to get irq resource\n");

Syntax is: dev_err_probe

quoted

+		ret = -ENXIO;

Don't override error codes.

Understood, I'll clean up this part in v4 based on your feedback.

Thanks,
Nas.

quoted

+		goto err_m2m_dev_release;
+	}
+
+	ret = kfifo_alloc(&core->irq_status, 16 * sizeof(int), GFP_KERNEL);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to allocate fifo\n");
+		goto err_m2m_dev_release;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq,
+					wave6_vpu_core_irq,
+					wave6_vpu_core_irq_thread,
+					0, "vpu_irq", core);
+	if (ret) {
+		dev_err(&pdev->dev, "fail to register interrupt

handler: %d\n", ret);

quoted

+		goto err_kfifo_free;
+	}
+
+	core->temp_vbuf.size = ALIGN(W6_TEMPBUF_SIZE, 4096);
+	ret = wave6_vdi_alloc_dma(core->dev, &core->temp_vbuf);
+	if (ret) {
+		dev_err(&pdev->dev, "alloc temp of size %zu failed\n",
+			core->temp_vbuf.size);
+		goto err_kfifo_free;
+	}
+
+	core->debugfs = debugfs_lookup(WAVE6_VPU_DEBUGFS_DIR, NULL);
+	if (IS_ERR_OR_NULL(core->debugfs))
+		core->debugfs = debugfs_create_dir(WAVE6_VPU_DEBUGFS_DIR,

NULL);

quoted

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

fail: %d\n", ret);

quoted

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

fail: %d\n", ret);

quoted

+			goto err_dec_unreg;
+		}
+	}

Best regards,
Krzysztof