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

[Nas Chung]

Hi, Krzysztof.

>-----Original Message-----
>From: Krzysztof Kozlowski <krzk at kernel.org>
>Sent: Friday, August 29, 2025 11:03 PM
>To: Nas Chung <nas.chung at chipsnmedia.com>; mchehab at kernel.org;
>hverkuil at xs4all.nl; robh at kernel.org; krzk+dt at kernel.org;
>conor+dt at kernel.org; shawnguo at kernel.org; s.hauer at pengutronix.de
>Cc: linux-media at vger.kernel.org; devicetree at vger.kernel.org; linux-
>kernel at vger.kernel.org; linux-imx at nxp.com; linux-arm-
>kernel at lists.infradead.org; jackson.lee <jackson.lee at chipsnmedia.com>;
>lafley.kim <lafley.kim at chipsnmedia.com>; Ming Qian <ming.qian at oss.nxp.com>
>Subject: Re: [PATCH v3 5/9] media: chips-media: wave6: Add Wave6 core
>driver
>
>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/submitt
>ing-patches.rst#L94

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

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

I'll remove it in v4.

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

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

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

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

Thanks,
Nas.

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