Hi, Krzysztof.
>-----Original Message-----
>From: Krzysztof Kozlowski <krzk@xxxxxxxxxx>
>Sent: Tuesday, February 11, 2025 2:41 AM
>To: Nas Chung <nas.chung@xxxxxxxxxxxxxxx>; mchehab@xxxxxxxxxx;
>hverkuil@xxxxxxxxx; sebastian.fricke@xxxxxxxxxxxxx; robh@xxxxxxxxxx;
>krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx
>Cc: linux-media@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; linux-imx@xxxxxxx; linux-arm-
>kernel@xxxxxxxxxxxxxxxxxxx; jackson.lee <jackson.lee@xxxxxxxxxxxxxxx>;
>lafley.kim <lafley.kim@xxxxxxxxxxxxxxx>
>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
