From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Message-ID: <1530607882.2900.182.camel@baylibre.com> Subject: Re: [PATCH 3/3] clk: meson: add sub EMMC clock controller driver From: Jerome Brunet To: Yixun Lan , Neil Armstrong Cc: Kevin Hilman , Carlo Caione , Michael Turquette , Stephen Boyd , Rob Herring , Miquel Raynal , Boris Brezillon , Martin Blumenstingl , Liang Yang , Qiufang Dai , Jian Hu , linux-clk@vger.kernel.org, linux-amlogic@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Date: Tue, 03 Jul 2018 10:51:22 +0200 In-Reply-To: <20180703145716.31860-4-yixun.lan@amlogic.com> References: <20180703145716.31860-1-yixun.lan@amlogic.com> <20180703145716.31860-4-yixun.lan@amlogic.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 List-ID: On Tue, 2018-07-03 at 14:57 +0000, Yixun Lan wrote: > This patch will add a EMMC clock controller driver support, > It provide a mux and divider clock. > > This clock driver can be protentially used by either EMMC and > NAND driver. > > Signed-off-by: Yixun Lan > --- > drivers/clk/meson/Kconfig | 9 +++ > drivers/clk/meson/Makefile | 1 + > drivers/clk/meson/emmc-clkc.c | 136 ++++++++++++++++++++++++++++++++++ > 3 files changed, 146 insertions(+) > create mode 100644 drivers/clk/meson/emmc-clkc.c > > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig > index efaa70f682b4..2f27ff08e4eb 100644 > --- a/drivers/clk/meson/Kconfig > +++ b/drivers/clk/meson/Kconfig > @@ -15,6 +15,15 @@ config COMMON_CLK_MESON_AO > select COMMON_CLK_REGMAP_MESON > select RESET_CONTROLLER > > +config COMMON_CLK_EMMC_MESON > + tristate "Meson EMMC Sub Clock Controller Driver" > + depends on COMMON_CLK_AMLOGIC > + select MFD_SYSCON > + select REGMAP > + help > + Support for the EMMC sub clock controller on AmLogic Meson Platform, I thought you were not writing amlogic this way anymore -^ ?? ^ ... or be consistent about it. | | Drop the camel case please -------------| > + Say Y if you want this clock enabled. > + > config COMMON_CLK_REGMAP_MESON > bool > select REGMAP > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile > index 72ec8c40d848..2f04f77ba4de 100644 > --- a/drivers/clk/meson/Makefile > +++ b/drivers/clk/meson/Makefile > @@ -9,4 +9,5 @@ obj-$(CONFIG_COMMON_CLK_MESON8B) += meson8b.o > obj-$(CONFIG_COMMON_CLK_GXBB) += gxbb.o gxbb-aoclk.o gxbb-aoclk-32k.o > obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o > obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o > +obj-$(CONFIG_COMMON_CLK_EMMC_MESON) += emmc-clkc.o > obj-$(CONFIG_COMMON_CLK_REGMAP_MESON) += clk-regmap.o > diff --git a/drivers/clk/meson/emmc-clkc.c b/drivers/clk/meson/emmc-clkc.c > new file mode 100644 > index 000000000000..cf5bb9f34327 > --- /dev/null > +++ b/drivers/clk/meson/emmc-clkc.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Amlogic Meson EMMC Sub Clock Controller Driver > + * > + * Copyright (c) 2018 Amlogic, inc. > + * Author: Yixun Lan > + */ > + > +#include > +#include > +#include replace with linux/module.h > +#include Why do you need this ? > +#include > +#include > +#include > +#include > + > +#include > + > +#include "clkc.h" > + > +#define SD_EMMC_CLOCK 0 > +#define MUX_CLK_NUM_PARENTS 2 > +#define EMMC_MAX_CLKS 2 > +#define CLK_NAME_LEN 48 > + > +static struct clk_regmap_mux_data emmc_clkc_mux_data = { > + .offset = SD_EMMC_CLOCK, > + .mask = 0x3, > + .shift = 6, If you round the divider "closest", shouldn't you do the same for the mux ? > +}; > + > +static struct clk_regmap_div_data emmc_clkc_div_data = { > + .offset = SD_EMMC_CLOCK, > + .shift = 0, > + .width = 6, > + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > +}; > + > +static const struct of_device_id clkc_match_table[] = { > + { .compatible = "amlogic,emmc-clkc" }, > + {} > +}; > + > +static int emmc_clkc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct clk_regmap *mux, *div; > + struct regmap *map; > + struct clk *clk; > + int i, ret; > + const char *parent_names[MUX_CLK_NUM_PARENTS]; > + struct clk_init_data init; > + char mux_name[CLK_NAME_LEN], div_name[CLK_NAME_LEN]; I'm not big fan, especially if you append the dev_name() to the clock name. > + struct clk_hw_onecell_data *onecell_data; > + > + map = syscon_node_to_regmap(dev->of_node); > + > + mux = devm_kzalloc(dev, sizeof(*mux), GFP_KERNEL); > + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); > + > + onecell_data = devm_kzalloc(dev, sizeof(*onecell_data) + > + sizeof(*onecell_data->hws) * EMMC_MAX_CLKS, > + GFP_KERNEL); > + > + if (!mux || !div || !onecell_data) > + return -ENOMEM; > + > + for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { > + char name[8]; > + > + snprintf(name, sizeof(name), "clkin%d", i); > + clk = devm_clk_get(dev, name); > + if (IS_ERR(clk)) { > + if (clk != ERR_PTR(-EPROBE_DEFER)) > + dev_err(dev, "Missing clock %s\n", name); > + return PTR_ERR(clk); > + } > + > + parent_names[i] = __clk_get_name(clk); > + } > + > + mux->map = map; > + mux->data = &emmc_clkc_mux_data; > + > + snprintf(mux_name, CLK_NAME_LEN, "%s#emmc_mux", dev_name(dev)); I'd prefer if you used kasprintf() and free the name after the name after registration. > + > + init.name = mux_name; > + init.ops = &clk_regmap_mux_ops; > + init.flags = CLK_SET_RATE_PARENT; If you really intend to have manual control over the mux, as commented in patch 2, you need CLK_SET_RATE_NOREPARENT here, otherwise whatever you set may be overwritten by the next clk_set_rate() call. Please choose. > + init.parent_names = parent_names; > + init.num_parents = MUX_CLK_NUM_PARENTS; > + > + mux->hw.init = &init; > + ret = devm_clk_hw_register(dev, &mux->hw); > + if (ret) { > + dev_err(dev, "Mux clock registration failed\n"); > + return ret; > + } > + > + parent_names[0] = mux_name; > + div->map = map; > + div->data = &emmc_clkc_div_data; > + > + snprintf(div_name, CLK_NAME_LEN, "%s#emmc_div", dev_name(dev)); > + > + init.name = div_name; > + init.ops = &clk_regmap_divider_ops; > + init.flags = CLK_SET_RATE_PARENT; > + init.parent_names = parent_names; > + init.num_parents = 1; > + > + div->hw.init = &init; > + ret = devm_clk_hw_register(dev, &div->hw); > + if (ret) { > + dev_err(dev, "Div clock registration failed\n"); s/Div/Divider > + return ret; > + } > + > + onecell_data->hws[CLKID_EMMC_C_MUX] = &mux->hw, > + onecell_data->hws[CLKID_EMMC_C_DIV] = &div->hw, > + onecell_data->num = EMMC_MAX_CLKS; > + > + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > + onecell_data); This is a long function with a lot of locals. please break that up. (for example, one function to register each clock, helper to allocate the clk_hw and names ...) > +} > + > +static struct platform_driver emmc_clkc_driver = { > + .probe = emmc_clkc_probe, > + .driver = { > + .name = "emmc-clkc", > + .of_match_table = clkc_match_table, > + }, > +}; > + It should be a module, not a builtin - especially when the configuration is a tristate > +builtin_platform_driver(emmc_clkc_driver);