From: Stephen Boyd <sboyd@kernel.org>
To: Zong Li <zong.li@sifive.com>,
aou@eecs.berkeley.edu, linux-clk@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org,
mturquette@baylibre.com, palmer@dabbelt.com,
paul.walmsley@sifive.com, yash.shah@sifive.com
Cc: Zong Li <zong.li@sifive.com>
Subject: Re: [PATCH 4/4] clk: sifive: Refactor __prci_clock array by using macro
Date: Wed, 04 Nov 2020 18:50:45 -0800 [thread overview]
Message-ID: <160454464591.3965362.9361884545184336266@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <8ad64f9814137c5255d43d4ba670b5fcd15837ab.1602838910.git.zong.li@sifive.com>
Quoting Zong Li (2020-10-16 02:18:26)
> Refactor code by using DEFINE_PRCI_CLOCK to define each clock
> and reduce duplicate code.
What is duplicate?
>
> Signed-off-by: Zong Li <zong.li@sifive.com>
> ---
> drivers/clk/sifive/fu540-prci.c | 38 ++++++----------
> drivers/clk/sifive/fu540-prci.h | 2 +-
> drivers/clk/sifive/fu740-prci.c | 74 ++++++++++++--------------------
> drivers/clk/sifive/fu740-prci.h | 2 +-
> drivers/clk/sifive/sifive-prci.c | 2 +-
> drivers/clk/sifive/sifive-prci.h | 10 ++++-
> 6 files changed, 53 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c
> index 840b97bfff85..d43b9a9984f6 100644
> --- a/drivers/clk/sifive/fu540-prci.c
> +++ b/drivers/clk/sifive/fu540-prci.c
> @@ -54,29 +54,19 @@ static const struct clk_ops sifive_fu540_prci_tlclksel_clk_ops = {
> .recalc_rate = sifive_prci_tlclksel_recalc_rate,
> };
>
> +DEFINE_PRCI_CLOCK(fu540, corepll, hfclk,
> + &sifive_fu540_prci_wrpll_clk_ops, &__prci_corepll_data);
> +DEFINE_PRCI_CLOCK(fu540, ddrpll, hfclk,
> + &sifive_fu540_prci_wrpll_ro_clk_ops, &__prci_ddrpll_data);
> +DEFINE_PRCI_CLOCK(fu540, gemgxlpll, hfclk,
> + &sifive_fu540_prci_wrpll_clk_ops, &__prci_gemgxlpll_data);
> +DEFINE_PRCI_CLOCK(fu540, tlclk, corepll,
> + &sifive_fu540_prci_tlclksel_clk_ops, NULL);
Readability looks to decrease with this change. Why should all us code
reviewers suffer because the code author wants to type a few less
characters? Named initializers are useful so we don't have to hold each
macro argument in our head and map it to the struct member that is being
initialized.
> +
> /* List of clock controls provided by the PRCI */
> -struct __prci_clock __prci_init_clocks_fu540[] = {
> - [PRCI_CLK_COREPLL] = {
> - .name = "corepll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu540_prci_wrpll_clk_ops,
> - .pwd = &__prci_corepll_data,
> - },
> - [PRCI_CLK_DDRPLL] = {
> - .name = "ddrpll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu540_prci_wrpll_ro_clk_ops,
> - .pwd = &__prci_ddrpll_data,
> - },
> - [PRCI_CLK_GEMGXLPLL] = {
> - .name = "gemgxlpll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu540_prci_wrpll_clk_ops,
> - .pwd = &__prci_gemgxlpll_data,
> - },
> - [PRCI_CLK_TLCLK] = {
> - .name = "tlclk",
> - .parent_name = "corepll",
> - .ops = &sifive_fu540_prci_tlclksel_clk_ops,
> - },
> +struct __prci_clock *__prci_init_clocks_fu540[] = {
> + [PRCI_CLK_COREPLL] = &fu540_corepll,
> + [PRCI_CLK_DDRPLL] = &fu540_ddrpll,
> + [PRCI_CLK_GEMGXLPLL] = &fu540_gemgxlpll,
> + [PRCI_CLK_TLCLK] = &fu540_tlclk,
> };
> diff --git a/drivers/clk/sifive/fu540-prci.h b/drivers/clk/sifive/fu540-prci.h
> index c8271efa7bdc..281200cd8848 100644
> --- a/drivers/clk/sifive/fu540-prci.h
> +++ b/drivers/clk/sifive/fu540-prci.h
> @@ -11,7 +11,7 @@
>
> #define NUM_CLOCK_FU540 4
>
> -extern struct __prci_clock __prci_init_clocks_fu540[NUM_CLOCK_FU540];
> +extern struct __prci_clock *__prci_init_clocks_fu540[NUM_CLOCK_FU540];
>
> static const struct prci_clk_desc prci_clk_fu540 = {
> .clks = __prci_init_clocks_fu540,
> diff --git a/drivers/clk/sifive/fu740-prci.c b/drivers/clk/sifive/fu740-prci.c
> index 3b87e273c3eb..676cad2c3886 100644
> --- a/drivers/clk/sifive/fu740-prci.c
> +++ b/drivers/clk/sifive/fu740-prci.c
> @@ -71,52 +71,32 @@ static const struct clk_ops sifive_fu740_prci_hfpclkplldiv_clk_ops = {
> .recalc_rate = sifive_prci_hfpclkplldiv_recalc_rate,
> };
>
> +
> +DEFINE_PRCI_CLOCK(fu740, corepll, hfclk,
> + &sifive_fu740_prci_wrpll_clk_ops, &__prci_corepll_data);
> +DEFINE_PRCI_CLOCK(fu740, ddrpll, hfclk,
> + &sifive_fu740_prci_wrpll_ro_clk_ops, &__prci_ddrpll_data);
> +DEFINE_PRCI_CLOCK(fu740, gemgxlpll, hfclk,
> + &sifive_fu740_prci_wrpll_clk_ops, &__prci_gemgxlpll_data);
> +DEFINE_PRCI_CLOCK(fu740, dvfscorepll, hfclk,
> + &sifive_fu740_prci_wrpll_clk_ops, &__prci_dvfscorepll_data);
> +DEFINE_PRCI_CLOCK(fu740, hfpclkpll, hfclk,
> + &sifive_fu740_prci_wrpll_clk_ops, &__prci_hfpclkpll_data);
> +DEFINE_PRCI_CLOCK(fu740, cltxpll, hfclk,
> + &sifive_fu740_prci_wrpll_clk_ops, &__prci_cltxpll_data);
> +DEFINE_PRCI_CLOCK(fu740, tlclk, corepll,
> + &sifive_fu740_prci_tlclksel_clk_ops, NULL);
> +DEFINE_PRCI_CLOCK(fu740, pclk, hfpclkpll,
> + &sifive_fu740_prci_hfpclkplldiv_clk_ops, NULL);
> +
> /* List of clock controls provided by the PRCI */
> -struct __prci_clock __prci_init_clocks_fu740[] = {
> - [PRCI_CLK_COREPLL] = {
> - .name = "corepll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu740_prci_wrpll_clk_ops,
> - .pwd = &__prci_corepll_data,
> - },
> - [PRCI_CLK_DDRPLL] = {
> - .name = "ddrpll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu740_prci_wrpll_ro_clk_ops,
> - .pwd = &__prci_ddrpll_data,
> - },
> - [PRCI_CLK_GEMGXLPLL] = {
> - .name = "gemgxlpll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu740_prci_wrpll_clk_ops,
> - .pwd = &__prci_gemgxlpll_data,
> - },
> - [PRCI_CLK_DVFSCOREPLL] = {
> - .name = "dvfscorepll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu740_prci_wrpll_clk_ops,
> - .pwd = &__prci_dvfscorepll_data,
> - },
> - [PRCI_CLK_HFPCLKPLL] = {
> - .name = "hfpclkpll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu740_prci_wrpll_clk_ops,
> - .pwd = &__prci_hfpclkpll_data,
> - },
> - [PRCI_CLK_CLTXPLL] = {
> - .name = "cltxpll",
> - .parent_name = "hfclk",
> - .ops = &sifive_fu740_prci_wrpll_clk_ops,
> - .pwd = &__prci_cltxpll_data,
> - },
> - [PRCI_CLK_TLCLK] = {
> - .name = "tlclk",
> - .parent_name = "corepll",
> - .ops = &sifive_fu740_prci_tlclksel_clk_ops,
> - },
> - [PRCI_CLK_PCLK] = {
> - .name = "pclk",
> - .parent_name = "hfpclkpll",
> - .ops = &sifive_fu740_prci_hfpclkplldiv_clk_ops,
> - },
> +struct __prci_clock *__prci_init_clocks_fu740[] = {
> + [PRCI_CLK_COREPLL] = &fu740_corepll,
> + [PRCI_CLK_DDRPLL] = &fu740_ddrpll,
> + [PRCI_CLK_GEMGXLPLL] = &fu740_gemgxlpll,
> + [PRCI_CLK_DVFSCOREPLL] = &fu740_dvfscorepll,
> + [PRCI_CLK_HFPCLKPLL] = &fu740_hfpclkpll,
> + [PRCI_CLK_CLTXPLL] = &fu740_cltxpll,
> + [PRCI_CLK_TLCLK] = &fu740_tlclk,
> + [PRCI_CLK_PCLK] = &fu740_pclk,
> };
I suppose this is fine and then non-macro structs above this array of
pointers.
> diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
> index 13ef971f7764..3f03295f719a 100644
> --- a/drivers/clk/sifive/fu740-prci.h
> +++ b/drivers/clk/sifive/fu740-prci.h
> @@ -11,7 +11,7 @@
>
> #define NUM_CLOCK_FU740 8
>
> -extern struct __prci_clock __prci_init_clocks_fu740[NUM_CLOCK_FU740];
> +extern struct __prci_clock *__prci_init_clocks_fu740[NUM_CLOCK_FU740];
>
> static const struct prci_clk_desc prci_clk_fu740 = {
> .clks = __prci_init_clocks_fu740,
> diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
> index 4098dbc5881a..2ef3f9f91b33 100644
> --- a/drivers/clk/sifive/sifive-prci.c
> +++ b/drivers/clk/sifive/sifive-prci.c
> @@ -431,7 +431,7 @@ static int __prci_register_clocks(struct device *dev, struct __prci_data *pd,
>
> /* Register PLLs */
> for (i = 0; i < desc->num_clks; ++i) {
> - pic = &(desc->clks[i]);
> + pic = desc->clks[i];
This is related how?
>
> init.name = pic->name;
> init.parent_names = &pic->parent_name;
> diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
> index bc0646bc9c3e..e6c9f72e20de 100644
> --- a/drivers/clk/sifive/sifive-prci.h
> +++ b/drivers/clk/sifive/sifive-prci.h
> @@ -253,6 +253,14 @@ struct __prci_clock {
> struct __prci_data *pd;
> };
>
> +#define DEFINE_PRCI_CLOCK(_platform, _name, _parent, _ops, _pwd) \
> + static struct __prci_clock _platform##_##_name = { \
> + .name = #_name, \
> + .parent_name = #_parent, \
> + .ops = _ops, \
> + .pwd = _pwd, \
> + } \
> +
> #define clk_hw_to_prci_clock(pwd) container_of(pwd, struct __prci_clock, hw)
>
> /*
> @@ -261,7 +269,7 @@ struct __prci_clock {
> * @num_clks: the number of element of clks
> */
> struct prci_clk_desc {
> - struct __prci_clock *clks;
> + struct __prci_clock **clks;
Huh? Nothing in the commit text mentions this.
> size_t num_clks;
> };
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2020-11-05 3:33 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-16 9:18 [PATCH 0/4] clk: add driver for the SiFive FU740 Zong Li
2020-10-16 9:18 ` [PATCH 1/4] clk: sifive: Extract prci core to common base Zong Li
2020-11-05 2:45 ` Stephen Boyd
2020-11-05 7:22 ` Zong Li
2020-11-05 9:19 ` Andreas Schwab
2020-11-05 9:30 ` Zong Li
2020-10-16 9:18 ` [PATCH 2/4] clk: sifive: Use common name for prci configuration Zong Li
2020-11-05 2:46 ` Stephen Boyd
2020-11-05 6:56 ` Zong Li
2020-11-05 18:54 ` Palmer Dabbelt
2020-10-16 9:18 ` [PATCH 3/4] clk: sifive: Add a driver for the SiFive FU740 PRCI IP block Zong Li
2020-10-16 9:18 ` [PATCH 4/4] clk: sifive: Refactor __prci_clock array by using macro Zong Li
2020-11-05 2:50 ` Stephen Boyd [this message]
2020-11-05 7:16 ` Zong Li
2020-10-16 14:17 ` [PATCH 0/4] clk: add driver for the SiFive FU740 Sean Anderson
2020-10-16 14:56 ` Zong Li
2020-11-04 6:37 ` Zong Li
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=160454464591.3965362.9361884545184336266@swboyd.mtv.corp.google.com \
--to=sboyd@kernel.org \
--cc=aou@eecs.berkeley.edu \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mturquette@baylibre.com \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=yash.shah@sifive.com \
--cc=zong.li@sifive.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).