From: Tomasz Figa <tomasz.figa@gmail.com>
To: "Heiko Stübner" <heiko@sntech.de>, "Kukjin Kim" <kgene.kim@samsung.com>
Cc: t.figa@samsung.com, mturquette@linaro.org,
linux-arm-kernel@lists.infradead.org,
linux-samsung-soc@vger.kernel.org
Subject: Re: [PATCH 03/12] clk: samsung: add clock driver for external clock outputs
Date: Sun, 09 Feb 2014 03:25:21 +0100 [thread overview]
Message-ID: <52F6E711.4020109@gmail.com> (raw)
In-Reply-To: <201312131359.36576.heiko@sntech.de>
Hi Heiko,
On 13.12.2013 13:59, Heiko Stübner wrote:
> This adds a driver for controlling the external clock outputs of
> s3c24xx architectures including the dclk muxes and dividers.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> drivers/clk/samsung/Makefile | 1 +
> drivers/clk/samsung/clk-s3c2410-dclk.c | 517 ++++++++++++++++++++++
> include/dt-bindings/clock/samsung,s3c2410-dclk.h | 28 ++
> 3 files changed, 546 insertions(+)
> create mode 100644 drivers/clk/samsung/clk-s3c2410-dclk.c
> create mode 100644 include/dt-bindings/clock/samsung,s3c2410-dclk.h
>
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 4c892c6..568683c 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -8,5 +8,6 @@ obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
> obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
> obj-$(CONFIG_SOC_EXYNOS5440) += clk-exynos5440.o
> obj-$(CONFIG_ARCH_EXYNOS) += clk-exynos-audss.o
> +obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
> obj-$(CONFIG_S3C2443_COMMON_CLK)+= clk-s3c2443.o
> obj-$(CONFIG_ARCH_S3C64XX) += clk-s3c64xx.o
> diff --git a/drivers/clk/samsung/clk-s3c2410-dclk.c b/drivers/clk/samsung/clk-s3c2410-dclk.c
> new file mode 100644
> index 0000000..de10e5c
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-s3c2410-dclk.c
> @@ -0,0 +1,517 @@
> +/*
> + * Copyright (c) 2013 Heiko Stuebner <heiko@sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Common Clock Framework support for s3c24xx external clock output.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
> +#include <dt-bindings/clock/samsung,s3c2410-dclk.h>
> +#include "clk.h"
> +
> +/* legacy access to misccr, until dt conversion is finished */
> +#include <mach/hardware.h>
> +#include <mach/regs-gpio.h>
> +
> +enum supported_socs {
> + S3C2410,
> + S3C2412,
> + S3C2440,
> + S3C2443,
> +};
> +
> +struct s3c24xx_dclk_drv_data {
> + int cpu_type;
> +};
> +
> +/*
> + * Clock for output-parent selection in misccr
> + */
> +
> +struct s3c24xx_clkout {
> + struct clk_hw hw;
> + struct regmap *misccr;
> + u32 mask;
> + u8 shift;
> +};
> +
> +#define to_s3c24xx_clkout(_hw) container_of(_hw, struct s3c24xx_clkout, hw)
> +
> +static u8 s3c24xx_clkout_get_parent(struct clk_hw *hw)
> +{
> + struct s3c24xx_clkout *clkout = to_s3c24xx_clkout(hw);
> + int num_parents = __clk_get_num_parents(hw->clk);
> + u32 val;
> + int ret = 0;
> +
> + if (clkout->misccr)
> + ret = regmap_read(clkout->misccr, 0, &val);
> + else
> + val = readl_relaxed(S3C24XX_MISCCR) >> clkout->shift;
I wonder if this couldn't be simplified by always providing a regmap.
> +
> + if (ret)
> + return ret;
> +
> + val >>= clkout->shift;
> + val &= clkout->mask;
> +
> + if (val >= num_parents)
> + return -EINVAL;
> +
> + return val;
> +}
[snip]
> +#define to_s3c24xx_dclk0(x) \
> + container_of(x, struct s3c24xx_dclk, dclk0_div_change_nb)
> +
> +#define to_s3c24xx_dclk1(x) \
> + container_of(x, struct s3c24xx_dclk, dclk1_div_change_nb)
> +
> +static const char dummy_nm[] __initconst = "dummy_name";
What's the advantage of having it defined this way instead of using
"dummy_name" (or probably "reserved" or "none", as in Samsung clock
drivers) directly in parent lists?
> +
> +PNAME(dclk_s3c2410_p) = { "pclk", "uclk" };
> +PNAME(clkout0_s3c2410_p) = { "mpll", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk0" };
> +PNAME(clkout1_s3c2410_p) = { "mpll", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk1" };
> +
> +PNAME(clkout0_s3c2412_p) = { "mpll", "upll", dummy_nm /* rtc clock output */,
> + "hclk", "pclk", "gate_dclk0" };
Hmm, this would suggest that instead of dummy_nm, a real name should be
used here, even if such clock doesn't exist yet. CCF will handle this fine.
> +PNAME(clkout1_s3c2412_p) = { "xti", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk1" };
> +
> +PNAME(clkout0_s3c2440_p) = { "xti", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk0" };
> +PNAME(clkout1_s3c2440_p) = { "mpll", "upll", dummy_nm /* rtc clock output */,
> + "hclk", "pclk", "gate_dclk1" };
[snip]
> +static int s3c24xx_dclk_probe(struct platform_device *pdev)
> +{
> + struct s3c24xx_dclk *s3c24xx_dclk;
> + struct device_node *np = pdev->dev.of_node;
> + struct regmap *misccr = NULL;
> + struct resource *mem;
> + struct clk **clk_table;
> + const char **clkout0_parent_names, **clkout1_parent_names;
> + u8 clkout0_num_parents, clkout1_num_parents;
> + int current_soc, ret, i;
> +
> + s3c24xx_dclk = devm_kzalloc(&pdev->dev, sizeof(*s3c24xx_dclk),
> + GFP_KERNEL);
> + if (!s3c24xx_dclk)
> + return -ENOMEM;
> +
> + s3c24xx_dclk->dev = &pdev->dev;
> + platform_set_drvdata(pdev, s3c24xx_dclk);
> + spin_lock_init(&s3c24xx_dclk->dclk_lock);
> +
> + clk_table = devm_kzalloc(&pdev->dev,
> + sizeof(struct clk *) * DCLK_MAX_CLKS,
> + GFP_KERNEL);
> + if (!clk_table)
> + return -ENOMEM;
> +
> + s3c24xx_dclk->clk_data.clks = clk_table;
> + s3c24xx_dclk->clk_data.clk_num = DCLK_MAX_CLKS;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + s3c24xx_dclk->base = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(s3c24xx_dclk->base))
> + return PTR_ERR(s3c24xx_dclk->base);
> +
> + /* when run from devicetree, get the misccr through a syscon-regmap */
> + if (np) {
> + misccr = syscon_regmap_lookup_by_phandle(np, "samsung,misccr");
> + if (IS_ERR(misccr)) {
> + dev_err(&pdev->dev, "could not get misccr syscon, %ld\n",
> + PTR_ERR(misccr));
> + return PTR_ERR(misccr);
> + }
> + }
> +
> + current_soc = s3c24xx_dclk_get_driver_data(pdev);
> +
> + if (current_soc == S3C2443) {
> + clk_table[MUX_DCLK0] = clk_register_mux(&pdev->dev,
> + "mux_dclk0", dclk_s3c2443_p,
> + ARRAY_SIZE(dclk_s3c2443_p), 0,
> + s3c24xx_dclk->base, 1, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + clk_table[MUX_DCLK1] = clk_register_mux(&pdev->dev,
> + "mux_dclk1", dclk_s3c2443_p,
> + ARRAY_SIZE(dclk_s3c2443_p), 0,
> + s3c24xx_dclk->base, 17, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + } else {
> + clk_table[MUX_DCLK0] = clk_register_mux(&pdev->dev,
> + "mux_dclk0", dclk_s3c2410_p,
> + ARRAY_SIZE(dclk_s3c2410_p), 0,
> + s3c24xx_dclk->base, 1, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + clk_table[MUX_DCLK1] = clk_register_mux(&pdev->dev,
> + "mux_dclk1", dclk_s3c2410_p,
> + ARRAY_SIZE(dclk_s3c2410_p), 0,
> + s3c24xx_dclk->base, 17, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + }
What about using a variant struct instead? Match tables would simply
contain pointers to respective structs and here the code would refer to
appropriate fields in a struct returned by s3c24xx_dclk_get_driver_data().
> +
> + clk_table[DIV_DCLK0] = clk_register_divider(&pdev->dev, "div_dclk0",
> + "mux_dclk0", 0, s3c24xx_dclk->base,
> + 4, 4, 0, &s3c24xx_dclk->dclk_lock);
> + clk_table[DIV_DCLK1] = clk_register_divider(&pdev->dev, "div_dclk1",
> + "mux_dclk1", 0, s3c24xx_dclk->base,
> + 20, 4, 0, &s3c24xx_dclk->dclk_lock);
> +
> + clk_table[GATE_DCLK0] = clk_register_gate(&pdev->dev, "gate_dclk0",
> + "div_dclk0", CLK_SET_RATE_PARENT,
> + s3c24xx_dclk->base, 0, 0,
> + &s3c24xx_dclk->dclk_lock);
> + clk_table[GATE_DCLK1] = clk_register_gate(&pdev->dev, "gate_dclk1",
> + "div_dclk1", CLK_SET_RATE_PARENT,
> + s3c24xx_dclk->base, 16, 0,
> + &s3c24xx_dclk->dclk_lock);
> +
> + switch (current_soc) {
> + case S3C2410:
> + clkout0_parent_names = clkout0_s3c2410_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2410_p);
> + clkout1_parent_names = clkout1_s3c2410_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2410_p);
> + break;
> + case S3C2412:
> + clkout0_parent_names = clkout0_s3c2412_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2412_p);
> + clkout1_parent_names = clkout1_s3c2412_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2412_p);
> + break;
> + case S3C2440:
> + clkout0_parent_names = clkout0_s3c2440_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2440_p);
> + clkout1_parent_names = clkout1_s3c2440_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2440_p);
> + break;
> + case S3C2443:
> + clkout0_parent_names = clkout0_s3c2443_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2443_p);
> + clkout1_parent_names = clkout1_s3c2443_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2443_p);
> + break;
> + default:
> + dev_err(&pdev->dev, "unsupported soc %d\n", current_soc);
> + ret = -EINVAL;
> + goto err_clk_register;
> + }
Ditto.
> +
> + clk_table[MUX_CLKOUT0] = s3c24xx_register_clkout(&pdev->dev,
> + "clkout0", clkout0_parent_names,
> + clkout0_num_parents, misccr, 4, 7);
> + clk_table[MUX_CLKOUT1] = s3c24xx_register_clkout(&pdev->dev, "clkout1",
> + clkout1_parent_names,
> + clkout1_num_parents, misccr, 8, 7);
> +
> + for (i = 0; i < DCLK_MAX_CLKS; i++)
> + if (IS_ERR(clk_table[i])) {
> + dev_err(&pdev->dev, "clock %d failed to register\n", i);
> + ret = PTR_ERR(clk_table[i]);
> + goto err_clk_register;
> + }
> +
> + ret = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL);
> + ret |= clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL);
> + ret |= clk_register_clkdev(clk_table[MUX_CLKOUT0], "clkout0", NULL);
> + ret |= clk_register_clkdev(clk_table[MUX_CLKOUT1], "clkout1", NULL);
Hmm, won't it break the error value if two calls return different error
codes?
I guess that
if (!ret)
ret = ...
if (!ret)
ret = ...
construct would be more appropriate here, if you don't want error
message per call.
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register aliases\n");
> + goto err_clk_register;
> + }
> +
> + s3c24xx_dclk->dclk0_div_change_nb.notifier_call =
> + s3c24xx_dclk0_div_notify;
> + s3c24xx_dclk->dclk0_div_change_nb.next = NULL;
Do you need to set this field to NULL explicitly?
Best regards,
Tomasz
WARNING: multiple messages have this Message-ID (diff)
From: tomasz.figa@gmail.com (Tomasz Figa)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 03/12] clk: samsung: add clock driver for external clock outputs
Date: Sun, 09 Feb 2014 03:25:21 +0100 [thread overview]
Message-ID: <52F6E711.4020109@gmail.com> (raw)
In-Reply-To: <201312131359.36576.heiko@sntech.de>
Hi Heiko,
On 13.12.2013 13:59, Heiko St?bner wrote:
> This adds a driver for controlling the external clock outputs of
> s3c24xx architectures including the dclk muxes and dividers.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> drivers/clk/samsung/Makefile | 1 +
> drivers/clk/samsung/clk-s3c2410-dclk.c | 517 ++++++++++++++++++++++
> include/dt-bindings/clock/samsung,s3c2410-dclk.h | 28 ++
> 3 files changed, 546 insertions(+)
> create mode 100644 drivers/clk/samsung/clk-s3c2410-dclk.c
> create mode 100644 include/dt-bindings/clock/samsung,s3c2410-dclk.h
>
> diff --git a/drivers/clk/samsung/Makefile b/drivers/clk/samsung/Makefile
> index 4c892c6..568683c 100644
> --- a/drivers/clk/samsung/Makefile
> +++ b/drivers/clk/samsung/Makefile
> @@ -8,5 +8,6 @@ obj-$(CONFIG_SOC_EXYNOS5250) += clk-exynos5250.o
> obj-$(CONFIG_SOC_EXYNOS5420) += clk-exynos5420.o
> obj-$(CONFIG_SOC_EXYNOS5440) += clk-exynos5440.o
> obj-$(CONFIG_ARCH_EXYNOS) += clk-exynos-audss.o
> +obj-$(CONFIG_S3C2410_COMMON_DCLK)+= clk-s3c2410-dclk.o
> obj-$(CONFIG_S3C2443_COMMON_CLK)+= clk-s3c2443.o
> obj-$(CONFIG_ARCH_S3C64XX) += clk-s3c64xx.o
> diff --git a/drivers/clk/samsung/clk-s3c2410-dclk.c b/drivers/clk/samsung/clk-s3c2410-dclk.c
> new file mode 100644
> index 0000000..de10e5c
> --- /dev/null
> +++ b/drivers/clk/samsung/clk-s3c2410-dclk.c
> @@ -0,0 +1,517 @@
> +/*
> + * Copyright (c) 2013 Heiko Stuebner <heiko@sntech.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Common Clock Framework support for s3c24xx external clock output.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/clk-provider.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
> +#include <dt-bindings/clock/samsung,s3c2410-dclk.h>
> +#include "clk.h"
> +
> +/* legacy access to misccr, until dt conversion is finished */
> +#include <mach/hardware.h>
> +#include <mach/regs-gpio.h>
> +
> +enum supported_socs {
> + S3C2410,
> + S3C2412,
> + S3C2440,
> + S3C2443,
> +};
> +
> +struct s3c24xx_dclk_drv_data {
> + int cpu_type;
> +};
> +
> +/*
> + * Clock for output-parent selection in misccr
> + */
> +
> +struct s3c24xx_clkout {
> + struct clk_hw hw;
> + struct regmap *misccr;
> + u32 mask;
> + u8 shift;
> +};
> +
> +#define to_s3c24xx_clkout(_hw) container_of(_hw, struct s3c24xx_clkout, hw)
> +
> +static u8 s3c24xx_clkout_get_parent(struct clk_hw *hw)
> +{
> + struct s3c24xx_clkout *clkout = to_s3c24xx_clkout(hw);
> + int num_parents = __clk_get_num_parents(hw->clk);
> + u32 val;
> + int ret = 0;
> +
> + if (clkout->misccr)
> + ret = regmap_read(clkout->misccr, 0, &val);
> + else
> + val = readl_relaxed(S3C24XX_MISCCR) >> clkout->shift;
I wonder if this couldn't be simplified by always providing a regmap.
> +
> + if (ret)
> + return ret;
> +
> + val >>= clkout->shift;
> + val &= clkout->mask;
> +
> + if (val >= num_parents)
> + return -EINVAL;
> +
> + return val;
> +}
[snip]
> +#define to_s3c24xx_dclk0(x) \
> + container_of(x, struct s3c24xx_dclk, dclk0_div_change_nb)
> +
> +#define to_s3c24xx_dclk1(x) \
> + container_of(x, struct s3c24xx_dclk, dclk1_div_change_nb)
> +
> +static const char dummy_nm[] __initconst = "dummy_name";
What's the advantage of having it defined this way instead of using
"dummy_name" (or probably "reserved" or "none", as in Samsung clock
drivers) directly in parent lists?
> +
> +PNAME(dclk_s3c2410_p) = { "pclk", "uclk" };
> +PNAME(clkout0_s3c2410_p) = { "mpll", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk0" };
> +PNAME(clkout1_s3c2410_p) = { "mpll", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk1" };
> +
> +PNAME(clkout0_s3c2412_p) = { "mpll", "upll", dummy_nm /* rtc clock output */,
> + "hclk", "pclk", "gate_dclk0" };
Hmm, this would suggest that instead of dummy_nm, a real name should be
used here, even if such clock doesn't exist yet. CCF will handle this fine.
> +PNAME(clkout1_s3c2412_p) = { "xti", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk1" };
> +
> +PNAME(clkout0_s3c2440_p) = { "xti", "upll", "fclk", "hclk", "pclk",
> + "gate_dclk0" };
> +PNAME(clkout1_s3c2440_p) = { "mpll", "upll", dummy_nm /* rtc clock output */,
> + "hclk", "pclk", "gate_dclk1" };
[snip]
> +static int s3c24xx_dclk_probe(struct platform_device *pdev)
> +{
> + struct s3c24xx_dclk *s3c24xx_dclk;
> + struct device_node *np = pdev->dev.of_node;
> + struct regmap *misccr = NULL;
> + struct resource *mem;
> + struct clk **clk_table;
> + const char **clkout0_parent_names, **clkout1_parent_names;
> + u8 clkout0_num_parents, clkout1_num_parents;
> + int current_soc, ret, i;
> +
> + s3c24xx_dclk = devm_kzalloc(&pdev->dev, sizeof(*s3c24xx_dclk),
> + GFP_KERNEL);
> + if (!s3c24xx_dclk)
> + return -ENOMEM;
> +
> + s3c24xx_dclk->dev = &pdev->dev;
> + platform_set_drvdata(pdev, s3c24xx_dclk);
> + spin_lock_init(&s3c24xx_dclk->dclk_lock);
> +
> + clk_table = devm_kzalloc(&pdev->dev,
> + sizeof(struct clk *) * DCLK_MAX_CLKS,
> + GFP_KERNEL);
> + if (!clk_table)
> + return -ENOMEM;
> +
> + s3c24xx_dclk->clk_data.clks = clk_table;
> + s3c24xx_dclk->clk_data.clk_num = DCLK_MAX_CLKS;
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + s3c24xx_dclk->base = devm_ioremap_resource(&pdev->dev, mem);
> + if (IS_ERR(s3c24xx_dclk->base))
> + return PTR_ERR(s3c24xx_dclk->base);
> +
> + /* when run from devicetree, get the misccr through a syscon-regmap */
> + if (np) {
> + misccr = syscon_regmap_lookup_by_phandle(np, "samsung,misccr");
> + if (IS_ERR(misccr)) {
> + dev_err(&pdev->dev, "could not get misccr syscon, %ld\n",
> + PTR_ERR(misccr));
> + return PTR_ERR(misccr);
> + }
> + }
> +
> + current_soc = s3c24xx_dclk_get_driver_data(pdev);
> +
> + if (current_soc == S3C2443) {
> + clk_table[MUX_DCLK0] = clk_register_mux(&pdev->dev,
> + "mux_dclk0", dclk_s3c2443_p,
> + ARRAY_SIZE(dclk_s3c2443_p), 0,
> + s3c24xx_dclk->base, 1, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + clk_table[MUX_DCLK1] = clk_register_mux(&pdev->dev,
> + "mux_dclk1", dclk_s3c2443_p,
> + ARRAY_SIZE(dclk_s3c2443_p), 0,
> + s3c24xx_dclk->base, 17, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + } else {
> + clk_table[MUX_DCLK0] = clk_register_mux(&pdev->dev,
> + "mux_dclk0", dclk_s3c2410_p,
> + ARRAY_SIZE(dclk_s3c2410_p), 0,
> + s3c24xx_dclk->base, 1, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + clk_table[MUX_DCLK1] = clk_register_mux(&pdev->dev,
> + "mux_dclk1", dclk_s3c2410_p,
> + ARRAY_SIZE(dclk_s3c2410_p), 0,
> + s3c24xx_dclk->base, 17, 1, 0,
> + &s3c24xx_dclk->dclk_lock);
> + }
What about using a variant struct instead? Match tables would simply
contain pointers to respective structs and here the code would refer to
appropriate fields in a struct returned by s3c24xx_dclk_get_driver_data().
> +
> + clk_table[DIV_DCLK0] = clk_register_divider(&pdev->dev, "div_dclk0",
> + "mux_dclk0", 0, s3c24xx_dclk->base,
> + 4, 4, 0, &s3c24xx_dclk->dclk_lock);
> + clk_table[DIV_DCLK1] = clk_register_divider(&pdev->dev, "div_dclk1",
> + "mux_dclk1", 0, s3c24xx_dclk->base,
> + 20, 4, 0, &s3c24xx_dclk->dclk_lock);
> +
> + clk_table[GATE_DCLK0] = clk_register_gate(&pdev->dev, "gate_dclk0",
> + "div_dclk0", CLK_SET_RATE_PARENT,
> + s3c24xx_dclk->base, 0, 0,
> + &s3c24xx_dclk->dclk_lock);
> + clk_table[GATE_DCLK1] = clk_register_gate(&pdev->dev, "gate_dclk1",
> + "div_dclk1", CLK_SET_RATE_PARENT,
> + s3c24xx_dclk->base, 16, 0,
> + &s3c24xx_dclk->dclk_lock);
> +
> + switch (current_soc) {
> + case S3C2410:
> + clkout0_parent_names = clkout0_s3c2410_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2410_p);
> + clkout1_parent_names = clkout1_s3c2410_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2410_p);
> + break;
> + case S3C2412:
> + clkout0_parent_names = clkout0_s3c2412_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2412_p);
> + clkout1_parent_names = clkout1_s3c2412_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2412_p);
> + break;
> + case S3C2440:
> + clkout0_parent_names = clkout0_s3c2440_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2440_p);
> + clkout1_parent_names = clkout1_s3c2440_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2440_p);
> + break;
> + case S3C2443:
> + clkout0_parent_names = clkout0_s3c2443_p;
> + clkout0_num_parents = ARRAY_SIZE(clkout0_s3c2443_p);
> + clkout1_parent_names = clkout1_s3c2443_p;
> + clkout1_num_parents = ARRAY_SIZE(clkout1_s3c2443_p);
> + break;
> + default:
> + dev_err(&pdev->dev, "unsupported soc %d\n", current_soc);
> + ret = -EINVAL;
> + goto err_clk_register;
> + }
Ditto.
> +
> + clk_table[MUX_CLKOUT0] = s3c24xx_register_clkout(&pdev->dev,
> + "clkout0", clkout0_parent_names,
> + clkout0_num_parents, misccr, 4, 7);
> + clk_table[MUX_CLKOUT1] = s3c24xx_register_clkout(&pdev->dev, "clkout1",
> + clkout1_parent_names,
> + clkout1_num_parents, misccr, 8, 7);
> +
> + for (i = 0; i < DCLK_MAX_CLKS; i++)
> + if (IS_ERR(clk_table[i])) {
> + dev_err(&pdev->dev, "clock %d failed to register\n", i);
> + ret = PTR_ERR(clk_table[i]);
> + goto err_clk_register;
> + }
> +
> + ret = clk_register_clkdev(clk_table[MUX_DCLK0], "dclk0", NULL);
> + ret |= clk_register_clkdev(clk_table[MUX_DCLK1], "dclk1", NULL);
> + ret |= clk_register_clkdev(clk_table[MUX_CLKOUT0], "clkout0", NULL);
> + ret |= clk_register_clkdev(clk_table[MUX_CLKOUT1], "clkout1", NULL);
Hmm, won't it break the error value if two calls return different error
codes?
I guess that
if (!ret)
ret = ...
if (!ret)
ret = ...
construct would be more appropriate here, if you don't want error
message per call.
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register aliases\n");
> + goto err_clk_register;
> + }
> +
> + s3c24xx_dclk->dclk0_div_change_nb.notifier_call =
> + s3c24xx_dclk0_div_notify;
> + s3c24xx_dclk->dclk0_div_change_nb.next = NULL;
Do you need to set this field to NULL explicitly?
Best regards,
Tomasz
next prev parent reply other threads:[~2014-02-09 2:25 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-13 12:56 [PATCH 00/12] ARM: S3C24XX: convert s3c2410, s3c2440 s3c2442 to common clock framework Heiko Stübner
2013-12-13 12:56 ` Heiko Stübner
2013-12-13 12:57 ` [PATCH 01/12] ARM: S3C24XX: cpufreq-utils: don't write raw values to MPLLCON when using ccf Heiko Stübner
2013-12-13 12:57 ` Heiko Stübner
2014-02-08 20:23 ` Tomasz Figa
2014-02-08 20:23 ` Tomasz Figa
2013-12-13 12:59 ` [PATCH 02/12] dt-bindings: document s3c24xx controller for external clock output Heiko Stübner
2013-12-13 12:59 ` Heiko Stübner
2014-02-09 1:54 ` Tomasz Figa
2014-02-09 1:54 ` Tomasz Figa
2014-02-16 20:33 ` Heiko Stübner
2014-02-16 20:33 ` Heiko Stübner
2014-02-16 20:51 ` Tomasz Figa
2014-02-16 20:51 ` Tomasz Figa
2013-12-13 12:59 ` [PATCH 03/12] clk: samsung: add clock driver for external clock outputs Heiko Stübner
2013-12-13 12:59 ` Heiko Stübner
2013-12-31 19:46 ` Mike Turquette
2013-12-31 19:46 ` Mike Turquette
2014-02-09 2:25 ` Tomasz Figa [this message]
2014-02-09 2:25 ` Tomasz Figa
2013-12-13 13:00 ` [PATCH 04/12] ARM: S3C24XX: enable usage of common dclk if common clock framework is enabled Heiko Stübner
2013-12-13 13:00 ` Heiko Stübner
2013-12-13 13:00 ` [PATCH 05/12] ARM: S3C24XX: only store clock registers when old clock code is active Heiko Stübner
2013-12-13 13:00 ` Heiko Stübner
2013-12-13 13:00 ` [PATCH 06/12] clk: samsung: add plls used by the early s3c24xx cpus Heiko Stübner
2013-12-13 13:00 ` Heiko Stübner
2013-12-31 19:45 ` Mike Turquette
2013-12-31 19:45 ` Mike Turquette
2013-12-13 13:01 ` [PATCH 08/12] clk: samsung: add clock controller driver for s3c2410, s3c2440 and s3c2442 Heiko Stübner
2013-12-13 13:01 ` Heiko Stübner
2014-02-06 14:12 ` Mike Turquette
2014-02-06 14:12 ` Mike Turquette
2014-02-09 19:34 ` Tomasz Figa
2014-02-09 19:34 ` Tomasz Figa
2014-02-17 21:05 ` Heiko Stübner
2014-02-17 21:05 ` Heiko Stübner
2014-02-17 22:37 ` Tomasz Figa
2014-02-17 22:37 ` Tomasz Figa
2014-02-17 22:48 ` Heiko Stübner
2014-02-17 22:48 ` Heiko Stübner
2013-12-13 13:02 ` [PATCH 09/12] ARM: S3C24XX: add platform code for conversion to the common clock framework Heiko Stübner
2013-12-13 13:02 ` Heiko Stübner
2013-12-13 13:02 ` [PATCH 10/12] ARM: S3C24XX: convert s3c2440 and s3c2442 to " Heiko Stübner
2013-12-13 13:02 ` Heiko Stübner
2013-12-13 13:03 ` [PATCH 11/12] ARM: S3C24XX: convert s3c2410 " Heiko Stübner
2013-12-13 13:03 ` Heiko Stübner
2013-12-13 13:03 ` [PATCH 12/12] ARM: S3C24XX: remove legacy clock code Heiko Stübner
2013-12-13 13:03 ` Heiko Stübner
2014-02-09 19:56 ` [PATCH 00/12] ARM: S3C24XX: convert s3c2410, s3c2440 s3c2442 to common clock framework Tomasz Figa
2014-02-09 19:56 ` Tomasz Figa
2014-02-11 6:16 ` Kukjin Kim
2014-02-11 6:16 ` Kukjin Kim
2014-02-11 9:20 ` Heiko Stübner
2014-02-11 9:20 ` Heiko Stübner
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=52F6E711.4020109@gmail.com \
--to=tomasz.figa@gmail.com \
--cc=heiko@sntech.de \
--cc=kgene.kim@samsung.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=mturquette@linaro.org \
--cc=t.figa@samsung.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.