Re: [PATCH v5 4/8] phy: rockchip-samsung-dcphy: Name the transmitter helpers and ops

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

Hi Jason,

This looks good to me.

On 10/16/26 10:56, Jason Yang wrote:
> The block is a combo PHY: it exposes a transmitter and a receiver, and a
> later change registers one struct phy for each. Prepare for that. Give
> every helper that programs the master lanes a tx_ prefix, and move the
> phy pointer and the lane count, which only the transmitter uses, into a
> struct samsung_mipi_phy that the receiver can have one of too.
> 
> Name the phy_ops and its callbacks after the transmitter as well, so
> that the receiver can bring its own rather than share these.
> 
> The BIAS block and the PLL keep their names: the TRM puts both in the
> part common to the whole block [1], so neither belongs to one PHY even
> though only the transmitter drives the PLL.
> 
> No functional change intended.
> 
> [1] RK3588 TRM: sections 22.3.1 (bias generator), 22.3.2 (PLL) and
>     22.4.1 (address map)
> 
> Assisted-by: LLM
> Signed-off-by: Jason Yang <jason98166@gmail.com>

Reviewed-by: Michael Riesch <michael.riesch@collabora.com>

Best regards,
Michael

> ---
>  drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 67 ++++++++++++-----------
>  1 file changed, 36 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> index 1471a1563d2a..e4293c8d2b29 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> @@ -280,6 +280,11 @@ struct samsung_mipi_dcphy_plat_data {
>  	u32 dphy_tx_max_lane_kbps;
>  };
>  
> +struct samsung_mipi_phy {
> +	struct phy *phy;
> +	unsigned int lanes;
> +};
> +
>  struct samsung_mipi_dcphy {
>  	struct device *dev;
>  	struct clk *ref_clk;
> @@ -290,8 +295,7 @@ struct samsung_mipi_dcphy {
>  	struct reset_control *s_phy_rst;
>  	struct reset_control *apb_rst;
>  	struct reset_control *grf_apb_rst;
> -	unsigned int lanes;
> -	struct phy *phy;
> +	struct samsung_mipi_phy tx;
>  	u8 type;
>  
>  	const struct samsung_mipi_dcphy_plat_data *pdata;
> @@ -982,13 +986,13 @@ static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *sams
>  						 REG_400M_400MV | REG_645M_645MV);
>  }
>  
> -static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
> +static void samsung_mipi_dphy_tx_lane_enable(struct samsung_mipi_dcphy *samsung)
>  {
>  	regmap_write(samsung->regmap, DPHY_MC_GNR_CON1, T_PHY_READY(0x2000));
>  	regmap_update_bits(samsung->regmap, DPHY_MC_GNR_CON0,
>  			   PHY_ENABLE, PHY_ENABLE);
>  
> -	switch (samsung->lanes) {
> +	switch (samsung->tx.lanes) {
>  	case 4:
>  		regmap_write(samsung->regmap, DPHY_MD3_GNR_CON1,
>  			     T_PHY_READY(0x2000));
> @@ -1017,9 +1021,9 @@ static void samsung_mipi_dphy_lane_enable(struct samsung_mipi_dcphy *samsung)
>  	}
>  }
>  
> -static void samsung_mipi_dphy_lane_disable(struct samsung_mipi_dcphy *samsung)
> +static void samsung_mipi_dphy_tx_lane_disable(struct samsung_mipi_dcphy *samsung)
>  {
> -	switch (samsung->lanes) {
> +	switch (samsung->tx.lanes) {
>  	case 4:
>  		regmap_update_bits(samsung->regmap, DPHY_MD3_GNR_CON0,
>  				   PHY_ENABLE, 0);
> @@ -1094,7 +1098,7 @@ static void samsung_mipi_dcphy_pll_disable(struct samsung_mipi_dcphy *samsung)
>  }
>  
>  static const struct samsung_mipi_dphy_timing *
> -samsung_mipi_dphy_get_timing(struct samsung_mipi_dcphy *samsung)
> +samsung_mipi_dphy_tx_get_timing(struct samsung_mipi_dcphy *samsung)
>  {
>  	const struct samsung_mipi_dphy_timing *timings;
>  	unsigned int num_timings;
> @@ -1201,13 +1205,13 @@ samsung_mipi_dcphy_pll_round_rate(struct samsung_mipi_dcphy *samsung,
>  }
>  
>  static void
> -samsung_mipi_dphy_clk_lane_timing_init(struct samsung_mipi_dcphy *samsung)
> +samsung_mipi_dphy_tx_clk_lane_timing_init(struct samsung_mipi_dcphy *samsung)
>  {
>  	const struct samsung_mipi_dphy_timing *timing;
>  	unsigned int lane_hs_rate = div64_ul(samsung->pll.rate, USEC_PER_SEC);
>  	u32 val, res_up, res_down;
>  
> -	timing = samsung_mipi_dphy_get_timing(samsung);
> +	timing = samsung_mipi_dphy_tx_get_timing(samsung);
>  	regmap_write(samsung->regmap, DPHY_MC_GNR_CON0, 0xf000);
>  
>  	/*
> @@ -1256,13 +1260,13 @@ samsung_mipi_dphy_clk_lane_timing_init(struct samsung_mipi_dcphy *samsung)
>  }
>  
>  static void
> -samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
> +samsung_mipi_dphy_tx_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
>  {
>  	const struct samsung_mipi_dphy_timing *timing;
>  	unsigned int lane_hs_rate = div64_ul(samsung->pll.rate, USEC_PER_SEC);
>  	u32 val, res_up, res_down;
>  
> -	timing = samsung_mipi_dphy_get_timing(samsung);
> +	timing = samsung_mipi_dphy_tx_get_timing(samsung);
>  
>  	/*
>  	 * The Drive-Strength / Voltage-Amplitude is adjusted by adjusting the
> @@ -1325,7 +1329,7 @@ samsung_mipi_dphy_data_lane_timing_init(struct samsung_mipi_dcphy *samsung)
>  	regmap_write(samsung->regmap, DPHY_MD3_TIME_CON4, 0x1f4);
>  }
>  
> -static int samsung_mipi_dphy_power_on(struct samsung_mipi_dcphy *samsung)
> +static int samsung_mipi_dphy_tx_power_on(struct samsung_mipi_dcphy *samsung)
>  {
>  	int ret;
>  
> @@ -1339,13 +1343,13 @@ static int samsung_mipi_dphy_power_on(struct samsung_mipi_dcphy *samsung)
>  			   I_MUX_SEL_MASK, I_MUX_400MV);
>  
>  	samsung_mipi_dcphy_pll_configure(samsung);
> -	samsung_mipi_dphy_clk_lane_timing_init(samsung);
> -	samsung_mipi_dphy_data_lane_timing_init(samsung);
> +	samsung_mipi_dphy_tx_clk_lane_timing_init(samsung);
> +	samsung_mipi_dphy_tx_data_lane_timing_init(samsung);
>  	ret = samsung_mipi_dcphy_pll_enable(samsung);
>  	if (ret < 0)
>  		return ret;
>  
> -	samsung_mipi_dphy_lane_enable(samsung);
> +	samsung_mipi_dphy_tx_lane_enable(samsung);
>  
>  	reset_control_deassert(samsung->m_phy_rst);
>  
> @@ -1357,13 +1361,13 @@ static int samsung_mipi_dphy_power_on(struct samsung_mipi_dcphy *samsung)
>  	return 0;
>  }
>  
> -static int samsung_mipi_dcphy_power_on(struct phy *phy)
> +static int samsung_mipi_dcphy_tx_power_on(struct phy *phy)
>  {
>  	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
>  
>  	switch (samsung->type) {
>  	case PHY_TYPE_DPHY:
> -		return samsung_mipi_dphy_power_on(samsung);
> +		return samsung_mipi_dphy_tx_power_on(samsung);
>  	default:
>  		/* CPHY part to be implemented later */
>  		return -EOPNOTSUPP;
> @@ -1372,13 +1376,13 @@ static int samsung_mipi_dcphy_power_on(struct phy *phy)
>  	return 0;
>  }
>  
> -static int samsung_mipi_dcphy_power_off(struct phy *phy)
> +static int samsung_mipi_dcphy_tx_power_off(struct phy *phy)
>  {
>  	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
>  
>  	switch (samsung->type) {
>  	case PHY_TYPE_DPHY:
> -		samsung_mipi_dphy_lane_disable(samsung);
> +		samsung_mipi_dphy_tx_lane_disable(samsung);
>  		break;
>  	default:
>  		/* CPHY part to be implemented later */
> @@ -1477,13 +1481,13 @@ samsung_mipi_dcphy_pll_calc_rate(struct samsung_mipi_dcphy *samsung,
>  	}
>  }
>  
> -static int samsung_mipi_dcphy_configure(struct phy *phy,
> -					union phy_configure_opts *opts)
> +static int samsung_mipi_dcphy_tx_configure(struct phy *phy,
> +					   union phy_configure_opts *opts)
>  {
>  	struct samsung_mipi_dcphy *samsung = phy_get_drvdata(phy);
>  	unsigned long long target_rate = opts->mipi_dphy.hs_clk_rate;
>  
> -	samsung->lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
> +	samsung->tx.lanes = opts->mipi_dphy.lanes > 4 ? 4 : opts->mipi_dphy.lanes;
>  
>  	samsung_mipi_dcphy_pll_calc_rate(samsung, target_rate);
>  	opts->mipi_dphy.hs_clk_rate = samsung->pll.rate;
> @@ -1507,10 +1511,10 @@ static int samsung_mipi_dcphy_exit(struct phy *phy)
>  	return 0;
>  }
>  
> -static const struct phy_ops samsung_mipi_dcphy_ops = {
> -	.configure = samsung_mipi_dcphy_configure,
> -	.power_on  = samsung_mipi_dcphy_power_on,
> -	.power_off = samsung_mipi_dcphy_power_off,
> +static const struct phy_ops samsung_mipi_dcphy_tx_ops = {
> +	.configure = samsung_mipi_dcphy_tx_configure,
> +	.power_on  = samsung_mipi_dcphy_tx_power_on,
> +	.power_off = samsung_mipi_dcphy_tx_power_off,
>  	.init = samsung_mipi_dcphy_init,
>  	.exit = samsung_mipi_dcphy_exit,
>  	.owner	   = THIS_MODULE,
> @@ -1540,7 +1544,7 @@ static struct phy *samsung_mipi_dcphy_xlate(struct device *dev,
>  
>  	samsung->type = args->args[0];
>  
> -	return samsung->phy;
> +	return samsung->tx.phy;
>  }
>  
>  static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
> @@ -1609,11 +1613,12 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
>  
> -	samsung->phy = devm_phy_create(dev, NULL, &samsung_mipi_dcphy_ops);
> -	if (IS_ERR(samsung->phy))
> -		return dev_err_probe(dev, PTR_ERR(samsung->phy), "Failed to create MIPI DC-PHY\n");
> +	samsung->tx.phy = devm_phy_create(dev, NULL, &samsung_mipi_dcphy_tx_ops);
> +	if (IS_ERR(samsung->tx.phy))
> +		return dev_err_probe(dev, PTR_ERR(samsung->tx.phy),
> +				     "Failed to create MIPI DC-PHY transmitter\n");
>  
> -	phy_set_drvdata(samsung->phy, samsung);
> +	phy_set_drvdata(samsung->tx.phy, samsung);
>  
>  	phy_provider = devm_of_phy_provider_register(dev, samsung_mipi_dcphy_xlate);
>  	if (IS_ERR(phy_provider))
>