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

[Krzysztof Kozlowski <krzk@kernel.org>]

On 29/08/2025 10:46, Nas Chung wrote:
> 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/submitting-patches.rst#L94

> 
> The core region provides the encoding and decoding capabilities of
> the VPU and depends on the control region for firmware 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 = device_get_match_data(&pdev->dev);
> +	if (!res) {
> +		dev_err(&pdev->dev, "missing resource\n");

This is almost impossible condition. Not worth printing errors.

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

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

> +		ret = -ENXIO;

Don't override error codes.

> +		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);
> +		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);
> +
> +	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);
> +			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);
> +			goto err_dec_unreg;
> +		}
> +	}
Best regards,
Krzysztof