Re: [PATCH v6 07/28] media: rockchip: rga: use clk_bulk api

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Switch to `devm_clk_bulk_get_all()` silently drops validation for mandatory clocks and alters the hardware initialization sequence.
--

commit 234eed9f39e78b285ced7d7570cee6af431aad44
Author: Sven Püschel <s.pueschel@pengutronix.de>

media: rockchip: rga: use clk_bulk api

This commit replaces individual clock retrieval and enablement calls with
the clk_bulk API to reduce code duplication.

> diff --git a/drivers/media/platform/rockchip/rga/rga.c b/drivers/media/platform/rockchip/rga/rga.c
> --- a/drivers/media/platform/rockchip/rga/rga.c
> +++ b/drivers/media/platform/rockchip/rga/rga.c
[ ... ]
> @@ -771,23 +733,12 @@ static int rga_parse_dt(struct rockchip_rga *rga)
>  	udelay(1);
>  	reset_control_deassert(ahb_rst);
>  
> -	rga->sclk = devm_clk_get(rga->dev, "sclk");
[ ... ]
> -	rga->hclk = devm_clk_get(rga->dev, "hclk");
> -	if (IS_ERR(rga->hclk)) {
> -		dev_err(rga->dev, "failed to get hclk clock\n");
> -		return PTR_ERR(rga->hclk);
> +	ret = devm_clk_bulk_get_all(rga->dev, &rga->clks);
> +	if (ret < 0) {
> +		dev_err(rga->dev, "failed to get clocks\n");
> +		return ret;
>  	}
> +	rga->num_clks = ret;

Does this code introduce a regression when the device tree is missing
the clocks property?

Looking at devm_clk_bulk_get_all(), it returns the number of clocks found,
which is 0 if the clocks property is missing. Because the check is only for
ret < 0, a malformed device tree could cause the probe to succeed without
the required clocks. Accessing the hardware without all three clocks running
might result in an asynchronous external abort.

Also, does this change the required clock enablement sequence?

The previous code explicitly prepared and enabled the clocks in a fixed
order (sclk, aclk, hclk) and disabled them in a specific sequence. By using
devm_clk_bulk_get_all(), the order now depends strictly on the device tree
array index. If the hardware requires a specific sequence, this might cause
initialization issues.

Could devm_clk_bulk_get() with a predefined clk_bulk_data array be used
instead to explicitly define the required clock IDs? This would preserve both
validation and the enablement sequence.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-spu-rga3-v6-0-e547152eb9c9@pengutronix.de?part=7