From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752537AbdDKRBe (ORCPT ); Tue, 11 Apr 2017 13:01:34 -0400 Received: from mail-pg0-f68.google.com ([74.125.83.68]:33458 "EHLO mail-pg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750721AbdDKRBc (ORCPT ); Tue, 11 Apr 2017 13:01:32 -0400 Subject: Re: [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories To: Dong Aisheng , linux-clk@vger.kernel.org References: <1491969809-20154-1-git-send-email-aisheng.dong@nxp.com> Cc: leonard.crestez@nxp.com, ping.bai@nxp.com, anson.huang@nxp.com, viresh.kumar@linaro.org, mturquette@baylibre.com, rjw@rjwysocki.net, linux-kernel@vger.kernel.org, broonie@kernel.org, octavian.purdila@nxp.com, kernel@pengutronix.de, fabio.estevam@nxp.com, yibin.gong@nxp.com, shawnguo@kernel.org, sboyd@codeaurora.org, linux-arm-kernel@lists.infradead.org From: Florian Fainelli Message-ID: <86200633-8349-f1c3-f4d8-a130a26381f7@gmail.com> Date: Tue, 11 Apr 2017 10:01:29 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1491969809-20154-1-git-send-email-aisheng.dong@nxp.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/11/2017 09:03 PM, Dong Aisheng wrote: > These helper function allows drivers to get several clk consumers in > one operation. If any of the clk cannot be acquired then any clks > that were got will be put before returning to the caller. > > The idea firstly came out when i wanted to clean up and optimize > imx6q-cpufreq driver which needs to handle a lot of clocks as it > becomes a bit mess. > > e.g. drivers/cpufreq/imx6q-cpufreq.c > > You will see we need get 7 clocks during driver probe. > (Add there will be more, e.g. pll1, pll1_bypass, pll1_bypass_src, > in the future when adding busfreq support). > > static int imx6q_cpufreq_probe(struct platform_device *pdev) > { > .... > arm_clk = clk_get(cpu_dev, "arm"); > pll1_sys_clk = clk_get(cpu_dev, "pll1_sys"); > pll1_sw_clk = clk_get(cpu_dev, "pll1_sw"); > step_clk = clk_get(cpu_dev, "step"); > pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m"); > if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) || > IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) { > dev_err(cpu_dev, "failed to get clocks\n"); > ret = -ENOENT; > goto put_clk; > } > > if (of_machine_is_compatible("fsl,imx6ul")) { > pll2_bus_clk = clk_get(cpu_dev, "pll2_bus"); > secondary_sel_clk = clk_get(cpu_dev, "secondary_sel"); > if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) { > dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n"); > ret = -ENOENT; > goto put_clk; > } > } > .... > put_clk: > if (!IS_ERR(arm_clk)) > clk_put(arm_clk); > if (!IS_ERR(pll1_sys_clk)) > clk_put(pll1_sys_clk); > ........... > } > > Together with the err path handling for each clocks, it does make > things a bit ugly. > > Since we already have regulator_bulk_get accessories, i thought we > probably could introduce clk_bulk_get as well to handle such case to > ease the driver owners' life. > > Besides IMX cpufreq driver, there is also some similar cases > in kernel which could befinit from this api as well. > e.g. > drivers/cpufreq/tegra124-cpufreq.c > drivers/cpufreq/s3c2412-cpufreq.c > sound/soc/samsung/smdk_spdif.c > arch/arm/mach-omap1/serial.c > ... > > And actually, if we handle clocks more than 3, then it might be > worthy to try, which there is quite many manay in kernel and > that probably could save a lot codes. > > This is a RFC patch intending to bring up the idea to discuss. > > Comments are welcome and appreciated! Thanks a lot for doing this, we have historically done something similar on ARCH_BRCMSTB platforms in our downstream tree with a concept of a "software" clock which has multiple parents (yikes), using the bulk accessors would essentially allow us to remove part of this hack, so I am all in favor of this! > > Dong Aisheng (3): > clk: add clk_bulk_get accessories > clk: add managed version of clk_bulk_get > cpufreq: imx6q: refine clk operations > > drivers/clk/clk-devres.c | 36 +++++++++++ > drivers/clk/clk.c | 99 +++++++++++++++++++++++++++++ > drivers/clk/clkdev.c | 42 +++++++++++++ > drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++------------------- > include/linux/clk.h | 135 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 365 insertions(+), 66 deletions(-) > -- Florian From mboxrd@z Thu Jan 1 00:00:00 1970 From: f.fainelli@gmail.com (Florian Fainelli) Date: Tue, 11 Apr 2017 10:01:29 -0700 Subject: [RFC PATCH 0/3] clk: introduce clk_bulk_get accessories In-Reply-To: <1491969809-20154-1-git-send-email-aisheng.dong@nxp.com> References: <1491969809-20154-1-git-send-email-aisheng.dong@nxp.com> Message-ID: <86200633-8349-f1c3-f4d8-a130a26381f7@gmail.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 04/11/2017 09:03 PM, Dong Aisheng wrote: > These helper function allows drivers to get several clk consumers in > one operation. If any of the clk cannot be acquired then any clks > that were got will be put before returning to the caller. > > The idea firstly came out when i wanted to clean up and optimize > imx6q-cpufreq driver which needs to handle a lot of clocks as it > becomes a bit mess. > > e.g. drivers/cpufreq/imx6q-cpufreq.c > > You will see we need get 7 clocks during driver probe. > (Add there will be more, e.g. pll1, pll1_bypass, pll1_bypass_src, > in the future when adding busfreq support). > > static int imx6q_cpufreq_probe(struct platform_device *pdev) > { > .... > arm_clk = clk_get(cpu_dev, "arm"); > pll1_sys_clk = clk_get(cpu_dev, "pll1_sys"); > pll1_sw_clk = clk_get(cpu_dev, "pll1_sw"); > step_clk = clk_get(cpu_dev, "step"); > pll2_pfd2_396m_clk = clk_get(cpu_dev, "pll2_pfd2_396m"); > if (IS_ERR(arm_clk) || IS_ERR(pll1_sys_clk) || IS_ERR(pll1_sw_clk) || > IS_ERR(step_clk) || IS_ERR(pll2_pfd2_396m_clk)) { > dev_err(cpu_dev, "failed to get clocks\n"); > ret = -ENOENT; > goto put_clk; > } > > if (of_machine_is_compatible("fsl,imx6ul")) { > pll2_bus_clk = clk_get(cpu_dev, "pll2_bus"); > secondary_sel_clk = clk_get(cpu_dev, "secondary_sel"); > if (IS_ERR(pll2_bus_clk) || IS_ERR(secondary_sel_clk)) { > dev_err(cpu_dev, "failed to get clocks specific to imx6ul\n"); > ret = -ENOENT; > goto put_clk; > } > } > .... > put_clk: > if (!IS_ERR(arm_clk)) > clk_put(arm_clk); > if (!IS_ERR(pll1_sys_clk)) > clk_put(pll1_sys_clk); > ........... > } > > Together with the err path handling for each clocks, it does make > things a bit ugly. > > Since we already have regulator_bulk_get accessories, i thought we > probably could introduce clk_bulk_get as well to handle such case to > ease the driver owners' life. > > Besides IMX cpufreq driver, there is also some similar cases > in kernel which could befinit from this api as well. > e.g. > drivers/cpufreq/tegra124-cpufreq.c > drivers/cpufreq/s3c2412-cpufreq.c > sound/soc/samsung/smdk_spdif.c > arch/arm/mach-omap1/serial.c > ... > > And actually, if we handle clocks more than 3, then it might be > worthy to try, which there is quite many manay in kernel and > that probably could save a lot codes. > > This is a RFC patch intending to bring up the idea to discuss. > > Comments are welcome and appreciated! Thanks a lot for doing this, we have historically done something similar on ARCH_BRCMSTB platforms in our downstream tree with a concept of a "software" clock which has multiple parents (yikes), using the bulk accessors would essentially allow us to remove part of this hack, so I am all in favor of this! > > Dong Aisheng (3): > clk: add clk_bulk_get accessories > clk: add managed version of clk_bulk_get > cpufreq: imx6q: refine clk operations > > drivers/clk/clk-devres.c | 36 +++++++++++ > drivers/clk/clk.c | 99 +++++++++++++++++++++++++++++ > drivers/clk/clkdev.c | 42 +++++++++++++ > drivers/cpufreq/imx6q-cpufreq.c | 119 ++++++++++++++++------------------- > include/linux/clk.h | 135 ++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 365 insertions(+), 66 deletions(-) > -- Florian