| Message ID | 20250321151831.623575-3-elder@riscstar.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | clk: spacemit: add K1 reset support | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | fail | Failed to apply series |
Hi Alex: this patch change relate to clock only, so how about let's fold it into clk patches (which now has not been merged), so we make the code right at first place? cause some moving around and renaming On 10:18 Fri 21 Mar , Alex Elder wrote: > Define a new structure type to be used for describing the OF match data. > Rather than using the array of spacemit_ccu_clk structures for match > data, we use this structure instead. > > Move the definition of the spacemit_ccu_clk structure closer to the top > of the source file, and add the new structure definition below it. > > Shorten the name of spacemit_ccu_register() to be k1_ccu_register(). any good reason to change this? it make the code style inconsistent, do you just change it for shorten function, or want it to be more k1 specific, so next SoC - e.g maybe k2? will introduce another function? > > Signed-off-by: Alex Elder <elder@riscstar.com> > --- > drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++--------- > 1 file changed, 43 insertions(+), 15 deletions(-) > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c > index 44db48ae71313..f7367271396a0 100644 > --- a/drivers/clk/spacemit/ccu-k1.c > +++ b/drivers/clk/spacemit/ccu-k1.c > @@ -129,6 +129,15 @@ > #define APMU_EMAC0_CLK_RES_CTRL 0x3e4 > #define APMU_EMAC1_CLK_RES_CTRL 0x3ec > > +struct spacemit_ccu_clk { > + int id; > + struct clk_hw *hw; > +}; > + > +struct k1_ccu_data { > + struct spacemit_ccu_clk *clk; /* array with sentinel */ > +}; > + > /* APBS clocks start */ > > /* Frequency of pll{1,2} should not be updated at runtime */ > @@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk), > 0); > /* APMU clocks end */ > > -struct spacemit_ccu_clk { > - int id; > - struct clk_hw *hw; > -}; > - > static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = { > { CLK_PLL1, &pll1.common.hw }, > { CLK_PLL2, &pll2.common.hw }, > @@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = { > { 0, NULL }, > }; > > +static const struct k1_ccu_data k1_ccu_apbs_data = { > + .clk = k1_ccu_apbs_clks, > +}; > + > static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = { > { CLK_PLL1_307P2, &pll1_d8_307p2.common.hw }, > { CLK_PLL1_76P8, &pll1_d32_76p8.common.hw }, > @@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = { > { 0, NULL }, > }; > > +static const struct k1_ccu_data k1_ccu_mpmu_data = { > + .clk = k1_ccu_mpmu_clks, > +}; > + > static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = { > { CLK_UART0, &uart0_clk.common.hw }, > { CLK_UART2, &uart2_clk.common.hw }, > @@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = { > { 0, NULL }, > }; > > +static const struct k1_ccu_data k1_ccu_apbc_data = { > + .clk = k1_ccu_apbc_clks, > +}; > + > static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { > { CLK_CCI550, &cci550_clk.common.hw }, > { CLK_CPU_C0_HI, &cpu_c0_hi_clk.common.hw }, > @@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { > { 0, NULL }, > }; > > -static int spacemit_ccu_register(struct device *dev, > - struct regmap *regmap, struct regmap *lock_regmap, > - const struct spacemit_ccu_clk *clks) > +static const struct k1_ccu_data k1_ccu_apmu_data = { > + .clk = k1_ccu_apmu_clks, > +}; > + > +static int k1_ccu_register(struct device *dev, struct regmap *regmap, > + struct regmap *lock_regmap, > + struct spacemit_ccu_clk *clks) > { > const struct spacemit_ccu_clk *clk; > int i, ret, max_id = 0; > @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev, > > clk_data->num = max_id + 1; > > - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); > + if (ret) > + dev_err(dev, "error %d adding clock hardware provider\n", ret); > + > + return ret; I'd use "return 0;", nothing different, just explicitly short ok, I can understand this change ease debug procedure once there is problem. (but I'm fine with either way, failure should rarely happen & will identify early) > } > > static int k1_ccu_probe(struct platform_device *pdev) > { > struct regmap *base_regmap, *lock_regmap = NULL; > struct device *dev = &pdev->dev; > + const struct k1_ccu_data *data; > int ret; > > + data = of_device_get_match_data(dev); > + if (!data) > + return -EINVAL; > + > base_regmap = device_node_to_regmap(dev->of_node); > if (IS_ERR(base_regmap)) > return dev_err_probe(dev, PTR_ERR(base_regmap), > @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev) > "failed to get lock regmap\n"); > } > > - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, > - of_device_get_match_data(dev)); > + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk); > if (ret) > return dev_err_probe(dev, ret, "failed to register clocks\n"); > > @@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev) > static const struct of_device_id of_k1_ccu_match[] = { > { > .compatible = "spacemit,k1-pll", > - .data = k1_ccu_apbs_clks, > + .data = &k1_ccu_apbs_data, > }, > { > .compatible = "spacemit,k1-syscon-mpmu", > - .data = k1_ccu_mpmu_clks, > + .data = &k1_ccu_mpmu_data, > }, > { > .compatible = "spacemit,k1-syscon-apbc", > - .data = k1_ccu_apbc_clks, > + .data = &k1_ccu_apbc_data, > }, > { > .compatible = "spacemit,k1-syscon-apmu", > - .data = k1_ccu_apmu_clks, > + .data = &k1_ccu_apmu_data, > }, > { } { /* sentinel */ } > }; > -- > 2.43.0 >
On 3/22/25 10:50 AM, Yixun Lan wrote: > Hi Alex: > > this patch change relate to clock only, so how about let's fold > it into clk patches (which now has not been merged), so we make > the code right at first place? cause some moving around and renaming No I don't want to do that. The clock patches are Haylen's and the are getting closer to acceptance. Let's not confuse things by adding a bunch of new functionality. Get those patches in, and mine can follow not too long after that. -Alex > > On 10:18 Fri 21 Mar , Alex Elder wrote: >> Define a new structure type to be used for describing the OF match data. >> Rather than using the array of spacemit_ccu_clk structures for match >> data, we use this structure instead. >> >> Move the definition of the spacemit_ccu_clk structure closer to the top >> of the source file, and add the new structure definition below it. >> >> Shorten the name of spacemit_ccu_register() to be k1_ccu_register(). > any good reason to change this? it make the code style inconsistent, > do you just change it for shorten function, or want it to be more k1 > specific, so next SoC - e.g maybe k2? will introduce another function? > >> >> Signed-off-by: Alex Elder <elder@riscstar.com> >> --- >> drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++--------- >> 1 file changed, 43 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c >> index 44db48ae71313..f7367271396a0 100644 >> --- a/drivers/clk/spacemit/ccu-k1.c >> +++ b/drivers/clk/spacemit/ccu-k1.c >> @@ -129,6 +129,15 @@ >> #define APMU_EMAC0_CLK_RES_CTRL 0x3e4 >> #define APMU_EMAC1_CLK_RES_CTRL 0x3ec >> >> +struct spacemit_ccu_clk { >> + int id; >> + struct clk_hw *hw; >> +}; >> + >> +struct k1_ccu_data { >> + struct spacemit_ccu_clk *clk; /* array with sentinel */ >> +}; >> + >> /* APBS clocks start */ >> >> /* Frequency of pll{1,2} should not be updated at runtime */ >> @@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk), >> 0); >> /* APMU clocks end */ >> >> -struct spacemit_ccu_clk { >> - int id; >> - struct clk_hw *hw; >> -}; >> - >> static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = { >> { CLK_PLL1, &pll1.common.hw }, >> { CLK_PLL2, &pll2.common.hw }, >> @@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = { >> { 0, NULL }, >> }; >> >> +static const struct k1_ccu_data k1_ccu_apbs_data = { >> + .clk = k1_ccu_apbs_clks, >> +}; >> + >> static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = { >> { CLK_PLL1_307P2, &pll1_d8_307p2.common.hw }, >> { CLK_PLL1_76P8, &pll1_d32_76p8.common.hw }, >> @@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = { >> { 0, NULL }, >> }; >> >> +static const struct k1_ccu_data k1_ccu_mpmu_data = { >> + .clk = k1_ccu_mpmu_clks, >> +}; >> + >> static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = { >> { CLK_UART0, &uart0_clk.common.hw }, >> { CLK_UART2, &uart2_clk.common.hw }, >> @@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = { >> { 0, NULL }, >> }; >> >> +static const struct k1_ccu_data k1_ccu_apbc_data = { >> + .clk = k1_ccu_apbc_clks, >> +}; >> + >> static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { >> { CLK_CCI550, &cci550_clk.common.hw }, >> { CLK_CPU_C0_HI, &cpu_c0_hi_clk.common.hw }, >> @@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { >> { 0, NULL }, >> }; >> >> -static int spacemit_ccu_register(struct device *dev, >> - struct regmap *regmap, struct regmap *lock_regmap, >> - const struct spacemit_ccu_clk *clks) >> +static const struct k1_ccu_data k1_ccu_apmu_data = { >> + .clk = k1_ccu_apmu_clks, >> +}; >> + >> +static int k1_ccu_register(struct device *dev, struct regmap *regmap, >> + struct regmap *lock_regmap, >> + struct spacemit_ccu_clk *clks) >> { >> const struct spacemit_ccu_clk *clk; >> int i, ret, max_id = 0; >> @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev, >> >> clk_data->num = max_id + 1; >> >> - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); >> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); >> + if (ret) >> + dev_err(dev, "error %d adding clock hardware provider\n", ret); >> + >> + return ret; > I'd use "return 0;", nothing different, just explicitly short > > ok, I can understand this change ease debug procedure once there is problem. > (but I'm fine with either way, failure should rarely happen & will > identify early) > >> } >> >> static int k1_ccu_probe(struct platform_device *pdev) >> { >> struct regmap *base_regmap, *lock_regmap = NULL; >> struct device *dev = &pdev->dev; >> + const struct k1_ccu_data *data; >> int ret; >> >> + data = of_device_get_match_data(dev); >> + if (!data) >> + return -EINVAL; >> + >> base_regmap = device_node_to_regmap(dev->of_node); >> if (IS_ERR(base_regmap)) >> return dev_err_probe(dev, PTR_ERR(base_regmap), >> @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev) >> "failed to get lock regmap\n"); >> } >> >> - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, >> - of_device_get_match_data(dev)); >> + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk); >> if (ret) >> return dev_err_probe(dev, ret, "failed to register clocks\n"); >> >> @@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev) >> static const struct of_device_id of_k1_ccu_match[] = { >> { >> .compatible = "spacemit,k1-pll", >> - .data = k1_ccu_apbs_clks, >> + .data = &k1_ccu_apbs_data, >> }, >> { >> .compatible = "spacemit,k1-syscon-mpmu", >> - .data = k1_ccu_mpmu_clks, >> + .data = &k1_ccu_mpmu_data, >> }, >> { >> .compatible = "spacemit,k1-syscon-apbc", >> - .data = k1_ccu_apbc_clks, >> + .data = &k1_ccu_apbc_data, >> }, >> { >> .compatible = "spacemit,k1-syscon-apmu", >> - .data = k1_ccu_apmu_clks, >> + .data = &k1_ccu_apmu_data, >> }, >> { } > { /* sentinel */ } >> }; >> -- >> 2.43.0 >> >
On 07:43 Sun 23 Mar , Alex Elder wrote: > On 3/22/25 10:50 AM, Yixun Lan wrote: > > Hi Alex: > > > > this patch change relate to clock only, so how about let's fold > > it into clk patches (which now has not been merged), so we make > > the code right at first place? cause some moving around and renaming > > No I don't want to do that. > > The clock patches are Haylen's and the are getting closer to > acceptance. Let's not confuse things by adding a bunch of new > functionality. Get those patches in, and mine can follow not > too long after that. > I only mean patch [2/7], not all patches, as it's still clock related but, either way fine by me if you insist
On 3/23/25 8:04 AM, Yixun Lan wrote: > On 07:43 Sun 23 Mar , Alex Elder wrote: >> On 3/22/25 10:50 AM, Yixun Lan wrote: >>> Hi Alex: >>> >>> this patch change relate to clock only, so how about let's fold >>> it into clk patches (which now has not been merged), so we make >>> the code right at first place? cause some moving around and renaming >> >> No I don't want to do that. >> >> The clock patches are Haylen's and the are getting closer to >> acceptance. Let's not confuse things by adding a bunch of new >> functionality. Get those patches in, and mine can follow not >> too long after that. >> > > I only mean patch [2/7], not all patches, as it's still clock related > but, either way fine by me if you insist I see. It would be great for Haylen to implement this, it's a better way to do it anyway--you can define the number of elements in the array using ARRAY_SIZE() this way also (rather than having to count them at runtime). Haylen is welcome to use my patch as the basis of this, but if it doesn't get into that code I'll add it. -Alex >
On Fri, Mar 21, 2025 at 10:18:25AM -0500, Alex Elder wrote: > Define a new structure type to be used for describing the OF match data. > Rather than using the array of spacemit_ccu_clk structures for match > data, we use this structure instead. > > Move the definition of the spacemit_ccu_clk structure closer to the top > of the source file, and add the new structure definition below it. > > Shorten the name of spacemit_ccu_register() to be k1_ccu_register(). I've read your conversation about moving parts of the patch into the clock series, I'm of course willing to :) > Signed-off-by: Alex Elder <elder@riscstar.com> > --- > drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++--------- > 1 file changed, 43 insertions(+), 15 deletions(-) > > diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c > index 44db48ae71313..f7367271396a0 100644 > --- a/drivers/clk/spacemit/ccu-k1.c > +++ b/drivers/clk/spacemit/ccu-k1.c > @@ -129,6 +129,15 @@ > #define APMU_EMAC0_CLK_RES_CTRL 0x3e4 > #define APMU_EMAC1_CLK_RES_CTRL 0x3ec > > +struct spacemit_ccu_clk { > + int id; > + struct clk_hw *hw; > +}; > + > +struct k1_ccu_data { > + struct spacemit_ccu_clk *clk; /* array with sentinel */ > +}; This is something like what I've dropped in v5 of the clock series so I doubt whether it should be added back in clock series again, as at that point there's no reason for an extra structure: Alex, is it okay for you to keep the change in reset series? ... > +static int k1_ccu_register(struct device *dev, struct regmap *regmap, > + struct regmap *lock_regmap, > + struct spacemit_ccu_clk *clks) > { > const struct spacemit_ccu_clk *clk; > int i, ret, max_id = 0; > @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev, > > clk_data->num = max_id + 1; > > - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); > + if (ret) > + dev_err(dev, "error %d adding clock hardware provider\n", ret); This error message definitely should go in the clock series. > + return ret; > } > static int k1_ccu_probe(struct platform_device *pdev) > { > struct regmap *base_regmap, *lock_regmap = NULL; > struct device *dev = &pdev->dev; > + const struct k1_ccu_data *data; > int ret; > > + data = of_device_get_match_data(dev); > + if (!data) > + return -EINVAL; Looking through the reset series, I don't see a reason that of_device_get_match_data() could return NULL. This is also something you've asked me to drop in v4 of the clock series, so I guess it isn't necessary. > base_regmap = device_node_to_regmap(dev->of_node); > if (IS_ERR(base_regmap)) > return dev_err_probe(dev, PTR_ERR(base_regmap), > @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev) > "failed to get lock regmap\n"); > } > > - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, > - of_device_get_match_data(dev)); > + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk); > if (ret) > return dev_err_probe(dev, ret, "failed to register clocks\n"); For using ARRAY_SIZE() to simplify runtime code, it's mostly okay since binding IDs are continuous 0-based integers. But I split the handling of TWSI8 into another patch, which creates a hole in the range and breaks the assumption. Do you think the TWSI8 commit should be merged back in the clock driver one? Best regards, Haylen Chu
On 3/24/25 6:53 AM, Haylen Chu wrote: > On Fri, Mar 21, 2025 at 10:18:25AM -0500, Alex Elder wrote: >> Define a new structure type to be used for describing the OF match data. >> Rather than using the array of spacemit_ccu_clk structures for match >> data, we use this structure instead. >> >> Move the definition of the spacemit_ccu_clk structure closer to the top >> of the source file, and add the new structure definition below it. >> >> Shorten the name of spacemit_ccu_register() to be k1_ccu_register(). > > I've read your conversation about moving parts of the patch into the > clock series, I'm of course willing to :) > >> Signed-off-by: Alex Elder <elder@riscstar.com> >> --- >> drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++--------- >> 1 file changed, 43 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c >> index 44db48ae71313..f7367271396a0 100644 >> --- a/drivers/clk/spacemit/ccu-k1.c >> +++ b/drivers/clk/spacemit/ccu-k1.c >> @@ -129,6 +129,15 @@ >> #define APMU_EMAC0_CLK_RES_CTRL 0x3e4 >> #define APMU_EMAC1_CLK_RES_CTRL 0x3ec >> >> +struct spacemit_ccu_clk { >> + int id; >> + struct clk_hw *hw; >> +}; >> + >> +struct k1_ccu_data { >> + struct spacemit_ccu_clk *clk; /* array with sentinel */ >> +}; > > This is something like what I've dropped in v5 of the clock series so I > doubt whether it should be added back in clock series again, as at that > point there's no reason for an extra structure: Alex, is it okay for you > to keep the change in reset series? That's perfectly fine with me. It's not necessary yet, so it's just fine for you to do things the way you did, and I'll add this in as part of the reset series. > ... > >> +static int k1_ccu_register(struct device *dev, struct regmap *regmap, >> + struct regmap *lock_regmap, >> + struct spacemit_ccu_clk *clks) >> { >> const struct spacemit_ccu_clk *clk; >> int i, ret, max_id = 0; >> @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev, >> >> clk_data->num = max_id + 1; >> >> - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); >> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); >> + if (ret) >> + dev_err(dev, "error %d adding clock hardware provider\n", ret); > > This error message definitely should go in the clock series. > >> + return ret; >> } > >> static int k1_ccu_probe(struct platform_device *pdev) >> { >> struct regmap *base_regmap, *lock_regmap = NULL; >> struct device *dev = &pdev->dev; >> + const struct k1_ccu_data *data; >> int ret; >> >> + data = of_device_get_match_data(dev); >> + if (!data) >> + return -EINVAL; > > Looking through the reset series, I don't see a reason that > of_device_get_match_data() could return NULL. This is also something > you've asked me to drop in v4 of the clock series, so I guess it isn't > necessary. You are correct. I'll drop it. I contemplated this and thought it's useful to tell the reader it's necessary to not be null, but you can tell it has to be by inspection. >> base_regmap = device_node_to_regmap(dev->of_node); >> if (IS_ERR(base_regmap)) >> return dev_err_probe(dev, PTR_ERR(base_regmap), >> @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev) >> "failed to get lock regmap\n"); >> } >> >> - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, >> - of_device_get_match_data(dev)); >> + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk); >> if (ret) >> return dev_err_probe(dev, ret, "failed to register clocks\n"); > > For using ARRAY_SIZE() to simplify runtime code, it's mostly okay since > binding IDs are continuous 0-based integers. But I split the handling of > TWSI8 into another patch, which creates a hole in the range and breaks > the assumption. Do you think the TWSI8 commit should be merged back in > the clock driver one? I didn't understand the reason why you separated the TWSI8 into a separate commit. Now I know. The hole in the range doesn't really matter much; you already initialize your ->hws[] array of pointers with ERR_PTR(-ENOENT), so any holes are handled properly. -Alex > > Best regards, > Haylen Chu
diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c index 44db48ae71313..f7367271396a0 100644 --- a/drivers/clk/spacemit/ccu-k1.c +++ b/drivers/clk/spacemit/ccu-k1.c @@ -129,6 +129,15 @@ #define APMU_EMAC0_CLK_RES_CTRL 0x3e4 #define APMU_EMAC1_CLK_RES_CTRL 0x3ec +struct spacemit_ccu_clk { + int id; + struct clk_hw *hw; +}; + +struct k1_ccu_data { + struct spacemit_ccu_clk *clk; /* array with sentinel */ +}; + /* APBS clocks start */ /* Frequency of pll{1,2} should not be updated at runtime */ @@ -1359,11 +1368,6 @@ static CCU_GATE_DEFINE(emmc_bus_clk, CCU_PARENT_HW(pmua_aclk), 0); /* APMU clocks end */ -struct spacemit_ccu_clk { - int id; - struct clk_hw *hw; -}; - static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = { { CLK_PLL1, &pll1.common.hw }, { CLK_PLL2, &pll2.common.hw }, @@ -1403,6 +1407,10 @@ static struct spacemit_ccu_clk k1_ccu_apbs_clks[] = { { 0, NULL }, }; +static const struct k1_ccu_data k1_ccu_apbs_data = { + .clk = k1_ccu_apbs_clks, +}; + static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = { { CLK_PLL1_307P2, &pll1_d8_307p2.common.hw }, { CLK_PLL1_76P8, &pll1_d32_76p8.common.hw }, @@ -1440,6 +1448,10 @@ static struct spacemit_ccu_clk k1_ccu_mpmu_clks[] = { { 0, NULL }, }; +static const struct k1_ccu_data k1_ccu_mpmu_data = { + .clk = k1_ccu_mpmu_clks, +}; + static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = { { CLK_UART0, &uart0_clk.common.hw }, { CLK_UART2, &uart2_clk.common.hw }, @@ -1544,6 +1556,10 @@ static struct spacemit_ccu_clk k1_ccu_apbc_clks[] = { { 0, NULL }, }; +static const struct k1_ccu_data k1_ccu_apbc_data = { + .clk = k1_ccu_apbc_clks, +}; + static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { { CLK_CCI550, &cci550_clk.common.hw }, { CLK_CPU_C0_HI, &cpu_c0_hi_clk.common.hw }, @@ -1610,9 +1626,13 @@ static struct spacemit_ccu_clk k1_ccu_apmu_clks[] = { { 0, NULL }, }; -static int spacemit_ccu_register(struct device *dev, - struct regmap *regmap, struct regmap *lock_regmap, - const struct spacemit_ccu_clk *clks) +static const struct k1_ccu_data k1_ccu_apmu_data = { + .clk = k1_ccu_apmu_clks, +}; + +static int k1_ccu_register(struct device *dev, struct regmap *regmap, + struct regmap *lock_regmap, + struct spacemit_ccu_clk *clks) { const struct spacemit_ccu_clk *clk; int i, ret, max_id = 0; @@ -1648,15 +1668,24 @@ static int spacemit_ccu_register(struct device *dev, clk_data->num = max_id + 1; - return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, clk_data); + if (ret) + dev_err(dev, "error %d adding clock hardware provider\n", ret); + + return ret; } static int k1_ccu_probe(struct platform_device *pdev) { struct regmap *base_regmap, *lock_regmap = NULL; struct device *dev = &pdev->dev; + const struct k1_ccu_data *data; int ret; + data = of_device_get_match_data(dev); + if (!data) + return -EINVAL; + base_regmap = device_node_to_regmap(dev->of_node); if (IS_ERR(base_regmap)) return dev_err_probe(dev, PTR_ERR(base_regmap), @@ -1677,8 +1706,7 @@ static int k1_ccu_probe(struct platform_device *pdev) "failed to get lock regmap\n"); } - ret = spacemit_ccu_register(dev, base_regmap, lock_regmap, - of_device_get_match_data(dev)); + ret = k1_ccu_register(dev, base_regmap, lock_regmap, data->clk); if (ret) return dev_err_probe(dev, ret, "failed to register clocks\n"); @@ -1688,19 +1716,19 @@ static int k1_ccu_probe(struct platform_device *pdev) static const struct of_device_id of_k1_ccu_match[] = { { .compatible = "spacemit,k1-pll", - .data = k1_ccu_apbs_clks, + .data = &k1_ccu_apbs_data, }, { .compatible = "spacemit,k1-syscon-mpmu", - .data = k1_ccu_mpmu_clks, + .data = &k1_ccu_mpmu_data, }, { .compatible = "spacemit,k1-syscon-apbc", - .data = k1_ccu_apbc_clks, + .data = &k1_ccu_apbc_data, }, { .compatible = "spacemit,k1-syscon-apmu", - .data = k1_ccu_apmu_clks, + .data = &k1_ccu_apmu_data, }, { } };
Define a new structure type to be used for describing the OF match data. Rather than using the array of spacemit_ccu_clk structures for match data, we use this structure instead. Move the definition of the spacemit_ccu_clk structure closer to the top of the source file, and add the new structure definition below it. Shorten the name of spacemit_ccu_register() to be k1_ccu_register(). Signed-off-by: Alex Elder <elder@riscstar.com> --- drivers/clk/spacemit/ccu-k1.c | 58 ++++++++++++++++++++++++++--------- 1 file changed, 43 insertions(+), 15 deletions(-)