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.
>
> 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.
> +
> +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?
> + 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;
> +
> + 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
>
--
With best wishes
Dmitry
