From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stanimir Varbanov Subject: Re: [PATCH v2 2/6] clk: qcom: gdsc: Prepare common clk probe to register gdscs Date: Thu, 19 Mar 2015 12:46:48 +0200 Message-ID: <550AA918.3000604@mm-sol.com> References: <1426752145-4365-1-git-send-email-rnayak@codeaurora.org> <1426752145-4365-3-git-send-email-rnayak@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: Received: from ns.mm-sol.com ([37.157.136.199]:50334 "EHLO extserv.mm-sol.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751195AbbCSKq6 (ORCPT ); Thu, 19 Mar 2015 06:46:58 -0400 In-Reply-To: <1426752145-4365-3-git-send-email-rnayak@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Rajendra Nayak Cc: sboyd@codeaurora.org, mturquette@linaro.org, linux-arm-msm@vger.kernel.org, georgi.djakov@linaro.org, linux-arm-kernel@lists.infradead.org Hi Rajendra, Thanks for the patch! On 03/19/2015 10:02 AM, Rajendra Nayak wrote: > The common clk probe registers a clk provider and a reset controller. > Update it to register a genpd provider using the gdsc data provided > by each platform. > > Signed-off-by: Rajendra Nayak > --- > drivers/clk/qcom/common.c | 14 +++++++++++++- > drivers/clk/qcom/common.h | 2 ++ > drivers/clk/qcom/gdsc.c | 34 +++++++++++++++++++++++++++++++++- > drivers/clk/qcom/gdsc.h | 9 ++++++++- > 4 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index a946b48..cc9f56f 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -21,6 +21,7 @@ > #include "clk-rcg.h" > #include "clk-regmap.h" > #include "reset.h" > +#include "gdsc.h" > > struct qcom_cc { > struct qcom_reset_controller reset; > @@ -125,8 +126,18 @@ int qcom_cc_really_probe(struct platform_device *pdev, > > ret = reset_controller_register(&reset->rcdev); > if (ret) > - of_clk_del_provider(dev->of_node); > + goto err_reset; > > + ret = gdsc_register(dev, desc->gdscs, desc->num_gdscs, regmap); > + if (ret) > + goto err_pd; > + > + return 0; > +err_pd: > + dev_err(dev, "Failed to register power domains\n"); > + reset_controller_unregister(&reset->rcdev); > +err_reset: > + of_clk_del_provider(dev->of_node); > return ret; > } > EXPORT_SYMBOL_GPL(qcom_cc_really_probe); > @@ -145,6 +156,7 @@ EXPORT_SYMBOL_GPL(qcom_cc_probe); > > void qcom_cc_remove(struct platform_device *pdev) > { > + of_genpd_del_provider(pdev->dev.of_node); It would be nice to introduce gdsc_unregister() for symmetry. > of_clk_del_provider(pdev->dev.of_node); > reset_controller_unregister(platform_get_drvdata(pdev)); > } > + > +int gdsc_register(struct device *dev, struct gdsc **scs, size_t num, > + struct regmap *regmap) > +{ Could you squash implementation of this function with the first patch 1/6. > + int i, ret; > + struct genpd_onecell_data *data; > + > + if (!num || !scs || !dev || !dev->of_node) > + return 0; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->domains = devm_kzalloc(dev, sizeof(*data->domains) * num, > + GFP_KERNEL); Just wondering, are there some obstacles to embed struct genpd_onecell_data in struct gdsc, and thus avoid having two memory allocations? > + if (!data->domains) > + return -ENOMEM; > + > + data->num_domains = num; > + for (i = 0; i < num; i++) { > + if (!scs[i]) > + continue; > + scs[i]->regmap = regmap; > + ret = gdsc_init(scs[i]); > + if (ret) > + return ret; > + data->domains[i] = &scs[i]->pd; > + } > + return of_genpd_add_provider_onecell(dev->of_node, data); > +} > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h > index ac6a2d5..14de304 100644 > --- a/drivers/clk/qcom/gdsc.h > +++ b/drivers/clk/qcom/gdsc.h > @@ -32,5 +32,12 @@ struct gdsc { > > #define domain_to_gdsc(domain) container_of(domain, struct gdsc, pd) This is used only from gdsc.c, please move it there. -- regards, Stan From mboxrd@z Thu Jan 1 00:00:00 1970 From: svarbanov@mm-sol.com (Stanimir Varbanov) Date: Thu, 19 Mar 2015 12:46:48 +0200 Subject: [PATCH v2 2/6] clk: qcom: gdsc: Prepare common clk probe to register gdscs In-Reply-To: <1426752145-4365-3-git-send-email-rnayak@codeaurora.org> References: <1426752145-4365-1-git-send-email-rnayak@codeaurora.org> <1426752145-4365-3-git-send-email-rnayak@codeaurora.org> Message-ID: <550AA918.3000604@mm-sol.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Rajendra, Thanks for the patch! On 03/19/2015 10:02 AM, Rajendra Nayak wrote: > The common clk probe registers a clk provider and a reset controller. > Update it to register a genpd provider using the gdsc data provided > by each platform. > > Signed-off-by: Rajendra Nayak > --- > drivers/clk/qcom/common.c | 14 +++++++++++++- > drivers/clk/qcom/common.h | 2 ++ > drivers/clk/qcom/gdsc.c | 34 +++++++++++++++++++++++++++++++++- > drivers/clk/qcom/gdsc.h | 9 ++++++++- > 4 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c > index a946b48..cc9f56f 100644 > --- a/drivers/clk/qcom/common.c > +++ b/drivers/clk/qcom/common.c > @@ -21,6 +21,7 @@ > #include "clk-rcg.h" > #include "clk-regmap.h" > #include "reset.h" > +#include "gdsc.h" > > struct qcom_cc { > struct qcom_reset_controller reset; > @@ -125,8 +126,18 @@ int qcom_cc_really_probe(struct platform_device *pdev, > > ret = reset_controller_register(&reset->rcdev); > if (ret) > - of_clk_del_provider(dev->of_node); > + goto err_reset; > > + ret = gdsc_register(dev, desc->gdscs, desc->num_gdscs, regmap); > + if (ret) > + goto err_pd; > + > + return 0; > +err_pd: > + dev_err(dev, "Failed to register power domains\n"); > + reset_controller_unregister(&reset->rcdev); > +err_reset: > + of_clk_del_provider(dev->of_node); > return ret; > } > EXPORT_SYMBOL_GPL(qcom_cc_really_probe); > @@ -145,6 +156,7 @@ EXPORT_SYMBOL_GPL(qcom_cc_probe); > > void qcom_cc_remove(struct platform_device *pdev) > { > + of_genpd_del_provider(pdev->dev.of_node); It would be nice to introduce gdsc_unregister() for symmetry. > of_clk_del_provider(pdev->dev.of_node); > reset_controller_unregister(platform_get_drvdata(pdev)); > } > + > +int gdsc_register(struct device *dev, struct gdsc **scs, size_t num, > + struct regmap *regmap) > +{ Could you squash implementation of this function with the first patch 1/6. > + int i, ret; > + struct genpd_onecell_data *data; > + > + if (!num || !scs || !dev || !dev->of_node) > + return 0; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->domains = devm_kzalloc(dev, sizeof(*data->domains) * num, > + GFP_KERNEL); Just wondering, are there some obstacles to embed struct genpd_onecell_data in struct gdsc, and thus avoid having two memory allocations? > + if (!data->domains) > + return -ENOMEM; > + > + data->num_domains = num; > + for (i = 0; i < num; i++) { > + if (!scs[i]) > + continue; > + scs[i]->regmap = regmap; > + ret = gdsc_init(scs[i]); > + if (ret) > + return ret; > + data->domains[i] = &scs[i]->pd; > + } > + return of_genpd_add_provider_onecell(dev->of_node, data); > +} > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h > index ac6a2d5..14de304 100644 > --- a/drivers/clk/qcom/gdsc.h > +++ b/drivers/clk/qcom/gdsc.h > @@ -32,5 +32,12 @@ struct gdsc { > > #define domain_to_gdsc(domain) container_of(domain, struct gdsc, pd) This is used only from gdsc.c, please move it there. -- regards, Stan