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

[Michael Riesch <michael.riesch@collabora.com>]

Hi Amin,

Thanks for your patches!

On 8/20/26 16:01, Amin Gattout wrote:
> The two RK3576 MIPI CSI-2 DPHYs can be supported using the existing
> phy-rockchip-inno-csidphy driver, the notable difference with the
> RK3588 variant being that each PHY instance only has an APB reset
> line.
> 
> Signed-off-by: Amin Gattout <amin.gattout@gmail.com>
> ---
>  drivers/phy/rockchip/phy-rockchip-inno-csidphy.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> index c79fb53d8ee5..24aefdcd6361 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-csidphy.c
> @@ -30,6 +30,7 @@
>  #define RK3568_GRF_VI_CON0		0x0340
>  #define RK3568_GRF_VI_CON1		0x0344
>  
> +#define RK3576_CSIDPHY_GRF_CON0		0x0000

This, ...

>  #define RK3588_CSIDPHY_GRF_CON0		0x0000
>  
>  /* PHY */
> @@ -119,6 +120,12 @@ static const struct dphy_reg rk3568_grf_dphy_regs[] = {
>  	[GRF_DPHY_CSIPHY_CLKLANE_EN] = PHY_REG(RK3568_GRF_VI_CON0, 1, 8),
>  };
>  
> +static const struct dphy_reg rk3576_grf_dphy_regs[] = {
> +	[GRF_DPHY_CSIPHY_FORCERXMODE] = PHY_REG(RK3576_CSIDPHY_GRF_CON0, 4, 0),
> +	[GRF_DPHY_CSIPHY_DATALANE_EN] = PHY_REG(RK3576_CSIDPHY_GRF_CON0, 4, 4),
> +	[GRF_DPHY_CSIPHY_CLKLANE_EN] = PHY_REG(RK3576_CSIDPHY_GRF_CON0, 1, 8),
> +};
> +

and this look pretty identical to RK3588, can't we just drop these two
and ...

>  static const struct dphy_reg rk3588_grf_dphy_regs[] = {
>  	[GRF_DPHY_CSIPHY_FORCERXMODE] = PHY_REG(RK3588_CSIDPHY_GRF_CON0, 4, 0),
>  	[GRF_DPHY_CSIPHY_DATALANE_EN] = PHY_REG(RK3588_CSIDPHY_GRF_CON0, 4, 4),
> @@ -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,

... use rk3588_grf_dphy_regs here?

> +	.resets = rk3368_reset_names,

So that the reset names are the only difference to RK3588 (which seems
to be the case as per commit message)?

> +	.resets_num = ARRAY_SIZE(rk3368_reset_names),
> +};
> +
>  static const struct dphy_drv_data rk3588_mipidphy_drv_data = {
>  	.pwrctl_offset = -1,
>  	.ths_settle_offset = RK3568_CSIDPHY_CLK_WR_THS_SETTLE,
> @@ -435,6 +453,10 @@ static const struct of_device_id rockchip_inno_csidphy_match_id[] = {
>  		.compatible = "rockchip,rk3568-csi-dphy",
>  		.data = &rk3568_mipidphy_drv_data,
>  	},
> +	{
> +		.compatible = "rockchip,rk3576-csi-dphy",
> +		.data = &rk3576_mipidphy_drv_data,
> +	},
>  	{
>  		.compatible = "rockchip,rk3588-csi-dphy",
>  		.data = &rk3588_mipidphy_drv_data,
> 

Apart from that LGTM!

Best regards,
Michael