From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tomeu Vizoso Subject: Re: [RFC v2 4/5] clk: per-user clock accounting for debug Date: Thu, 10 Jul 2014 12:51:56 +0200 Message-ID: <53BE704C.8080502@collabora.com> References: <1404398323-18934-1-git-send-email-tomeu.vizoso@collabora.com> <1404398323-18934-5-git-send-email-tomeu.vizoso@collabora.com> <53BD9FA3.1040204@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7BIT Return-path: In-Reply-To: <53BD9FA3.1040204-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Tomasz Figa , Stephen Warren , Thierry Reding , Mike Turquette , rabin-66gdRtMMWGc@public.gmane.org, linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org Cc: Rabin Vincent , Javier Martinez Canillas List-Id: linux-tegra@vger.kernel.org On 07/09/2014 10:01 PM, Tomasz Figa wrote: > Hi Tomeu, Rabin, > > Please see my comments inline. > > On 03.07.2014 16:38, Tomeu Vizoso wrote: >> From: Rabin Vincent >> >> When a clock has multiple users, the WARNING on imbalance of >> enable/disable may not show the guilty party since although they may >> have commited the error earlier, the warning is emitted later when some >> other user, presumably innocent, disables the clock. > > [snip] > >> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c >> index 5f2aba9..d6a5e59 100644 >> --- a/drivers/clk/clk-devres.c >> +++ b/drivers/clk/clk-devres.c >> @@ -37,7 +37,8 @@ EXPORT_SYMBOL(devm_clk_provider_get); >> >> struct clk *devm_clk_get(struct device *dev, const char *id) >> { >> - return clk_core_to_clk(devm_clk_provider_get(dev, id)); >> + const char *dev_id = dev ? dev_name(dev) : NULL; >> + return clk_core_to_clk(devm_clk_provider_get(dev, id), dev_id, id); > > Hmm, correct me if I'm missing something but you're just calling > devm_clk_provider() which guarantees calling of clk_provider_put() on > device removal, but what about the consumer struct being created here? > > Shouldn't you rather call normal clk_provider_get() here, then create a > consumer struct for it and then wrap it up using devres, providing > appropriate release function? You are totally right. >> } >> EXPORT_SYMBOL(devm_clk_get); >> >> diff --git a/drivers/clk/clk-highbank.c b/drivers/clk/clk-highbank.c >> index cc1bae1..4e38856 100644 >> --- a/drivers/clk/clk-highbank.c >> +++ b/drivers/clk/clk-highbank.c >> @@ -331,7 +331,7 @@ CLK_OF_DECLARE(hb_a9periph, "calxeda,hb-a9periph-clock", hb_a9periph_init); >> static void __init hb_a9bus_init(struct device_node *node) >> { >> struct clk_core *clk = hb_clk_init(node, &a9bclk_ops); >> - clk_prepare_enable(clk_core_to_clk(clk)); >> + clk_prepare_enable(clk_core_to_clk(clk, node->full_name, NULL)); >> } >> CLK_OF_DECLARE(hb_a9bus, "calxeda,hb-a9bus-clock", hb_a9bus_init); >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 644a824..b887c69 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -568,6 +568,28 @@ static int clk_disable_unused(void) >> } >> late_initcall_sync(clk_disable_unused); >> >> +struct clk *clk_core_to_clk(struct clk_core *clk_core, const char *dev, >> + const char *con) > > Name of this function suggests that this is just a simple conversion, > while it creates new object. I would suggest calling this > clk_create_consumer() or something along these lines. Agreed, that sounds better. >> +{ >> + struct clk *clk; >> + >> + clk = kzalloc(sizeof(*clk), GFP_KERNEL); >> + if (!clk) >> + return ERR_PTR(-ENOMEM); > > [snip] > >> +static void clk_free_clk(struct clk *clk) >> +{ >> + kfree(clk); >> +} > > Any need for this one-liner? Seems to be used just once below. Yeah, should probably remove it from this patch and only reintroduce it later when it's actually needed. >> + >> void clk_put(struct clk *clk) >> { >> - __clk_put(clk_to_clk_core(clk)); >> + clk_core_t *core = clk_to_clk_core(clk); >> + >> + clk_free_clk(clk); >> + __clk_put(core); >> } >> EXPORT_SYMBOL(clk_put); >> >> diff --git a/drivers/clk/qcom/mmcc-msm8960.c b/drivers/clk/qcom/mmcc-msm8960.c >> index 28a5d30..47f21ea 100644 >> --- a/drivers/clk/qcom/mmcc-msm8960.c >> +++ b/drivers/clk/qcom/mmcc-msm8960.c >> @@ -470,7 +470,8 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index) >> * needs to be on at what time. >> */ >> for (i = 0; i < num_parents; i++) { >> - ret = clk_prepare_enable(clk_core_to_clk(clk_get_parent_by_index(clk, i))); >> + struct clk_core *parent = clk_get_parent_by_index(clk, i); >> + ret = clk_prepare_enable(clk_core_to_clk(parent, NULL, __clk_get_name(parent))); > > Shouldn't this use clk_provider_*() instead? IMHO clk_core_to_clk() and > clk_to_clk_core() shouldn't be available to clock drivers, just the core > clock code. Consumers should operate on struct clks alone, clock drivers > should operate only on struct clk_cores and only the core code should be > responsible for necessary conversions. Totally, this is already fixed in my local version. >> if (ret) >> goto err; >> } >> @@ -498,8 +499,10 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index) >> udelay(1); >> >> err: >> - for (i--; i >= 0; i--) >> - clk_disable_unprepare(clk_core_to_clk(clk_get_parent_by_index(clk, i))); >> + for (i--; i >= 0; i--) { >> + struct clk_core *parent = clk_get_parent_by_index(clk, i); >> + clk_disable_unprepare(clk_core_to_clk(parent, NULL, __clk_get_name(parent))); > > Hmm. Memory leak? Every time this operation is called you create a > consumer struct for each parent two times. Yes, fortunately this won't be needed as we'll have clk_provider_disable_unprepare and friends. >> + } >> >> return ret; >> } >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index 5de44c1..550a691 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -1196,7 +1196,7 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks) >> struct clk_core *clk = clk_provider_get(NULL, clocks[i]); >> >> if (!IS_ERR(clk)) >> - clk_prepare_enable(clk_core_to_clk(clk)); >> + clk_prepare_enable(clk_core_to_clk(clk, NULL, clocks[i])); > > Probably should use clk_provider_*()? > >> } >> } >> >> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c >> index 385d101..2a03d47 100644 >> --- a/drivers/clk/tegra/clk.c >> +++ b/drivers/clk/tegra/clk.c >> @@ -228,7 +228,7 @@ void __init tegra_init_from_table(struct tegra_clk_init_table *tbl, >> } >> >> if (tbl->state) >> - if (clk_prepare_enable(clk_core_to_clk(clk))) { >> + if (clk_prepare_enable(clk_core_to_clk(clk, NULL, __clk_get_name(clk)))) { > > Ditto. And all similar cases below which I skipped due to lack of any > added value to this review. > >> pr_err("%s: Failed to enable %s\n", __func__, >> __clk_get_name(clk)); >> WARN_ON(1); > > [snip] > >> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h >> index 2c1ece9..9657fc8 100644 >> --- a/include/linux/clk-private.h >> +++ b/include/linux/clk-private.h >> @@ -57,6 +57,10 @@ struct clk_core { >> >> struct clk { >> struct clk_core *core; >> + unsigned int enable_count; >> + const char *dev_id; >> + const char *con_id; >> + void *last_disable; > > Probably the same as for enables should be done for prepares. Yes, I think it would be useful as well. >> }; >> >> /* >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index 87de32c..abfc31a 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h >> @@ -533,7 +533,8 @@ struct clk_core *clk_provider_get_sys(const char *dev_id, const char *con_id); >> struct clk_core *clk_provider_get(struct device *dev, const char *con_id); >> struct clk_core *devm_clk_provider_get(struct device *dev, const char *id); >> >> -#define clk_core_to_clk(core) ((struct clk *)(core)) > > Btw. this should have been a static inline. Agreed. Thanks, Tomeu > Best regards, > Tomasz > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751838AbaGJKwG (ORCPT ); Thu, 10 Jul 2014 06:52:06 -0400 Received: from bhuna.collabora.co.uk ([93.93.135.160]:42104 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751492AbaGJKwD (ORCPT ); Thu, 10 Jul 2014 06:52:03 -0400 Message-ID: <53BE704C.8080502@collabora.com> Date: Thu, 10 Jul 2014 12:51:56 +0200 From: Tomeu Vizoso User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Tomasz Figa , Stephen Warren , Thierry Reding , Mike Turquette , rabin@rab.in, linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org CC: Rabin Vincent , Javier Martinez Canillas Subject: Re: [RFC v2 4/5] clk: per-user clock accounting for debug References: <1404398323-18934-1-git-send-email-tomeu.vizoso@collabora.com> <1404398323-18934-5-git-send-email-tomeu.vizoso@collabora.com> <53BD9FA3.1040204@gmail.com> In-Reply-To: <53BD9FA3.1040204@gmail.com> Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/09/2014 10:01 PM, Tomasz Figa wrote: > Hi Tomeu, Rabin, > > Please see my comments inline. > > On 03.07.2014 16:38, Tomeu Vizoso wrote: >> From: Rabin Vincent >> >> When a clock has multiple users, the WARNING on imbalance of >> enable/disable may not show the guilty party since although they may >> have commited the error earlier, the warning is emitted later when some >> other user, presumably innocent, disables the clock. > > [snip] > >> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c >> index 5f2aba9..d6a5e59 100644 >> --- a/drivers/clk/clk-devres.c >> +++ b/drivers/clk/clk-devres.c >> @@ -37,7 +37,8 @@ EXPORT_SYMBOL(devm_clk_provider_get); >> >> struct clk *devm_clk_get(struct device *dev, const char *id) >> { >> - return clk_core_to_clk(devm_clk_provider_get(dev, id)); >> + const char *dev_id = dev ? dev_name(dev) : NULL; >> + return clk_core_to_clk(devm_clk_provider_get(dev, id), dev_id, id); > > Hmm, correct me if I'm missing something but you're just calling > devm_clk_provider() which guarantees calling of clk_provider_put() on > device removal, but what about the consumer struct being created here? > > Shouldn't you rather call normal clk_provider_get() here, then create a > consumer struct for it and then wrap it up using devres, providing > appropriate release function? You are totally right. >> } >> EXPORT_SYMBOL(devm_clk_get); >> >> diff --git a/drivers/clk/clk-highbank.c b/drivers/clk/clk-highbank.c >> index cc1bae1..4e38856 100644 >> --- a/drivers/clk/clk-highbank.c >> +++ b/drivers/clk/clk-highbank.c >> @@ -331,7 +331,7 @@ CLK_OF_DECLARE(hb_a9periph, "calxeda,hb-a9periph-clock", hb_a9periph_init); >> static void __init hb_a9bus_init(struct device_node *node) >> { >> struct clk_core *clk = hb_clk_init(node, &a9bclk_ops); >> - clk_prepare_enable(clk_core_to_clk(clk)); >> + clk_prepare_enable(clk_core_to_clk(clk, node->full_name, NULL)); >> } >> CLK_OF_DECLARE(hb_a9bus, "calxeda,hb-a9bus-clock", hb_a9bus_init); >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 644a824..b887c69 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -568,6 +568,28 @@ static int clk_disable_unused(void) >> } >> late_initcall_sync(clk_disable_unused); >> >> +struct clk *clk_core_to_clk(struct clk_core *clk_core, const char *dev, >> + const char *con) > > Name of this function suggests that this is just a simple conversion, > while it creates new object. I would suggest calling this > clk_create_consumer() or something along these lines. Agreed, that sounds better. >> +{ >> + struct clk *clk; >> + >> + clk = kzalloc(sizeof(*clk), GFP_KERNEL); >> + if (!clk) >> + return ERR_PTR(-ENOMEM); > > [snip] > >> +static void clk_free_clk(struct clk *clk) >> +{ >> + kfree(clk); >> +} > > Any need for this one-liner? Seems to be used just once below. Yeah, should probably remove it from this patch and only reintroduce it later when it's actually needed. >> + >> void clk_put(struct clk *clk) >> { >> - __clk_put(clk_to_clk_core(clk)); >> + clk_core_t *core = clk_to_clk_core(clk); >> + >> + clk_free_clk(clk); >> + __clk_put(core); >> } >> EXPORT_SYMBOL(clk_put); >> >> diff --git a/drivers/clk/qcom/mmcc-msm8960.c b/drivers/clk/qcom/mmcc-msm8960.c >> index 28a5d30..47f21ea 100644 >> --- a/drivers/clk/qcom/mmcc-msm8960.c >> +++ b/drivers/clk/qcom/mmcc-msm8960.c >> @@ -470,7 +470,8 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index) >> * needs to be on at what time. >> */ >> for (i = 0; i < num_parents; i++) { >> - ret = clk_prepare_enable(clk_core_to_clk(clk_get_parent_by_index(clk, i))); >> + struct clk_core *parent = clk_get_parent_by_index(clk, i); >> + ret = clk_prepare_enable(clk_core_to_clk(parent, NULL, __clk_get_name(parent))); > > Shouldn't this use clk_provider_*() instead? IMHO clk_core_to_clk() and > clk_to_clk_core() shouldn't be available to clock drivers, just the core > clock code. Consumers should operate on struct clks alone, clock drivers > should operate only on struct clk_cores and only the core code should be > responsible for necessary conversions. Totally, this is already fixed in my local version. >> if (ret) >> goto err; >> } >> @@ -498,8 +499,10 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index) >> udelay(1); >> >> err: >> - for (i--; i >= 0; i--) >> - clk_disable_unprepare(clk_core_to_clk(clk_get_parent_by_index(clk, i))); >> + for (i--; i >= 0; i--) { >> + struct clk_core *parent = clk_get_parent_by_index(clk, i); >> + clk_disable_unprepare(clk_core_to_clk(parent, NULL, __clk_get_name(parent))); > > Hmm. Memory leak? Every time this operation is called you create a > consumer struct for each parent two times. Yes, fortunately this won't be needed as we'll have clk_provider_disable_unprepare and friends. >> + } >> >> return ret; >> } >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index 5de44c1..550a691 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -1196,7 +1196,7 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks) >> struct clk_core *clk = clk_provider_get(NULL, clocks[i]); >> >> if (!IS_ERR(clk)) >> - clk_prepare_enable(clk_core_to_clk(clk)); >> + clk_prepare_enable(clk_core_to_clk(clk, NULL, clocks[i])); > > Probably should use clk_provider_*()? > >> } >> } >> >> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c >> index 385d101..2a03d47 100644 >> --- a/drivers/clk/tegra/clk.c >> +++ b/drivers/clk/tegra/clk.c >> @@ -228,7 +228,7 @@ void __init tegra_init_from_table(struct tegra_clk_init_table *tbl, >> } >> >> if (tbl->state) >> - if (clk_prepare_enable(clk_core_to_clk(clk))) { >> + if (clk_prepare_enable(clk_core_to_clk(clk, NULL, __clk_get_name(clk)))) { > > Ditto. And all similar cases below which I skipped due to lack of any > added value to this review. > >> pr_err("%s: Failed to enable %s\n", __func__, >> __clk_get_name(clk)); >> WARN_ON(1); > > [snip] > >> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h >> index 2c1ece9..9657fc8 100644 >> --- a/include/linux/clk-private.h >> +++ b/include/linux/clk-private.h >> @@ -57,6 +57,10 @@ struct clk_core { >> >> struct clk { >> struct clk_core *core; >> + unsigned int enable_count; >> + const char *dev_id; >> + const char *con_id; >> + void *last_disable; > > Probably the same as for enables should be done for prepares. Yes, I think it would be useful as well. >> }; >> >> /* >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index 87de32c..abfc31a 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h >> @@ -533,7 +533,8 @@ struct clk_core *clk_provider_get_sys(const char *dev_id, const char *con_id); >> struct clk_core *clk_provider_get(struct device *dev, const char *con_id); >> struct clk_core *devm_clk_provider_get(struct device *dev, const char *id); >> >> -#define clk_core_to_clk(core) ((struct clk *)(core)) > > Btw. this should have been a static inline. Agreed. Thanks, Tomeu > Best regards, > Tomasz > From mboxrd@z Thu Jan 1 00:00:00 1970 From: tomeu.vizoso@collabora.com (Tomeu Vizoso) Date: Thu, 10 Jul 2014 12:51:56 +0200 Subject: [RFC v2 4/5] clk: per-user clock accounting for debug In-Reply-To: <53BD9FA3.1040204@gmail.com> References: <1404398323-18934-1-git-send-email-tomeu.vizoso@collabora.com> <1404398323-18934-5-git-send-email-tomeu.vizoso@collabora.com> <53BD9FA3.1040204@gmail.com> Message-ID: <53BE704C.8080502@collabora.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 07/09/2014 10:01 PM, Tomasz Figa wrote: > Hi Tomeu, Rabin, > > Please see my comments inline. > > On 03.07.2014 16:38, Tomeu Vizoso wrote: >> From: Rabin Vincent >> >> When a clock has multiple users, the WARNING on imbalance of >> enable/disable may not show the guilty party since although they may >> have commited the error earlier, the warning is emitted later when some >> other user, presumably innocent, disables the clock. > > [snip] > >> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c >> index 5f2aba9..d6a5e59 100644 >> --- a/drivers/clk/clk-devres.c >> +++ b/drivers/clk/clk-devres.c >> @@ -37,7 +37,8 @@ EXPORT_SYMBOL(devm_clk_provider_get); >> >> struct clk *devm_clk_get(struct device *dev, const char *id) >> { >> - return clk_core_to_clk(devm_clk_provider_get(dev, id)); >> + const char *dev_id = dev ? dev_name(dev) : NULL; >> + return clk_core_to_clk(devm_clk_provider_get(dev, id), dev_id, id); > > Hmm, correct me if I'm missing something but you're just calling > devm_clk_provider() which guarantees calling of clk_provider_put() on > device removal, but what about the consumer struct being created here? > > Shouldn't you rather call normal clk_provider_get() here, then create a > consumer struct for it and then wrap it up using devres, providing > appropriate release function? You are totally right. >> } >> EXPORT_SYMBOL(devm_clk_get); >> >> diff --git a/drivers/clk/clk-highbank.c b/drivers/clk/clk-highbank.c >> index cc1bae1..4e38856 100644 >> --- a/drivers/clk/clk-highbank.c >> +++ b/drivers/clk/clk-highbank.c >> @@ -331,7 +331,7 @@ CLK_OF_DECLARE(hb_a9periph, "calxeda,hb-a9periph-clock", hb_a9periph_init); >> static void __init hb_a9bus_init(struct device_node *node) >> { >> struct clk_core *clk = hb_clk_init(node, &a9bclk_ops); >> - clk_prepare_enable(clk_core_to_clk(clk)); >> + clk_prepare_enable(clk_core_to_clk(clk, node->full_name, NULL)); >> } >> CLK_OF_DECLARE(hb_a9bus, "calxeda,hb-a9bus-clock", hb_a9bus_init); >> >> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c >> index 644a824..b887c69 100644 >> --- a/drivers/clk/clk.c >> +++ b/drivers/clk/clk.c >> @@ -568,6 +568,28 @@ static int clk_disable_unused(void) >> } >> late_initcall_sync(clk_disable_unused); >> >> +struct clk *clk_core_to_clk(struct clk_core *clk_core, const char *dev, >> + const char *con) > > Name of this function suggests that this is just a simple conversion, > while it creates new object. I would suggest calling this > clk_create_consumer() or something along these lines. Agreed, that sounds better. >> +{ >> + struct clk *clk; >> + >> + clk = kzalloc(sizeof(*clk), GFP_KERNEL); >> + if (!clk) >> + return ERR_PTR(-ENOMEM); > > [snip] > >> +static void clk_free_clk(struct clk *clk) >> +{ >> + kfree(clk); >> +} > > Any need for this one-liner? Seems to be used just once below. Yeah, should probably remove it from this patch and only reintroduce it later when it's actually needed. >> + >> void clk_put(struct clk *clk) >> { >> - __clk_put(clk_to_clk_core(clk)); >> + clk_core_t *core = clk_to_clk_core(clk); >> + >> + clk_free_clk(clk); >> + __clk_put(core); >> } >> EXPORT_SYMBOL(clk_put); >> >> diff --git a/drivers/clk/qcom/mmcc-msm8960.c b/drivers/clk/qcom/mmcc-msm8960.c >> index 28a5d30..47f21ea 100644 >> --- a/drivers/clk/qcom/mmcc-msm8960.c >> +++ b/drivers/clk/qcom/mmcc-msm8960.c >> @@ -470,7 +470,8 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index) >> * needs to be on at what time. >> */ >> for (i = 0; i < num_parents; i++) { >> - ret = clk_prepare_enable(clk_core_to_clk(clk_get_parent_by_index(clk, i))); >> + struct clk_core *parent = clk_get_parent_by_index(clk, i); >> + ret = clk_prepare_enable(clk_core_to_clk(parent, NULL, __clk_get_name(parent))); > > Shouldn't this use clk_provider_*() instead? IMHO clk_core_to_clk() and > clk_to_clk_core() shouldn't be available to clock drivers, just the core > clock code. Consumers should operate on struct clks alone, clock drivers > should operate only on struct clk_cores and only the core code should be > responsible for necessary conversions. Totally, this is already fixed in my local version. >> if (ret) >> goto err; >> } >> @@ -498,8 +499,10 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index) >> udelay(1); >> >> err: >> - for (i--; i >= 0; i--) >> - clk_disable_unprepare(clk_core_to_clk(clk_get_parent_by_index(clk, i))); >> + for (i--; i >= 0; i--) { >> + struct clk_core *parent = clk_get_parent_by_index(clk, i); >> + clk_disable_unprepare(clk_core_to_clk(parent, NULL, __clk_get_name(parent))); > > Hmm. Memory leak? Every time this operation is called you create a > consumer struct for each parent two times. Yes, fortunately this won't be needed as we'll have clk_provider_disable_unprepare and friends. >> + } >> >> return ret; >> } >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index 5de44c1..550a691 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -1196,7 +1196,7 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks) >> struct clk_core *clk = clk_provider_get(NULL, clocks[i]); >> >> if (!IS_ERR(clk)) >> - clk_prepare_enable(clk_core_to_clk(clk)); >> + clk_prepare_enable(clk_core_to_clk(clk, NULL, clocks[i])); > > Probably should use clk_provider_*()? > >> } >> } >> >> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c >> index 385d101..2a03d47 100644 >> --- a/drivers/clk/tegra/clk.c >> +++ b/drivers/clk/tegra/clk.c >> @@ -228,7 +228,7 @@ void __init tegra_init_from_table(struct tegra_clk_init_table *tbl, >> } >> >> if (tbl->state) >> - if (clk_prepare_enable(clk_core_to_clk(clk))) { >> + if (clk_prepare_enable(clk_core_to_clk(clk, NULL, __clk_get_name(clk)))) { > > Ditto. And all similar cases below which I skipped due to lack of any > added value to this review. > >> pr_err("%s: Failed to enable %s\n", __func__, >> __clk_get_name(clk)); >> WARN_ON(1); > > [snip] > >> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h >> index 2c1ece9..9657fc8 100644 >> --- a/include/linux/clk-private.h >> +++ b/include/linux/clk-private.h >> @@ -57,6 +57,10 @@ struct clk_core { >> >> struct clk { >> struct clk_core *core; >> + unsigned int enable_count; >> + const char *dev_id; >> + const char *con_id; >> + void *last_disable; > > Probably the same as for enables should be done for prepares. Yes, I think it would be useful as well. >> }; >> >> /* >> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h >> index 87de32c..abfc31a 100644 >> --- a/include/linux/clk-provider.h >> +++ b/include/linux/clk-provider.h >> @@ -533,7 +533,8 @@ struct clk_core *clk_provider_get_sys(const char *dev_id, const char *con_id); >> struct clk_core *clk_provider_get(struct device *dev, const char *con_id); >> struct clk_core *devm_clk_provider_get(struct device *dev, const char *id); >> >> -#define clk_core_to_clk(core) ((struct clk *)(core)) > > Btw. this should have been a static inline. Agreed. Thanks, Tomeu > Best regards, > Tomasz >