Re: [PATCH 3/4] phy: rockchip: phy-rockchip-inno-csidphy: add support for rk3576 variant

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

Pre-existing issues:
- [High] The driver acquires exclusive control over the PHY resets but never asserts or deasserts them during its lifecycle.
--

--- Patch [3]: [PATCH 3/4] phy: rockchip: phy-rockchip-inno-csidphy: add support for rk3576 variant ---
commit f333cc625c5d500108f905c6acdcb428ada8e7d6
Author: Amin Gattout <[email protected]>

phy: rockchip: phy-rockchip-inno-csidphy: add support for rk3576 variant

This commit adds support for the RK3576 MIPI CSI-2 DPHY to the existing
rockchip-inno-csidphy driver. It introduces driver data and register
definitions for the new variant.

> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> index c79fb53d8ee5c..24aefdcd63617 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c

[ ... ]

> @@ -403,6 +410,17 @@ static const struct dphy_drv_data rk3568_mipidphy_drv_data = {
>  	.resets_num = ARRAY_SIZE(rk3368_reset_names),
>  };
>  
> +static const struct dphy_drv_data rk3576_mipidphy_drv_data = {
> +	.pwrctl_offset = -1,
> +	.ths_settle_offset = RK3568_CSIDPHY_CLK_WR_THS_SETTLE,
> +	.calib_offset = RK3568_CSIDPHY_CLK_CALIB_EN,
> +	.hsfreq_ranges = rk1808_mipidphy_hsfreq_ranges,
> +	.num_hsfreq_ranges = ARRAY_SIZE(rk1808_mipidphy_hsfreq_ranges),
> +	.grf_regs = rk3576_grf_dphy_regs,
> +	.resets = rk3368_reset_names,
> +	.resets_num = ARRAY_SIZE(rk3368_reset_names),

[Severity: High]
This is a pre-existing issue, but does the driver actually use these resets
after acquiring them?

Looking at rockchip_inno_csidphy_probe(), the driver acquires exclusive
control of the resets:

drivers/phy/rockchip/phy-rockchip-inno-csidphy.c:rockchip_inno_csidphy_probe()
	ret = devm_reset_control_bulk_get_exclusive(dev, priv->resets_num,
						    priv->resets);
	if (ret) {
		dev_err(dev, "failed to get system reset control\n");
		return ret;
	}

However, the driver doesn't appear to ever call reset_control_bulk_deassert() 
or reset_control_bulk_assert() during its lifecycle (such as in power_on or
init paths).

Because the driver claims exclusive control, no other subsystem can manage
these resets. Will the hardware remain permanently in reset and silently fail
to operate if it is left asserted by the bootloader or a previous stage?

> +};
> +
>  static const struct dphy_drv_data rk3588_mipidphy_drv_data = {
>  	.pwrctl_offset = -1,
>  	.ths_settle_offset = RK3568_CSIDPHY_CLK_WR_THS_SETTLE,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy