Re: [PATCH v5 5/8] phy: rockchip-samsung-dcphy: Add a second PHY for the receiver

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

Hi Jason,

On 10/16/26 10:56, Jason Yang wrote:
> Give the receiver a struct phy of its own, with its own phy_ops and its
> own samsung_mipi_phy for the state the two do not share.
> 
> of_xlate() takes the direction from the second cell and falls back to
> the transmitter for a single-cell provider, so existing device trees
> keep working. It now rejects an unknown value in either cell, and a mix
> of D-PHY and C-PHY consumers, which the TRM does not support [1].
> 
> The receiver's callbacks return -EOPNOTSUPP until its bring-up is added
> in the next change.

Can you merge this patch with the subsequent one? IMHO it does not help
to first introduce some skeleton with -EOPNOTSUPP only to add support
for it in the very next patch...

Thanks and best regards,
Michael

> 
> [1] RK3588 TRM: section 22.1 (overview)
> 
> Assisted-by: LLM
> Signed-off-by: Jason Yang <jason98166@gmail.com>
> ---
>  drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c | 69 +++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> index e4293c8d2b29..316d4c4d49c1 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-dcphy.c
> @@ -296,6 +296,8 @@ struct samsung_mipi_dcphy {
>  	struct reset_control *apb_rst;
>  	struct reset_control *grf_apb_rst;
>  	struct samsung_mipi_phy tx;
> +	struct samsung_mipi_phy rx;
> +	/* PHY mode, PHY_TYPE_DPHY or PHY_TYPE_CPHY. */
>  	u8 type;
>  
>  	const struct samsung_mipi_dcphy_plat_data *pdata;
> @@ -1511,6 +1513,22 @@ static int samsung_mipi_dcphy_exit(struct phy *phy)
>  	return 0;
>  }
>  
> +static int samsung_mipi_dcphy_rx_configure(struct phy *phy,
> +					   union phy_configure_opts *opts)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int samsung_mipi_dcphy_rx_power_on(struct phy *phy)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static int samsung_mipi_dcphy_rx_power_off(struct phy *phy)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
>  static const struct phy_ops samsung_mipi_dcphy_tx_ops = {
>  	.configure = samsung_mipi_dcphy_tx_configure,
>  	.power_on  = samsung_mipi_dcphy_tx_power_on,
> @@ -1520,6 +1538,15 @@ static const struct phy_ops samsung_mipi_dcphy_tx_ops = {
>  	.owner	   = THIS_MODULE,
>  };
>  
> +static const struct phy_ops samsung_mipi_dcphy_rx_ops = {
> +	.configure = samsung_mipi_dcphy_rx_configure,
> +	.power_on  = samsung_mipi_dcphy_rx_power_on,
> +	.power_off = samsung_mipi_dcphy_rx_power_off,
> +	.init = samsung_mipi_dcphy_init,
> +	.exit = samsung_mipi_dcphy_exit,
> +	.owner	   = THIS_MODULE,
> +};
> +
>  static const struct regmap_config samsung_mipi_dcphy_regmap_config = {
>  	.name = "dcphy",
>  	.reg_bits = 32,
> @@ -1532,19 +1559,42 @@ static struct phy *samsung_mipi_dcphy_xlate(struct device *dev,
>  					    const struct of_phandle_args *args)
>  {
>  	struct samsung_mipi_dcphy *samsung = dev_get_drvdata(dev);
> +	/* Device trees without the second cell describe the transmitter. */
> +	u32 direction = PHY_DIRECTION_TX;
> +	u32 type;
>  
> -	if (args->args_count != 1) {
> +	if (args->args_count < 1 || args->args_count > 2) {
>  		dev_err(dev, "invalid number of arguments\n");
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	if (samsung->type != PHY_NONE && samsung->type != args->args[0])
> -		dev_warn(dev, "phy type select %d overwriting type %d\n",
> -			 args->args[0], samsung->type);
> +	type = args->args[0];
> +	if (type != PHY_TYPE_DPHY && type != PHY_TYPE_CPHY) {
> +		dev_err(dev, "invalid phy type %u\n", type);
> +		return ERR_PTR(-EINVAL);
> +	}
>  
> -	samsung->type = args->args[0];
> +	if (args->args_count == 2)
> +		direction = args->args[1];
>  
> -	return samsung->tx.phy;
> +	if (direction != PHY_DIRECTION_TX && direction != PHY_DIRECTION_RX) {
> +		dev_err(dev, "invalid direction %u\n", direction);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	/*
> +	 * The TRM (section 22.1) does not support the transmitter and the
> +	 * receiver running in different modes, so the mode belongs to the
> +	 * block.
> +	 */
> +	if (samsung->type != PHY_NONE && samsung->type != type) {
> +		dev_err(dev, "phy type %u conflicts with type %u already selected\n",
> +			type, samsung->type);
> +		return ERR_PTR(-EINVAL);
> +	}
> +	samsung->type = type;
> +
> +	return direction == PHY_DIRECTION_RX ? samsung->rx.phy : samsung->tx.phy;
>  }
>  
>  static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
> @@ -1620,6 +1670,13 @@ static int samsung_mipi_dcphy_probe(struct platform_device *pdev)
>  
>  	phy_set_drvdata(samsung->tx.phy, samsung);
>  
> +	samsung->rx.phy = devm_phy_create(dev, NULL, &samsung_mipi_dcphy_rx_ops);
> +	if (IS_ERR(samsung->rx.phy))
> +		return dev_err_probe(dev, PTR_ERR(samsung->rx.phy),
> +				     "Failed to create MIPI DC-PHY receiver\n");
> +
> +	phy_set_drvdata(samsung->rx.phy, samsung);
> +
>  	phy_provider = devm_of_phy_provider_register(dev, samsung_mipi_dcphy_xlate);
>  	if (IS_ERR(phy_provider))
>  		return dev_err_probe(dev, PTR_ERR(phy_provider),
>