Drop the rate parameter from a bunch of internal helpers and, instead,
make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver
data.
Additionally, rename the rather ambiguous ->rate member of struct
rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config()
updates it only after all the other operations have been successful.
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 59 ++++++++++++-----------
1 file changed, 30 insertions(+), 29 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
index a62c634f5254e742088515c3b21c05f95acbaded..98f831c3704f5cd65dcf29a4f0a12862a4aa0482 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
@@ -401,7 +401,7 @@ struct rk_hdptx_phy {
/* clk provider */
struct clk_hw hw;
- unsigned long rate;
+ unsigned long hw_rate;
bool restrict_rate_change;
atomic_t usage_count;
@@ -968,33 +968,34 @@ static bool rk_hdptx_phy_clk_pll_calc(unsigned long rate,
return true;
}
-static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
- unsigned long rate)
+static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx)
{
const struct ropll_config *cfg = NULL;
struct ropll_config rc = {0};
- int i;
+ int ret, i;
- hdptx->rate = rate;
+ if (!hdptx->hdmi_cfg.tmds_char_rate)
+ return 0;
for (i = 0; i < ARRAY_SIZE(ropll_tmds_cfg); i++)
- if (rate == ropll_tmds_cfg[i].rate) {
+ if (hdptx->hdmi_cfg.tmds_char_rate == ropll_tmds_cfg[i].rate) {
cfg = &ropll_tmds_cfg[i];
break;
}
if (!cfg) {
- if (rk_hdptx_phy_clk_pll_calc(rate, &rc)) {
- cfg = &rc;
- } else {
- dev_err(hdptx->dev, "%s cannot find pll cfg\n", __func__);
+ if (!rk_hdptx_phy_clk_pll_calc(hdptx->hdmi_cfg.tmds_char_rate, &rc)) {
+ dev_err(hdptx->dev, "%s cannot find pll cfg for rate=%llu\n",
+ __func__, hdptx->hdmi_cfg.tmds_char_rate);
return -EINVAL;
}
+
+ cfg = &rc;
}
- dev_dbg(hdptx->dev, "%s rate=%lu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
- __func__, rate, cfg->pms_mdiv, cfg->pms_sdiv + 1, cfg->sdm_en,
- cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
+ dev_dbg(hdptx->dev, "%s rate=%llu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
+ __func__, hdptx->hdmi_cfg.tmds_char_rate, cfg->pms_mdiv, cfg->pms_sdiv + 1,
+ cfg->sdm_en, cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
rk_hdptx_pre_power_up(hdptx);
@@ -1030,17 +1031,20 @@ static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
regmap_update_bits(hdptx->regmap, CMN_REG(0086), PLL_PCG_CLK_EN_MASK,
FIELD_PREP(PLL_PCG_CLK_EN_MASK, 0x1));
- return rk_hdptx_post_enable_pll(hdptx);
+ ret = rk_hdptx_post_enable_pll(hdptx);
+ if (!ret)
+ hdptx->hw_rate = hdptx->hdmi_cfg.tmds_char_rate;
+
+ return ret;
}
-static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx,
- unsigned long rate)
+static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx)
{
rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_common_sb_init_seq);
regmap_write(hdptx->regmap, LNTOP_REG(0200), 0x06);
- if (rate > HDMI14_MAX_RATE) {
+ if (hdptx->hdmi_cfg.tmds_char_rate > HDMI14_MAX_RATE) {
/* For 1/40 bitrate clk */
rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_tmds_lntop_highbr_seq);
} else {
@@ -1092,8 +1096,7 @@ static void rk_hdptx_dp_reset(struct rk_hdptx_phy *hdptx)
HDPTX_I_BGR_EN << 16 | FIELD_PREP(HDPTX_I_BGR_EN, 0x0));
}
-static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
- unsigned long rate)
+static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx)
{
enum phy_mode mode = phy_get_mode(hdptx->phy);
u32 status;
@@ -1112,11 +1115,9 @@ static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
if (mode == PHY_MODE_DP) {
rk_hdptx_dp_reset(hdptx);
} else {
- if (rate) {
- ret = rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
- if (ret)
- goto dec_usage;
- }
+ ret = rk_hdptx_ropll_tmds_cmn_config(hdptx);
+ if (ret)
+ goto dec_usage;
}
return 0;
@@ -1429,7 +1430,7 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
dev_dbg(hdptx->dev, "%s rate=%llu\n", __func__, hdptx->hdmi_cfg.tmds_char_rate);
}
- ret = rk_hdptx_phy_consumer_get(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
+ ret = rk_hdptx_phy_consumer_get(hdptx);
if (ret)
return ret;
@@ -1460,7 +1461,7 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
regmap_write(hdptx->grf, GRF_HDPTX_CON0,
HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
- ret = rk_hdptx_ropll_tmds_mode_config(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
+ ret = rk_hdptx_ropll_tmds_mode_config(hdptx);
if (ret)
rk_hdptx_phy_consumer_put(hdptx, true);
}
@@ -1828,7 +1829,7 @@ static int rk_hdptx_phy_clk_prepare(struct clk_hw *hw)
{
struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
- return rk_hdptx_phy_consumer_get(hdptx, hdptx->rate);
+ return rk_hdptx_phy_consumer_get(hdptx);
}
static void rk_hdptx_phy_clk_unprepare(struct clk_hw *hw)
@@ -1843,7 +1844,7 @@ static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
{
struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
- return hdptx->rate;
+ return hdptx->hw_rate;
}
static long rk_hdptx_phy_clk_round_rate(struct clk_hw *hw, unsigned long rate,
@@ -1895,7 +1896,7 @@ static int rk_hdptx_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
* while the latter being executed only once, i.e. when clock remains
* in the prepared state during rate changes.
*/
- return rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
+ return rk_hdptx_ropll_tmds_cmn_config(hdptx);
}
static const struct clk_ops hdptx_phy_clk_ops = {
--
2.48.1On Sat, 8 Mar 2025 at 14:21, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
>
> Drop the rate parameter from a bunch of internal helpers and, instead,
> make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver
> data.
>
> Additionally, rename the rather ambiguous ->rate member of struct
> rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config()
Would it be better to name it tmds_char_rate?
> updates it only after all the other operations have been successful.
Please don't mix refactoring / renames and the actual code changes.
>
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> ---
> drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 59 ++++++++++++-----------
> 1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index a62c634f5254e742088515c3b21c05f95acbaded..98f831c3704f5cd65dcf29a4f0a12862a4aa0482 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -401,7 +401,7 @@ struct rk_hdptx_phy {
>
> /* clk provider */
> struct clk_hw hw;
> - unsigned long rate;
> + unsigned long hw_rate;
> bool restrict_rate_change;
>
> atomic_t usage_count;
> @@ -968,33 +968,34 @@ static bool rk_hdptx_phy_clk_pll_calc(unsigned long rate,
> return true;
> }
>
> -static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
> - unsigned long rate)
> +static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx)
> {
> const struct ropll_config *cfg = NULL;
> struct ropll_config rc = {0};
> - int i;
> + int ret, i;
>
> - hdptx->rate = rate;
> + if (!hdptx->hdmi_cfg.tmds_char_rate)
> + return 0;
>
> for (i = 0; i < ARRAY_SIZE(ropll_tmds_cfg); i++)
> - if (rate == ropll_tmds_cfg[i].rate) {
> + if (hdptx->hdmi_cfg.tmds_char_rate == ropll_tmds_cfg[i].rate) {
> cfg = &ropll_tmds_cfg[i];
> break;
> }
>
> if (!cfg) {
> - if (rk_hdptx_phy_clk_pll_calc(rate, &rc)) {
> - cfg = &rc;
> - } else {
> - dev_err(hdptx->dev, "%s cannot find pll cfg\n", __func__);
> + if (!rk_hdptx_phy_clk_pll_calc(hdptx->hdmi_cfg.tmds_char_rate, &rc)) {
> + dev_err(hdptx->dev, "%s cannot find pll cfg for rate=%llu\n",
> + __func__, hdptx->hdmi_cfg.tmds_char_rate);
> return -EINVAL;
> }
> +
> + cfg = &rc;
> }
>
> - dev_dbg(hdptx->dev, "%s rate=%lu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
> - __func__, rate, cfg->pms_mdiv, cfg->pms_sdiv + 1, cfg->sdm_en,
> - cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
> + dev_dbg(hdptx->dev, "%s rate=%llu mdiv=%u sdiv=%u sdm_en=%u k_sign=%u k=%u lc=%u\n",
> + __func__, hdptx->hdmi_cfg.tmds_char_rate, cfg->pms_mdiv, cfg->pms_sdiv + 1,
> + cfg->sdm_en, cfg->sdm_num_sign, cfg->sdm_num, cfg->sdm_deno);
>
> rk_hdptx_pre_power_up(hdptx);
>
> @@ -1030,17 +1031,20 @@ static int rk_hdptx_ropll_tmds_cmn_config(struct rk_hdptx_phy *hdptx,
> regmap_update_bits(hdptx->regmap, CMN_REG(0086), PLL_PCG_CLK_EN_MASK,
> FIELD_PREP(PLL_PCG_CLK_EN_MASK, 0x1));
>
> - return rk_hdptx_post_enable_pll(hdptx);
> + ret = rk_hdptx_post_enable_pll(hdptx);
> + if (!ret)
> + hdptx->hw_rate = hdptx->hdmi_cfg.tmds_char_rate;
> +
> + return ret;
> }
>
> -static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx,
> - unsigned long rate)
> +static int rk_hdptx_ropll_tmds_mode_config(struct rk_hdptx_phy *hdptx)
> {
> rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_common_sb_init_seq);
>
> regmap_write(hdptx->regmap, LNTOP_REG(0200), 0x06);
>
> - if (rate > HDMI14_MAX_RATE) {
> + if (hdptx->hdmi_cfg.tmds_char_rate > HDMI14_MAX_RATE) {
> /* For 1/40 bitrate clk */
> rk_hdptx_multi_reg_write(hdptx, rk_hdtpx_tmds_lntop_highbr_seq);
> } else {
> @@ -1092,8 +1096,7 @@ static void rk_hdptx_dp_reset(struct rk_hdptx_phy *hdptx)
> HDPTX_I_BGR_EN << 16 | FIELD_PREP(HDPTX_I_BGR_EN, 0x0));
> }
>
> -static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
> - unsigned long rate)
> +static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx)
> {
> enum phy_mode mode = phy_get_mode(hdptx->phy);
> u32 status;
> @@ -1112,11 +1115,9 @@ static int rk_hdptx_phy_consumer_get(struct rk_hdptx_phy *hdptx,
> if (mode == PHY_MODE_DP) {
> rk_hdptx_dp_reset(hdptx);
> } else {
> - if (rate) {
> - ret = rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
> - if (ret)
> - goto dec_usage;
> - }
> + ret = rk_hdptx_ropll_tmds_cmn_config(hdptx);
> + if (ret)
> + goto dec_usage;
> }
>
> return 0;
> @@ -1429,7 +1430,7 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
> dev_dbg(hdptx->dev, "%s rate=%llu\n", __func__, hdptx->hdmi_cfg.tmds_char_rate);
> }
>
> - ret = rk_hdptx_phy_consumer_get(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
> + ret = rk_hdptx_phy_consumer_get(hdptx);
> if (ret)
> return ret;
>
> @@ -1460,7 +1461,7 @@ static int rk_hdptx_phy_power_on(struct phy *phy)
> regmap_write(hdptx->grf, GRF_HDPTX_CON0,
> HDPTX_MODE_SEL << 16 | FIELD_PREP(HDPTX_MODE_SEL, 0x0));
>
> - ret = rk_hdptx_ropll_tmds_mode_config(hdptx, hdptx->hdmi_cfg.tmds_char_rate);
> + ret = rk_hdptx_ropll_tmds_mode_config(hdptx);
> if (ret)
> rk_hdptx_phy_consumer_put(hdptx, true);
> }
> @@ -1828,7 +1829,7 @@ static int rk_hdptx_phy_clk_prepare(struct clk_hw *hw)
> {
> struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
>
> - return rk_hdptx_phy_consumer_get(hdptx, hdptx->rate);
> + return rk_hdptx_phy_consumer_get(hdptx);
> }
>
> static void rk_hdptx_phy_clk_unprepare(struct clk_hw *hw)
> @@ -1843,7 +1844,7 @@ static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
> {
> struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
>
> - return hdptx->rate;
> + return hdptx->hw_rate;
> }
>
> static long rk_hdptx_phy_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> @@ -1895,7 +1896,7 @@ static int rk_hdptx_phy_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> * while the latter being executed only once, i.e. when clock remains
> * in the prepared state during rate changes.
> */
> - return rk_hdptx_ropll_tmds_cmn_config(hdptx, rate);
> + return rk_hdptx_ropll_tmds_cmn_config(hdptx);
> }
>
> static const struct clk_ops hdptx_phy_clk_ops = {
>
> --
> 2.48.1
>
--
With best wishes
Dmitry
On 3/9/25 12:17 PM, Dmitry Baryshkov wrote: > On Sat, 8 Mar 2025 at 14:21, Cristian Ciocaltea > <cristian.ciocaltea@collabora.com> wrote: >> >> Drop the rate parameter from a bunch of internal helpers and, instead, >> make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver >> data. >> >> Additionally, rename the rather ambiguous ->rate member of struct >> rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config() > > Would it be better to name it tmds_char_rate? This is only used in rk_hdptx_phy_clk_recalc_rate() to provide the actual rate programmed in hardware, hence the "hw_rate" naming. Renaming it to tmds_char_rate might add some confusion with the similar one in ->hdmi_cfg, but I don't really have any strong preference here. >> updates it only after all the other operations have been successful. > > Please don't mix refactoring / renames and the actual code changes. Right, I should have moved that to a dedicated patch. Will handle in the next revision. Thanks, Cristian
On Sun, 9 Mar 2025 at 12:32, Cristian Ciocaltea <cristian.ciocaltea@collabora.com> wrote: > > On 3/9/25 12:17 PM, Dmitry Baryshkov wrote: > > On Sat, 8 Mar 2025 at 14:21, Cristian Ciocaltea > > <cristian.ciocaltea@collabora.com> wrote: > >> > >> Drop the rate parameter from a bunch of internal helpers and, instead, > >> make better use of the newly introduced ->hdmi_cfg.tmds_char_rate driver > >> data. > >> > >> Additionally, rename the rather ambiguous ->rate member of struct > >> rk_hdptx_phy to ->hw_rate and ensure rk_hdptx_ropll_tmds_cmn_config() > > > > Would it be better to name it tmds_char_rate? > > This is only used in rk_hdptx_phy_clk_recalc_rate() to provide the > actual rate programmed in hardware, hence the "hw_rate" naming. Ack > Renaming it to tmds_char_rate might add some confusion with the similar > one in ->hdmi_cfg, but I don't really have any strong preference here. > > >> updates it only after all the other operations have been successful. > > > > Please don't mix refactoring / renames and the actual code changes. > > Right, I should have moved that to a dedicated patch. Will handle in > the next revision. > > Thanks, > Cristian -- With best wishes Dmitry
© 2016 - 2026 Red Hat, Inc.