Re: [PATCH v4 07/17] phy: phy-rockchip-samsung-hdptx: Add support for eDP mode

[Damon Ding]

Hi Dmitry,

On 2024/12/30 20:45, Dmitry Baryshkov wrote:
> On Thu, Dec 26, 2024 at 02:33:03PM +0800, Damon Ding wrote:
> > Add basic support for RBR/HBR/HBR2 link rates, and the voltage swing and
> > pre-emphasis configurations of each link rate have been verified according
> > to the eDP 1.3 requirements.
>
> Well... Please describe what's happening here. That the HDMI PHY on your
> platform also provides support for DP / eDP. Please document any design
> decisions that you had to make.

Yes, I will add more relevant descriptions in the next version.

> > Signed-off-by: Damon Ding <damon.ding@xxxxxxxxxxxxxx>
> >
> > ---
> >
> > Changes in v2:
> > - Add the module author
> >
> > Changes in v3:
> > - Split this patch into two, one for correction and the other for
> >    extension
> >
> > Changes in v4:
> > - Add link_rate and lanes parameters in struct rk_hdptx_phy to store the
> >    phy_configure() result for &phy_configure_opts.dp.link_rate and
> >    &phy_configure_opts.dp.lanes
> > ---
> >   .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 896 +++++++++++++++++-
> >   1 file changed, 889 insertions(+), 7 deletions(-)
> >
> > @@ -933,9 +1484,339 @@ static int rk_hdptx_phy_power_off(struct phy *phy)
> >   	return rk_hdptx_phy_consumer_put(hdptx, false);
> >   }
> >   
> > +static int rk_hdptx_phy_set_mode(struct phy *phy, enum phy_mode mode,
> > +				 int submode)
> > +{
> > +	return 0;
> > +}
>
> No need for the stub, please drop it. The host controller driver can
> still call phy_set_mode() / _ext(), the call will return 0.

Without the &phy_ops.set_mode(), the phy driver can not get phy_mode to 
distinguish between HDMI and DP mode via the phy_get_mode(), even if the 
host driver calls phy_set_mode() / _ext(). Additionally, the previous 
discussion [0] also mentioned future considerations for dynamic 
switching. Indeed, I should add a related comment before the 'return 0;' 
to enhance understandability.

> > +
> > +static int rk_hdptx_phy_verify_config(struct rk_hdptx_phy *hdptx,
> > +				      struct phy_configure_opts_dp *dp)
> > +{
> > +	int i;
> > +
> > +	if (dp->set_rate) {
> > +		switch (dp->link_rate) {
> > +		case 1620:
> > +		case 2700:
> > +		case 5400:
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (dp->set_lanes) {
> > +		switch (dp->lanes) {
> > +		case 0:
>
> Is it really a valid config to have 0 lanes?

The case of 0 lanes is indeed unnecessary.

> > +		case 1:
> > +		case 2:
> > +		case 4:
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	if (dp->set_voltages) {
> > +		for (i = 0; i < hdptx->lanes; i++) {
> > +			if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> > +				return -EINVAL;
> > +
> > +			if (dp->voltage[i] + dp->pre[i] > 3)
> > +				return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
>
> [..]
>
> > +
> > +static int rk_hdptx_phy_configure(struct phy *phy, union phy_configure_opts *opts)
> > +{
> > +	struct rk_hdptx_phy *hdptx = phy_get_drvdata(phy);
> > +	enum phy_mode mode = phy_get_mode(phy);
> > +	int ret;
> > +
> > +	if (mode != PHY_MODE_DP)
> > +		return -EINVAL;
>
> I'd say, return 0;

Yes, it is really not an error.

> > +
> > +	ret = rk_hdptx_phy_verify_config(hdptx, &opts->dp);
> > +	if (ret) {
> > +		dev_err(hdptx->dev, "invalid params for phy configure\n");
> > +		return ret;
> > +	}
> > +
> > +	if (opts->dp.set_rate) {
> > +		ret = rk_hdptx_phy_set_rate(hdptx, &opts->dp);
> > +		if (ret) {
> > +			dev_err(hdptx->dev, "failed to set rate: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (opts->dp.set_lanes) {
> > +		ret = rk_hdptx_phy_set_lanes(hdptx, &opts->dp);
> > +		if (ret) {
> > +			dev_err(hdptx->dev, "failed to set lanes: %d\n", ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (opts->dp.set_voltages) {
> > +		ret = rk_hdptx_phy_set_voltages(hdptx, &opts->dp);
> > +		if (ret) {
> > +			dev_err(hdptx->dev, "failed to set voltages: %d\n",
> > +				ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct phy_ops rk_hdptx_phy_ops = {
> >   	.power_on  = rk_hdptx_phy_power_on,
> >   	.power_off = rk_hdptx_phy_power_off,
> > +	.set_mode  = rk_hdptx_phy_set_mode,
> > +	.configure = rk_hdptx_phy_configure,
> >   	.owner	   = THIS_MODULE,
> >   };
> >   
> > @@ -1149,5 +2030,6 @@ module_platform_driver(rk_hdptx_phy_driver);
> >   
> >   MODULE_AUTHOR("Algea Cao <algea.cao@xxxxxxxxxxxxxx>");
> >   MODULE_AUTHOR("Cristian Ciocaltea <cristian.ciocaltea@xxxxxxxxxxxxx>");
> > +MODULE_AUTHOR("Damon Ding <damon.ding@xxxxxxxxxxxxxx>");
> >   MODULE_DESCRIPTION("Samsung HDMI/eDP Transmitter Combo PHY Driver");
> >   MODULE_LICENSE("GPL");
> > --
> > 2.34.1

Best regards
Damon

[0]https://patchwork.kernel.org/project/linux-rockchip/patch/20241127075157.856029-5-damon.ding@xxxxxxxxxxxxxx/