Re: [PATCH v6 5/7] clk: qcom: clk-alpha-pll: add Lucid EVO PLL configuration interfaces

[Stephen Boyd <sboyd@kernel.org>]

Quoting Vladimir Zapolskiy (2022-06-03 01:01:01)
> The change adds controls for Lucid EVO PLL configuration and exports

Please read the section from this grep

 $ git grep "This patch" -- Documentation/process/

> control functions to clock controller drivers.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
> Changes from v3 to v6:
> * none.
> 
> Changes from v2 to v3:
> * improved commit subject and description per ask from Bjorn.
> 
>  drivers/clk/qcom/clk-alpha-pll.c | 65 ++++++++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-alpha-pll.h |  5 ++-
>  2 files changed, 69 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c
> index 47879ee5a677..54bad5277802 100644
> --- a/drivers/clk/qcom/clk-alpha-pll.c
> +++ b/drivers/clk/qcom/clk-alpha-pll.c
> @@ -1994,6 +1996,33 @@ const struct clk_ops clk_alpha_pll_zonda_ops = {
>  };
>  EXPORT_SYMBOL_GPL(clk_alpha_pll_zonda_ops);
>  
> +void clk_lucid_evo_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap,
> +                                const struct alpha_pll_config *config)
> +{
> +       clk_alpha_pll_write_config(regmap, PLL_L_VAL(pll), config->l |
> +                       (TRION_PLL_CAL_VAL << LUCID_EVO_PLL_CAL_L_VAL_SHIFT));

Prefer a local variable

	u32 lval = config->l;

	lval |= TRION_PLL_CAL_VAL << LUCID_EVO_PLL_CAL_L_VAL_SHIFT;
	clk_alpha_pll_write_config(regmap, PLL_L_VAL(pll), lval);


> +
> +       clk_alpha_pll_write_config(regmap, PLL_ALPHA_VAL(pll), config->alpha);
> +       clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL(pll), config->config_ctl_val);
> +       clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL_U(pll), config->config_ctl_hi_val);
> +       clk_alpha_pll_write_config(regmap, PLL_CONFIG_CTL_U1(pll), config->config_ctl_hi1_val);
> +       clk_alpha_pll_write_config(regmap, PLL_USER_CTL(pll), config->user_ctl_val);
> +       clk_alpha_pll_write_config(regmap, PLL_USER_CTL_U(pll), config->user_ctl_hi_val);
> +       clk_alpha_pll_write_config(regmap, PLL_TEST_CTL(pll), config->test_ctl_val);
> +       clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val);
> +       clk_alpha_pll_write_config(regmap, PLL_TEST_CTL_U1(pll), config->test_ctl_hi1_val);
> +
> +       /* Disable PLL output */
> +       regmap_update_bits(regmap, PLL_MODE(pll), PLL_OUTCTRL, 0);
> +
> +       /* Set operation mode to STANDBY */
> +       regmap_write(regmap, PLL_OPMODE(pll), PLL_STANDBY);
> +
> +       /* Place the PLL in STANDBY mode */

You mean reset? The comments don't seem very useful.

> +       regmap_update_bits(regmap, PLL_MODE(pll), PLL_RESET_N, PLL_RESET_N);
> +}
> +EXPORT_SYMBOL_GPL(clk_lucid_evo_pll_configure);
> +
>  static int alpha_pll_lucid_evo_enable(struct clk_hw *hw)
>  {
>         struct clk_alpha_pll *pll = to_clk_alpha_pll(hw);