[PATCH v4 1/3] phy: rockchip: phy-rockchip-inno-csidphy: fix rk1808 hsfreq table

[sashiko-bot at kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Valid MIPI D-PHY data rates <= 109 Mbps are incorrectly rejected due to an ambiguous sentinel check on `hsfreq`.
- [High] The driver requests exclusive hardware reset controls during probe but fails to ever deassert them.
- [High] The PHY provider is registered before PM runtime is enabled, leading to a race condition that can cause unexpected `-EACCES` failures on initialization.
--

--- Patch [1]: [PATCH v4 1/3] phy: rockchip: phy-rockchip-inno-csidphy: fix rk1808 hsfreq table ---
commit 4840808911c634155f4b288f67fa4f3c71720406
Author: Gerald Loacker <gerald.loacker at wolfvision.net>

phy: rockchip: phy-rockchip-inno-csidphy: fix rk1808 hsfreq table

The rk1808 hsfreq table capped at 2499 Mbps, preventing a data rate of
exactly 2500 Mbps. Extend the final entry to 2500 Mbps to support this
rate.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> index c79fb53d8ee5c..5281f8dea0ad3 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> @@ -170,7 +170,7 @@ static const struct hsfreq_range rk1808_mipidphy_hsfreq_ranges[] = {
>  	{ 299, 0x06}, { 399, 0x08}, { 499, 0x0b}, { 599, 0x0e},
>  	{ 699, 0x10}, { 799, 0x12}, { 999, 0x16}, {1199, 0x1e},
>  	{1399, 0x23}, {1599, 0x2d}, {1799, 0x32}, {1999, 0x37},
> -	{2199, 0x3c}, {2399, 0x41}, {2499, 0x46}
> +	{2199, 0x3c}, {2399, 0x41}, {2500, 0x46}
>  };
>  
>  static const struct hsfreq_range rk3326_mipidphy_hsfreq_ranges[] = {

[Severity: High]
This isn't a bug introduced by this patch, but since you are modifying the 
hsfreq table, I noticed a potential problem in how these entries are 
processed.

In rk3326_mipidphy_hsfreq_ranges, the lowest frequency range sets the 
configuration bit to 0x00:

static const struct hsfreq_range rk3326_mipidphy_hsfreq_ranges[] = {
	{ 109, 0x00}, { 149, 0x01}, { 199, 0x02}, { 249, 0x03},

Later in rockchip_inno_csidphy_configure(), we check for a match and fail 
if hsfreq is zero:

	for (i = 0; i < drv_data->num_hsfreq_ranges; i++) {
		if (drv_data->hsfreq_ranges[i].range_h >= data_rate_mbps) {
			hsfreq = drv_data->hsfreq_ranges[i].cfg_bit;
			break;
		}
	}
	if (!hsfreq)
		return -EINVAL;

Does this condition mistakenly reject valid low data rates (<= 109 Mbps)? 
When data_rate_mbps is 109 or lower, hsfreq becomes 0x00, which causes the 
check to fail and return -EINVAL. 

[Severity: High]
This is a pre-existing issue, but looking at the driver's initialization, 
it requests exclusive resets but doesn't seem to use them:

rockchip_inno_csidphy_probe() {
    ...
	ret = devm_reset_control_bulk_get_exclusive(dev, priv->resets_num,
						    priv->resets);
    ...
}

I don't see any calls to reset_control_bulk_deassert() or 
reset_control_bulk_assert() for priv->resets anywhere in 
rockchip_inno_csidphy_power_on(). Can this leave the PHY hardware stuck in 
reset if the bootloader didn't deassert it beforehand?

[Severity: High]
This is another pre-existing issue, but I noticed a potential race 
condition during initialization in rockchip_inno_csidphy_probe():

	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
	if (IS_ERR(phy_provider)) {
		dev_err(dev, "failed to register phy provider\n");
		return PTR_ERR(phy_provider);
	}

	pm_runtime_enable(dev);

Does this expose the PHY provider to consumers before PM runtime is fully 
enabled? 

If a consumer probes concurrently and calls phy_power_on(), the call routes 
to rockchip_inno_csidphy_power_on(). That function attempts to call 
pm_runtime_resume_and_get(). 

Wouldn't this return -EACCES since runtime PM isn't enabled yet, causing 
unexpected probe failures for the consumer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260725-feature-mipi-csi-dphy-4k60-v4-0-5b2c4626d31e@wolfvision.net?part=1