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

[Damon Ding <damon.ding@rock-chips.com>]

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@rock-chips.com>
>>
>> ---
>>
>> 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@rock-chips.com>");
>>   MODULE_AUTHOR("Cristian Ciocaltea <cristian.ciocaltea@collabora.com>");
>> +MODULE_AUTHOR("Damon Ding <damon.ding@rock-chips.com>");
>>   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@rock-chips.com/