Re: [PATCH v2 1/3] media: verisilicon: Remove deprecated variant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] media: verisilicon: DT ABI breakage and unreachable deprecation warning
- [Low] media: verisilicon: unused codec ops and reset functions left as dead code
--

commit bd50c02dbeb6cf3a61e4c9cb1e9948505f4bb9e8
Author: Benjamin Gaignard <benjamin.gaignard@collabora.com>
Subject: media: verisilicon: Remove deprecated variant

This commit removes the deprecated "nxp,imx8mq-vpu" compatible string
and its associated variant data structure from the hantro driver to clean
up the code. The variant has been deprecated since v5.17-rc1.

> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
> index ad71c0402ef3b..70de47ddb023b 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -725,7 +725,6 @@ static const struct of_device_id of_hantro_match[] = {
>  #endif
>  #ifdef CONFIG_VIDEO_HANTRO_IMX8M
>  	{ .compatible = "nxp,imx8mm-vpu-g1", .data = &imx8mm_vpu_g1_variant, },
> -	{ .compatible = "nxp,imx8mq-vpu", .data = &imx8mq_vpu_variant, },

[Severity: Medium]
Does removing this compatible string completely break backward compatibility
for systems booting with older Device Trees containing only nxp,imx8mq-vpu?

Since the driver no longer matches this string, hantro_probe() is never
invoked for it. This renders the backwards compatibility check and dev_warn()
further down in hantro_probe() completely unreachable:

drivers/media/platform/verisilicon/hantro_drv.c:hantro_probe() {
    ...
	/*
	 * Support for nxp,imx8mq-vpu is kept for backwards compatibility
	 * but it's deprecated. Please update your DTS file to use
	 * nxp,imx8mq-vpu-g1 or nxp,imx8mq-vpu-g2 instead.
	 */
	if (of_device_is_compatible(pdev->dev.of_node, "nxp,imx8mq-vpu"))
		dev_warn(&pdev->dev, "%s compatible is deprecated\n",
			 match->compatible);
    ...
}

Is it intentional to prevent the driver from binding entirely instead of just
warning about the deprecation? If so, should the unreachable warning block
in hantro_probe() also be removed?

>  	{ .compatible = "nxp,imx8mq-vpu-g1", .data = &imx8mq_vpu_g1_variant },
>  	{ .compatible = "nxp,imx8mq-vpu-g2", .data = &imx8mq_vpu_g2_variant },
>  #endif

[ ... ]

> diff --git a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> index fa4224de4b996..3b63f626d33f2 100644
> --- a/drivers/media/platform/verisilicon/imx8m_vpu_hw.c
> +++ b/drivers/media/platform/verisilicon/imx8m_vpu_hw.c

[ ... ]

> @@ -319,30 +280,9 @@ static const struct hantro_irq imx8mq_g2_irqs[] = {
>  	{ "g2", hantro_g2_irq },
>  };
>  
> -static const char * const imx8mq_clk_names[] = { "g1", "g2", "bus" };
> -static const char * const imx8mq_reg_names[] = { "g1", "g2", "ctrl" };
>  static const char * const imx8mq_g1_clk_names[] = { "g1" };
>  static const char * const imx8mq_g2_clk_names[] = { "g2" };
>  
> -const struct hantro_variant imx8mq_vpu_variant = {
> -	.dec_fmts = imx8m_vpu_dec_fmts,
> -	.num_dec_fmts = ARRAY_SIZE(imx8m_vpu_dec_fmts),
> -	.postproc_fmts = imx8m_vpu_postproc_fmts,
> -	.num_postproc_fmts = ARRAY_SIZE(imx8m_vpu_postproc_fmts),
> -	.postproc_ops = &hantro_g1_postproc_ops,
> -	.codec = HANTRO_MPEG2_DECODER | HANTRO_VP8_DECODER |
> -		 HANTRO_H264_DECODER,
> -	.codec_ops = imx8mq_vpu_codec_ops,
> -	.init = imx8mq_vpu_hw_init,
> -	.runtime_resume = imx8mq_runtime_resume,
> -	.irqs = imx8mq_irqs,
> -	.num_irqs = ARRAY_SIZE(imx8mq_irqs),
> -	.clk_names = imx8mq_clk_names,
> -	.num_clocks = ARRAY_SIZE(imx8mq_clk_names),
> -	.reg_names = imx8mq_reg_names,
> -	.num_regs = ARRAY_SIZE(imx8mq_reg_names)
> -};
> -

[Severity: Low]
Does the removal of imx8mq_vpu_variant leave several other functions and
declarations orphaned as dead code?

By removing the variant, there are no remaining references to the static
array imx8mq_vpu_codec_ops. That array is the only caller of imx8m_vpu_g1_reset,
which in turn uniquely calls imx8m_soft_reset.

Should these unused declarations, along with the extern declaration for
imx8mq_vpu_variant in drivers/media/platform/verisilicon/hantro_hw.h,
also be removed to prevent compiler warnings about unused variables and
functions?

>  static const struct of_device_id imx8mq_vpu_shared_resources[] = {
>  	{ .compatible = "nxp,imx8mq-vpu-g1", },
>  	{ .compatible = "nxp,imx8mq-vpu-g2", },

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924110825.166238-1-benjamin.gaignard@collabora.com?part=1