On Fri, 22 Dec 2023 at 15:01, Abel Vesa <abel.vesa@xxxxxxxxxx> wrote: > > Future platforms should not use different compatibles to differentiate > between eDP and DP mode. Instead, they should use a single compatible as the > IP block is the same. It will be the job of the controller to set the submode > of the PHY accordingly. Rework the device match config data so that it only > keeps the different knobs rather than swing and pre-emphasis tables. > > The existing platforms will remain with separate compatibles for each mode. > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx> > --- > drivers/phy/qualcomm/phy-qcom-edp.c | 90 ++++++++++++++++++++++++++++--------- > 1 file changed, 69 insertions(+), 21 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c > index 8e5078304646..efd7015c73ec 100644 > --- a/drivers/phy/qualcomm/phy-qcom-edp.c > +++ b/drivers/phy/qualcomm/phy-qcom-edp.c > @@ -14,6 +14,7 @@ > #include <linux/module.h> > #include <linux/of.h> > #include <linux/phy/phy.h> > +#include <linux/phy/phy-dp.h> > #include <linux/platform_device.h> > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > @@ -68,19 +69,21 @@ > > #define TXn_TRAN_DRVR_EMP_EN 0x0078 > > -struct qcom_edp_cfg { > - bool is_dp; > - > - /* DP PHY swing and pre_emphasis tables */ > +struct qcom_edp_swing_pre_emph_cfg { > const u8 (*swing_hbr_rbr)[4][4]; > const u8 (*swing_hbr3_hbr2)[4][4]; > const u8 (*pre_emphasis_hbr_rbr)[4][4]; > const u8 (*pre_emphasis_hbr3_hbr2)[4][4]; > }; > > +struct qcom_edp_phy_cfg { > + bool is_edp; > + bool needs_swing_pre_emph_cfg; I think something like needs_voltage_config sounds simpler and prettier. > +}; > + > struct qcom_edp { > struct device *dev; > - const struct qcom_edp_cfg *cfg; > + const struct qcom_edp_swing_pre_emph_cfg *swing_pre_emph_cfg; > > struct phy *phy; > > @@ -96,6 +99,8 @@ struct qcom_edp { > > struct clk_bulk_data clks[2]; > struct regulator_bulk_data supplies[2]; > + > + bool is_edp; > }; > > static const u8 dp_swing_hbr_rbr[4][4] = { > @@ -126,8 +131,7 @@ static const u8 dp_pre_emp_hbr2_hbr3[4][4] = { > { 0x04, 0xff, 0xff, 0xff } > }; > > -static const struct qcom_edp_cfg dp_phy_cfg = { > - .is_dp = true, > +static const struct qcom_edp_swing_pre_emph_cfg dp_phy_swing_pre_emph_cfg = { > .swing_hbr_rbr = &dp_swing_hbr_rbr, > .swing_hbr3_hbr2 = &dp_swing_hbr2_hbr3, > .pre_emphasis_hbr_rbr = &dp_pre_emp_hbr_rbr, > @@ -162,18 +166,29 @@ static const u8 edp_pre_emp_hbr2_hbr3[4][4] = { > { 0x00, 0xff, 0xff, 0xff } > }; > > -static const struct qcom_edp_cfg edp_phy_cfg = { > - .is_dp = false, > +static const struct qcom_edp_swing_pre_emph_cfg edp_phy_swing_pre_emph_cfg = { > .swing_hbr_rbr = &edp_swing_hbr_rbr, > .swing_hbr3_hbr2 = &edp_swing_hbr2_hbr3, > .pre_emphasis_hbr_rbr = &edp_pre_emp_hbr_rbr, > .pre_emphasis_hbr3_hbr2 = &edp_pre_emp_hbr2_hbr3, > }; > > +static struct qcom_edp_phy_cfg sc7280_dp_phy_cfg = { > +}; > + > +static struct qcom_edp_phy_cfg sc8280xp_dp_phy_cfg = { > + .needs_swing_pre_emph_cfg = true, > +}; > + > +static struct qcom_edp_phy_cfg sc8280xp_edp_phy_cfg = { > + .is_edp = true, > + .needs_swing_pre_emph_cfg = true, > +}; > + > static int qcom_edp_phy_init(struct phy *phy) > { > struct qcom_edp *edp = phy_get_drvdata(phy); > - const struct qcom_edp_cfg *cfg = edp->cfg; > + const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg; > int ret; > u8 cfg8; > > @@ -200,7 +215,7 @@ static int qcom_edp_phy_init(struct phy *phy) > DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN, > edp->edp + DP_PHY_PD_CTL); > > - if (cfg && cfg->is_dp) > + if (cfg && !edp->is_edp) > cfg8 = 0xb7; > else > cfg8 = 0x37; > @@ -234,7 +249,7 @@ static int qcom_edp_phy_init(struct phy *phy) > > static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configure_opts_dp *dp_opts) > { > - const struct qcom_edp_cfg *cfg = edp->cfg; > + const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg; > unsigned int v_level = 0; > unsigned int p_level = 0; > u8 ldo_config; > @@ -261,7 +276,7 @@ static int qcom_edp_set_voltages(struct qcom_edp *edp, const struct phy_configur > if (swing == 0xff || emph == 0xff) > return -EINVAL; > > - ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0; > + ldo_config = edp->is_edp ? 0x0 : 0x1; > > writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG); > writel(swing, edp->tx0 + TXn_TX_DRV_LVL); > @@ -447,10 +462,10 @@ static int qcom_edp_set_vco_div(const struct qcom_edp *edp, unsigned long *pixel > static int qcom_edp_phy_power_on(struct phy *phy) > { > const struct qcom_edp *edp = phy_get_drvdata(phy); > - const struct qcom_edp_cfg *cfg = edp->cfg; > + const struct qcom_edp_swing_pre_emph_cfg *cfg = edp->swing_pre_emph_cfg; > u32 bias0_en, drvr0_en, bias1_en, drvr1_en; > unsigned long pixel_freq; > - u8 ldo_config; > + u8 ldo_config = 0x0; > int timeout; > int ret; > u32 val; > @@ -468,7 +483,8 @@ static int qcom_edp_phy_power_on(struct phy *phy) > return timeout; > > > - ldo_config = (cfg && cfg->is_dp) ? 0x1 : 0x0; > + if (cfg && !edp->is_edp) > + ldo_config = 0x1; > > writel(ldo_config, edp->tx0 + TXn_LDO_CONFIG); > writel(ldo_config, edp->tx1 + TXn_LDO_CONFIG); > @@ -589,6 +605,31 @@ static int qcom_edp_phy_power_off(struct phy *phy) > return 0; > } > > +static int qcom_edp_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode) > +{ > + struct qcom_edp *edp = phy_get_drvdata(phy); > + > + if (mode != PHY_MODE_DP) > + return -EINVAL; > + > + switch (submode) { > + case PHY_SUBMODE_DP: > + edp->swing_pre_emph_cfg = &dp_phy_swing_pre_emph_cfg; > + edp->is_edp = false; > + break; > + > + case PHY_SUBMODE_EDP: > + edp->swing_pre_emph_cfg = &edp_phy_swing_pre_emph_cfg; Won't this override the sc7280 config which doesn't set the .needs_swing_pre_emph_cfg? So even > + edp->is_edp = true; > + break; > + > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > static int qcom_edp_phy_exit(struct phy *phy) > { > struct qcom_edp *edp = phy_get_drvdata(phy); > @@ -604,6 +645,7 @@ static const struct phy_ops qcom_edp_ops = { > .configure = qcom_edp_phy_configure, > .power_on = qcom_edp_phy_power_on, > .power_off = qcom_edp_phy_power_off, > + .set_mode = qcom_edp_phy_set_mode, > .exit = qcom_edp_phy_exit, > .owner = THIS_MODULE, > }; > @@ -770,6 +812,7 @@ static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np) > > static int qcom_edp_phy_probe(struct platform_device *pdev) > { > + const struct qcom_edp_phy_cfg *cfg = of_device_get_match_data(&pdev->dev); > struct phy_provider *phy_provider; > struct device *dev = &pdev->dev; > struct qcom_edp *edp; > @@ -780,7 +823,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > return -ENOMEM; > > edp->dev = dev; > - edp->cfg = of_device_get_match_data(&pdev->dev); > + edp->is_edp = cfg->is_edp; > + > + if (cfg->needs_swing_pre_emph_cfg) > + edp->swing_pre_emph_cfg = edp->is_edp ? > + &edp_phy_swing_pre_emph_cfg : > + &dp_phy_swing_pre_emph_cfg; > > edp->edp = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(edp->edp)) > @@ -839,10 +887,10 @@ static int qcom_edp_phy_probe(struct platform_device *pdev) > } > > static const struct of_device_id qcom_edp_phy_match_table[] = { > - { .compatible = "qcom,sc7280-edp-phy" }, > - { .compatible = "qcom,sc8180x-edp-phy" }, > - { .compatible = "qcom,sc8280xp-dp-phy", .data = &dp_phy_cfg }, > - { .compatible = "qcom,sc8280xp-edp-phy", .data = &edp_phy_cfg }, > + { .compatible = "qcom,sc7280-edp-phy" , .data = &sc7280_dp_phy_cfg, }, > + { .compatible = "qcom,sc8180x-edp-phy", .data = &sc7280_dp_phy_cfg, }, > + { .compatible = "qcom,sc8280xp-dp-phy", .data = &sc8280xp_dp_phy_cfg, }, > + { .compatible = "qcom,sc8280xp-edp-phy", .data = &sc8280xp_edp_phy_cfg, }, > { } > }; > MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table); > > -- > 2.34.1 > -- With best wishes Dmitry