Re: [PATCH DNI v2 11/11] media: rcar-fcp: Check device revision at probe time

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] pm: resource leak on probe error due to async pm_runtime_put()
- [Low] macro: malformed macro RCAR_FCP_REG_PICINFO1_STRIDE_DIV16 missing parameter list
- [Low] type: compiler warning from direct pointer-to-enum cast on 64-bit architectures
--

commit 4a6c9e446d9f471665024b004ad3e5842c693332
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

media: rcar-fcp: Check device revision at probe time

Verify that the device revision register reports a valid value,
otherwise reject the device.

> diff --git a/drivers/media/platform/renesas/rcar-fcp.c b/drivers/media/platform/renesas/rcar-fcp.c
> index 76d4c250b3081..5f2c483f71f42 100644
> --- a/drivers/media/platform/renesas/rcar-fcp.c
> +++ b/drivers/media/platform/renesas/rcar-fcp.c
[ ... ]
> +#define RCAR_FCP_REG_TL_CTRL			0x0070
> +#define RCAR_FCP_REG_TL_CTRL_TLEN		BIT(31)
> +#define RCAR_FCP_REG_TL_CTRL_VPOS_C(n)		((n) << 16)
> +#define RCAR_FCP_REG_TL_CTRL_VPOS_Y(n)		((n) << 0)
> +
> +#define RCAR_FCP_REG_PICINFO1			0x00c4
> +#define RCAR_FCP_REG_PICINFO1_STRIDE_DIV16	((n) << 0)

[Severity: Low]
Is this macro missing a parameter list? It looks like the missing (n) after
the macro name will prevent it from taking arguments, treating ((n) << 0)
as the literal expansion.

[ ... ]
>  static int rcar_fcp_probe(struct platform_device *pdev)
>  {
>  	struct rcar_fcp_device *fcp;
> +	int ret;
>  
>  	fcp = devm_kzalloc(&pdev->dev, sizeof(*fcp), GFP_KERNEL);
>  	if (fcp == NULL)
>  		return -ENOMEM;
>  
>  	fcp->dev = &pdev->dev;
> +	fcp->type = (enum rcar_fcp_type)device_get_match_data(&pdev->dev);

[Severity: Low]
Can this direct pointer-to-enum cast cause a compiler warning on 64-bit
architectures?

To avoid precision loss warnings on 64-bit platforms like ARM64, should the
pointer be cast to an integer of the same size, such as unsigned long or
uintptr_t, before being cast to the enum type?

>  	platform_set_drvdata(pdev, fcp);
[ ... ]
>  	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_resume_and_get(&pdev->dev);
> +	if (ret < 0)
> +		goto error_pm_disable;
> +
> +	ret = rcar_fcp_setup(fcp);
> +	if (ret < 0)
> +		goto error_pm_put;
> +
> +	pm_runtime_put(&pdev->dev);
>  
>  	mutex_lock(&fcp_lock);
>  	list_add_tail(&fcp->list, &fcp_devices);
>  	mutex_unlock(&fcp_lock);
>  
>  	return 0;
> +
> +error_pm_put:
> +	pm_runtime_put(&pdev->dev);
> +error_pm_disable:
> +	pm_runtime_disable(&pdev->dev);
> +	return ret;

[Severity: Medium]
Does calling pm_runtime_put() immediately before pm_runtime_disable() leak
the device power state?

Since pm_runtime_put() queues an asynchronous suspend, the subsequent
pm_runtime_disable() can cancel it, leaving the hardware active after a probe
failure.

Should this use pm_runtime_put_sync() instead to ensure the device powers
down correctly before being disabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919-epaul-v7-3-rc1-x5h-vsp-fcp-v2-0-dbe9236bfd22@ideasonboard.com?part=11