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

[Nas Chung <nas.chung@chipsnmedia.com>]

Hi, Frank.

>-----Original Message-----
>From: Frank Li <Frank.li@oss.nxp.com>
>Sent: Friday, September 11, 2026 5:49 AM
>To: Nas Chung <nas.chung@chipsnmedia.com>
>Cc: mchehab@kernel.org; hverkuil@xs4all.nl; robh@kernel.org;
>krzk+dt@kernel.org; conor+dt@kernel.org; shawnguo@kernel.org;
>s.hauer@pengutronix.de; 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
><jackson.lee@chipsnmedia.com>; lafley.kim <lafley.kim@chipsnmedia.com>;
>marek.vasut@mailbox.org; Ming Qian <ming.qian@oss.nxp.com>
>Subject: Re: [PATCH v7 5/9] media: chips-media: wave6: Add Wave6 core
>driver
>
>On Fri, Sep 04, 2026 at 03:46:31PM +0900, Nas Chung wrote:
>> Add the core driver for the Chips&Media Wave6 video codec IP.
>>
>> The hardware contains one control register region and four interface
>> register regions for a shared video processing engine. This driver
>> handles the interface register regions, each with its own MMIO range and
>> interrupt, while relying on the control driver for firmware loading and
>> shared resource management.
>>

...

>> +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 = dev_get_platdata(&pdev->dev);
>> +	if (!res)
>> +		return -ENODEV;
>> +
>> +	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
>> +	if (ret < 0) {
>> +		dev_err(&pdev->dev, "failed to set DMA mask: %d\n", ret);
>> +		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;
>> +
>> +	spin_lock_init(&core->inst_lock);
>> +	INIT_LIST_HEAD(&core->instances);
>> +	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))
>
>why need check this?

I'll remove the condition in v8.

>
>> +		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)
>> +		return dev_err_probe(&pdev->dev, ret, "failed to get
>clocks\n");
>> +
>> +	core->num_clks = ret;
>> +
>> +	ret = kfifo_init(&core->irq_fifo, core->irq_buffer,
>> +			 sizeof(core->irq_buffer));
>> +	if (ret)
>> +		return ret;
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	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, "failed to request IRQ: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = v4l2_device_register(&pdev->dev, &core->v4l2_dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to register v4l2_dev: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = wave6_vpu_init_m2m_dev(core);
>> +	if (ret)
>> +		goto err_v4l2_unregister;
>> +
>> +	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, "failed to allocate temp_vbuf: %d\n",
>ret);
>> +		goto err_m2m_dev_release;
>> +	}
>> +
>> +	core->debugfs = debugfs_lookup(WAVE6_VPU_DEBUGFS_DIR, NULL);
>> +	if (!IS_ERR_OR_NULL(core->debugfs))
>> +		dput(core->debugfs);
>> +	else
>> +		core->debugfs = debugfs_create_dir(WAVE6_VPU_DEBUGFS_DIR,
>NULL);
>> +
>> +	pm_runtime_enable(&pdev->dev);
>
>devm_pm_runtime_enable()

OK.

>
>> +
>> +	if (core->res->codec_types & WAVE6_IS_DEC) {
>> +		ret = wave6_vpu_dec_register_device(core);
>> +		if (ret) {
>> +			dev_err(&pdev->dev,
>> +				"failed to register video_dev_dec: %d\n", ret);
>> +			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,
>> +				"failed to register video_dev_enc: %d\n", ret);
>> +			goto err_dec_unreg;
>> +		}
>> +	}
>> +
>> +	dev_dbg(&pdev->dev, "Added wave6 driver with caps %s %s\n",
>> +		core->res->codec_types & WAVE6_IS_ENC ? "'ENCODE'" : "",
>> +		core->res->codec_types & WAVE6_IS_DEC ? "'DECODE'" : "");
>> +
>> +	return 0;
>> +
>> +err_dec_unreg:
>> +	if (core->res->codec_types & WAVE6_IS_DEC)
>> +		wave6_vpu_dec_unregister_device(core);
>> +err_temp_vbuf_free:
>> +	pm_runtime_disable(&pdev->dev);
>> +	wave6_vdi_free_dma(&core->temp_vbuf);
>> +err_m2m_dev_release:
>> +	wave6_vpu_release_m2m_dev(core);
>> +err_v4l2_unregister:
>> +	v4l2_device_unregister(&core->v4l2_dev);
>
>can you try use devm_add_action_or_reset() to elimiate these error label?

OK, I'll address this in v8.

>
>> +
>> +	return ret;
>> +}
>> +
>> +static void wave6_vpu_core_remove(struct platform_device *pdev)
>> +{
>> +	struct vpu_core_device *core = dev_get_drvdata(&pdev->dev);
>> +
>> +	wave6_vpu_enc_unregister_device(core);
>> +	wave6_vpu_dec_unregister_device(core);
>> +	pm_runtime_disable(&pdev->dev);
>> +	wave6_vdi_free_dma(&core->temp_vbuf);
>> +	wave6_vpu_release_m2m_dev(core);
>> +	v4l2_device_unregister(&core->v4l2_dev);
>> +}
>> +
>> +static int wave6_vpu_core_runtime_suspend(struct device *dev)
>> +{
>> +	struct vpu_core_device *core = dev_get_drvdata(dev);
>> +
>> +	if (WARN_ON(!core))
>> +		return -ENODEV;
>> +
>> +	/*
>> +	 * Only call parent VPU put_vpu if the core has a parent and is
>active.
>> +	 * - core->vpu: prevent access in core without parent VPU.
>> +	 * - core->active: execute sleep only after m2m streaming is started.
>> +	 */
>> +	if (core->vpu && core->active)
>> +		core->vpu->put_vpu(core->vpu, core);
>> +
>> +	if (core->num_clks)
>
>Need check it. clk_bulk_disable_unprepare() is empty ops when num_clks is 0

I'll drop the guard in v8.

>
>> +		clk_bulk_disable_unprepare(core->num_clks, core->clks);
>> +
>> +	return 0;
>> +}
>> +
>> +static int wave6_vpu_core_runtime_resume(struct device *dev)
>> +{
>> +	struct vpu_core_device *core = dev_get_drvdata(dev);
>> +	bool boot_vpu;
>> +	int ret;
>> +
>> +	if (WARN_ON(!core))
>> +		return -ENODEV;
>> +
>> +	if (core->num_clks) {
>
>need checkit. clk_bulk_prepare_enable() is empty ops when num_clks is 0

Same here.

Thanks.
Nas.

>
>Frank