RE: [PATCH 4/8] media: chips-media: wave6: Add Wave6 codec driver

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

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 <jackson.lee@chipsnmedia.com>;
>lafley.kim <lafley.kim@chipsnmedia.com>
>Subject: Re: [PATCH 4/8] media: chips-media: wave6: Add Wave6 codec driver
>
>On 10/02/2025 10:07, Nas Chung wrote:
>> +
>> +	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);
>> +
>> +	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);
>> +			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);
>> +			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
>
>> +	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
>
>> +	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.
>
>> +	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.

>
>> +	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)
>> +	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