linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@kernel.org>
To: Jonathan Marek <jonathan@marek.ca>, linux-arm-msm@vger.kernel.org
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] clk: qcom: add support for SM8350 DISPCC
Date: Wed, 02 Jun 2021 14:34:29 -0700	[thread overview]
Message-ID: <162266966911.4130789.18195427738586432385@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <20210519001802.1863-1-jonathan@marek.ca>

Quoting Jonathan Marek (2021-05-18 17:18:01)
> diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
> index de09cd5c209f..5f22a715e2f0 100644
> --- a/drivers/clk/qcom/dispcc-sm8250.c
> +++ b/drivers/clk/qcom/dispcc-sm8250.c
> @@ -36,6 +36,10 @@ static struct pll_vco vco_table[] = {
>         { 249600000, 2000000000, 0 },
>  };
>  
> +static struct pll_vco lucid_5lpe_vco[] = {

const

> +       { 249600000, 1750000000, 0 },
> +};
> +
>  static struct alpha_pll_config disp_cc_pll0_config = {
>         .l = 0x47,
>         .alpha = 0xE000,
> @@ -1039,6 +1043,7 @@ static const struct qcom_cc_desc disp_cc_sm8250_desc = {
>  static const struct of_device_id disp_cc_sm8250_match_table[] = {
>         { .compatible = "qcom,sm8150-dispcc" },
>         { .compatible = "qcom,sm8250-dispcc" },
> +       { .compatible = "qcom,sm8350-dispcc" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, disp_cc_sm8250_match_table);
> @@ -1051,7 +1056,7 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev)
>         if (IS_ERR(regmap))
>                 return PTR_ERR(regmap);
>  
> -       /* note: trion == lucid, except for the prepare() op */
> +       /* Apply differences for SM8150 and SM8350 */
>         BUILD_BUG_ON(CLK_ALPHA_PLL_TYPE_TRION != CLK_ALPHA_PLL_TYPE_LUCID);
>         if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8150-dispcc")) {
>                 disp_cc_pll0_config.config_ctl_hi_val = 0x00002267;
> @@ -1062,8 +1067,62 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev)
>                 disp_cc_pll1_config.config_ctl_hi1_val = 0x00000024;
>                 disp_cc_pll1_config.user_ctl_hi1_val = 0x000000D0;
>                 disp_cc_pll1_init.ops = &clk_alpha_pll_trion_ops;
> +       } else if (of_device_is_compatible(pdev->dev.of_node, "qcom,sm8350-dispcc")) {
> +               static struct clk_rcg2* const rcgs[] = {

Please move this to global scope and give it a name.

> +                       &disp_cc_mdss_byte0_clk_src,
> +                       &disp_cc_mdss_byte1_clk_src,
> +                       &disp_cc_mdss_dp_aux1_clk_src,
> +                       &disp_cc_mdss_dp_aux_clk_src,
> +                       &disp_cc_mdss_dp_link1_clk_src,
> +                       &disp_cc_mdss_dp_link_clk_src,
> +                       &disp_cc_mdss_dp_pixel1_clk_src,
> +                       &disp_cc_mdss_dp_pixel2_clk_src,
> +                       &disp_cc_mdss_dp_pixel_clk_src,
> +                       &disp_cc_mdss_esc0_clk_src,
> +                       &disp_cc_mdss_mdp_clk_src,
> +                       &disp_cc_mdss_pclk0_clk_src,
> +                       &disp_cc_mdss_pclk1_clk_src,
> +                       &disp_cc_mdss_rot_clk_src,
> +                       &disp_cc_mdss_vsync_clk_src,
> +               };
> +               static struct clk_regmap_div* const divs[] = {

Move global.

> +                       &disp_cc_mdss_byte0_div_clk_src,
> +                       &disp_cc_mdss_byte1_div_clk_src,
> +                       &disp_cc_mdss_dp_link1_div_clk_src,
> +                       &disp_cc_mdss_dp_link_div_clk_src,
> +               };
> +               unsigned i;

int i? I doubt being unsigned helps.

> +               static bool offset_applied = false;
> +
> +               /* only apply the offsets once (in case of deferred probe) */
> +               if (!offset_applied) {

Maybe instead of doing this in probe it can be done when the driver is
added in module init? It would mean searching the DT for the compatible
string and then if it is present running the subtraction code, but at
least we would only do it once and the code would be thrown away after
init.

> +                       for (i = 0; i < ARRAY_SIZE(rcgs); i++)
> +                               rcgs[i]->cmd_rcgr -= 4;
> +
> +                       for (i = 0; i < ARRAY_SIZE(divs); i++) {
> +                               divs[i]->reg -= 4;
> +                               divs[i]->width = 4;
> +                       }
> +
> +                       disp_cc_mdss_ahb_clk.halt_reg -= 4;
> +                       disp_cc_mdss_ahb_clk.clkr.enable_reg -= 4;
> +
> +                       offset_applied = true;
> +               }
> +
> +               disp_cc_mdss_ahb_clk_src.cmd_rcgr = 0x22a0;
> +
> +               disp_cc_pll0_config.config_ctl_hi1_val = 0x2A9A699C;
> +               disp_cc_pll0_config.test_ctl_hi1_val = 0x01800000;
> +               disp_cc_pll0_init.ops = &clk_alpha_pll_lucid_5lpe_ops;
> +               disp_cc_pll0.vco_table = lucid_5lpe_vco;
> +               disp_cc_pll1_config.config_ctl_hi1_val = 0x2A9A699C;

Lowercase hex please.

> +               disp_cc_pll1_config.test_ctl_hi1_val = 0x01800000;
> +               disp_cc_pll1_init.ops = &clk_alpha_pll_lucid_5lpe_ops;
> +               disp_cc_pll1.vco_table = lucid_5lpe_vco;
>         }
>  
> +       /* note for SM8350: downstream lucid_5lpe configure differs slightly */

Is this a TODO?

>         clk_lucid_pll_configure(&disp_cc_pll0, regmap, &disp_cc_pll0_config);
>         clk_lucid_pll_configure(&disp_cc_pll1, regmap, &disp_cc_pll1_config);
>

      parent reply	other threads:[~2021-06-02 21:34 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-19  0:18 Jonathan Marek
2021-05-19  0:18 ` [PATCH v2 2/2] dt-bindings: clock: add QCOM SM8350 display clock bindings Jonathan Marek
2021-06-02 21:27   ` Stephen Boyd
2021-06-04 17:25     ` Jonathan Marek
2021-06-28  2:39       ` Stephen Boyd
2021-06-06  4:11     ` Bjorn Andersson
2021-06-02 21:34 ` Stephen Boyd [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=162266966911.4130789.18195427738586432385@swboyd.mtv.corp.google.com \
    --to=sboyd@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=jonathan@marek.ca \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --subject='Re: [PATCH v2 1/2] clk: qcom: add support for SM8350 DISPCC' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).