[{"id":1761963,"web_url":"http://patchwork.ozlabs.org/comment/1761963/","msgid":"<20170901202606.GX21656@codeaurora.org>","list_archive_url":null,"date":"2017-09-01T20:26:06","subject":"Re: [PATCH V2] clk: qcom: Add spmi_pmic clock divider support","submitter":{"id":6071,"url":"http://patchwork.ozlabs.org/api/people/6071/","name":"Stephen Boyd","email":"sboyd@codeaurora.org"},"content":"On 08/31, Tirupathi Reddy wrote:\n> diff --git a/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt\n> new file mode 100644\n> index 0000000..48cb2f7\n> --- /dev/null\n> +++ b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt\n> @@ -0,0 +1,49 @@\n> +Qualcomm Technologies, Inc. SPMI PMIC clock divider (clkdiv)\n> +\n> +clkdiv configures the clock frequency of a set of outputs on the PMIC.\n> +These clocks are typically wired through alternate functions on\n> +gpio pins.\n> +\n> +=======================\n> +Supported Properties\n\nJust \"Properties\".\n\n> +=======================\n> +\n> +- compatible\n> +\tUsage: required\n> +\tValue type: \n> +\tDefinition: should be \"qcom,spmi-pmic-clkdiv\".\n> +\n> +- reg\n> +\tUsage: required\n> +\tValue type: \n> +\tDefinition: Addresses and sizes for the memory of this CLKDIV\n> +\t\t peripheral.\n> +\n> +- clocks:\n> +\tUsage: required\n> +\tValue type: \n> +\tDefinition: reference to the xo clock.\n> +\n> +- clock-names:\n> +\tUsage: required\n> +\tValue type: \n> +\tDefinition: must be \"xo\".\n> +\n> +=======\n> +Example\n> +=======\n> +\n> +pm8998_clk_divs: qcom,clkdiv@0 {\n\n@5b00\n\n> +\tcompatible = \"qcom,spmi-pmic-clkdiv\";\n> +\treg = <0x5b00 0x300>;\n\nDrop the size\n\n> +\t#clock-cells = <1>;\n> +\tclocks = <&xo_board>;\n> +\tclock-names = \"xo\";\n> +\n> +\tassigned-clocks = <&pm8998_clk_divs CLKDIV1>,\n> +\t\t\t <&pm8998_clk_divs CLKDIV2>,\n> +\t\t\t <&pm8998_clk_divs CLKDIV3>;\n> +\tassigned-clock-rates = <9600000>,\n> +\t\t\t <9600000>,\n> +\t\t\t <9600000>;\n\nNot sure we need this part in the example, but OK.\n\n> +};\n> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig\n> index 9f6c278..af5e489 100644\n> --- a/drivers/clk/qcom/Kconfig\n> +++ b/drivers/clk/qcom/Kconfig\n> @@ -196,3 +196,12 @@ config MSM_MMCC_8996\n> \t Support for the multimedia clock controller on msm8996 devices.\n> \t Say Y if you want to support multimedia devices such as display,\n> \t graphics, video encode/decode, camera, etc.\n> +\n> +config CLOCK_SPMI_PMIC_DIV\n\nconfig SPMI_PMIC_CLKDIV?\n\n> +\ttristate \"spmi pmic clkdiv driver\"\n> +\tdepends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST\n> +\thelp\n> +\t This driver supports the clkdiv functionality on the Qualcomm\n> +\t Technologies, Inc. SPMI PMIC. It configures the frequency of\n> +\t clkdiv outputs on the PMIC. These clocks are typically wired\n> +\t through alternate functions on gpio pins.\n> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile\n> index 3f3aff2..ee8c91c 100644\n> --- a/drivers/clk/qcom/Makefile\n> +++ b/drivers/clk/qcom/Makefile\n> @@ -15,6 +15,7 @@ clk-qcom-$(CONFIG_QCOM_GDSC) += gdsc.o\n> # Keep alphabetically sorted by config\n> obj-$(CONFIG_APQ_GCC_8084) += gcc-apq8084.o\n> obj-$(CONFIG_APQ_MMCC_8084) += mmcc-apq8084.o\n> +obj-$(CONFIG_CLOCK_SPMI_PMIC_DIV) += clk-spmi-pmic-div.o\n> obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o\n> obj-$(CONFIG_IPQ_GCC_806X) += gcc-ipq806x.o\n> obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o\n> diff --git a/drivers/clk/qcom/clk-spmi-pmic-div.c b/drivers/clk/qcom/clk-spmi-pmic-div.c\n> new file mode 100644\n> index 0000000..24461fb\n> --- /dev/null\n> +++ b/drivers/clk/qcom/clk-spmi-pmic-div.c\n> @@ -0,0 +1,327 @@\n> +/* Copyright (c) 2017, The Linux Foundation. All rights reserved.\n> + *\n> + * This program is free software; you can redistribute it and/or modify\n> + * it under the terms of the GNU General Public License version 2 and\n> + * only version 2 as published by the Free Software Foundation.\n> + *\n> + * This program is distributed in the hope that it will be useful,\n> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n> + * GNU General Public License for more details.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n\nNeeded?\n\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +#define REG_DIV_CTL1\t\t\t0x43\n> +#define DIV_CTL1_DIV_FACTOR_MASK\tGENMASK(2, 0)\n> +\n> +#define REG_EN_CTL\t\t\t0x46\n> +#define REG_EN_MASK\t\t\tBIT(7)\n> +#define REG_EN_SET\t\t\tBIT(7)\n\nWhy do we need this define? Just test if the mask comes out to be\nnon-zero instead.\n\n> +\n> +#define ENABLE_DELAY_NS(cxo_ns, div)\t((2 + 3 * div) * cxo_ns)\n> +#define DISABLE_DELAY_NS(cxo_ns, div)\t((3 * div) * cxo_ns)\n> +\n> +#define CLK_SPMI_PMIC_DIV_OFFSET\t1\n> +\n> +#define CLKDIV_XO_DIV_1_0\t\t0\n> +#define CLKDIV_XO_DIV_1\t\t\t1\n> +#define CLKDIV_XO_DIV_2\t\t\t2\n> +#define CLKDIV_XO_DIV_4\t\t\t3\n> +#define CLKDIV_XO_DIV_8\t\t\t4\n> +#define CLKDIV_XO_DIV_16\t\t5\n> +#define CLKDIV_XO_DIV_32\t\t6\n> +#define CLKDIV_XO_DIV_64\t\t7\n> +#define CLKDIV_MAX_ALLOWED\t\t8\n> +\n> +struct clkdiv {\n> +\tstruct regmap\t\t*regmap;\n> +\tu16\t\t\tbase;\n> +\n> +\t/* clock properties */\n> +\tstruct clk_hw\t\thw;\n> +\tunsigned int\t\tdiv_factor;\n> +\tunsigned int\t\tcxo_period_ns;\n> +};\n> +\n> +struct spmi_pmic_div_clk_cc {\n> +\tstruct clk_hw\t**div_clks;\n\nI don't understand this design. Shouldn't we just allocate an\narray of clkdiv structure?\n\n> +\tint\t\tnclks;\n> +};\n> +\n> +static inline struct clkdiv *to_clkdiv(struct clk_hw *hw)\n> +{\n> +\treturn container_of(hw, struct clkdiv, hw);\n> +}\n> +\n> +static inline unsigned int div_factor_to_div(unsigned int div_factor)\n> +{\n> +\tif (div_factor == CLKDIV_XO_DIV_1_0)\n> +\t\treturn 1;\n> +\n> +\treturn 1 << (div_factor - CLK_SPMI_PMIC_DIV_OFFSET);\n> +}\n> +\n> +static inline unsigned int div_to_div_factor(unsigned int div)\n> +{\n> +\treturn ilog2(div) + CLK_SPMI_PMIC_DIV_OFFSET;\n\nMaybe we should do the clamp here?\n\n> +}\n> +\n> +static bool is_spmi_pmic_clkdiv_enabled(struct clkdiv *clkdiv)\n> +{\n> +\tunsigned int val = 0;\n> +\n> +\tregmap_read(clkdiv->regmap, clkdiv->base + REG_EN_CTL,\n> +\t\t\t &val);\n> +\treturn ((val & REG_EN_MASK) == REG_EN_SET) ? true : false;\n> +}\n> +\n> +static int spmi_pmic_clkdiv_set_enable_state(struct clkdiv *clkdiv,\n> +\t\t\tbool enable_state)\n> +{\n> +\tint rc;\n> +\n> +\trc = regmap_update_bits(clkdiv->regmap, clkdiv->base + REG_EN_CTL,\n> +\t\t\t\tREG_EN_MASK,\n> +\t\t\t\t(enable_state == true) ? REG_EN_SET : 0);\n> +\tif (rc)\n> +\t\treturn rc;\n> +\n> +\tif (enable_state == true)\n> +\t\tndelay(ENABLE_DELAY_NS(clkdiv->cxo_period_ns,\n> +\t\t\t\tdiv_factor_to_div(clkdiv->div_factor)));\n> +\telse\n> +\t\tndelay(DISABLE_DELAY_NS(clkdiv->cxo_period_ns,\n> +\t\t\t\tdiv_factor_to_div(clkdiv->div_factor)));\n> +\n> +\treturn rc;\n> +}\n> +\n> +static int spmi_pmic_clkdiv_config_freq_div(struct clkdiv *clkdiv,\n> +\t\t\tunsigned int div)\n> +{\n> +\tunsigned int div_factor;\n> +\tbool enabled;\n> +\tint rc;\n> +\n> +\tdiv_factor = div_to_div_factor(div);\n> +\tif (div_factor <= 0 || div_factor >= CLKDIV_MAX_ALLOWED)\n\ndiv_factor is unsigned. It can't be less than 0.\n\n> +\t\treturn -EINVAL;\n> +\n> +\tenabled = is_spmi_pmic_clkdiv_enabled(clkdiv);\n> +\tif (enabled) {\n> +\t\trc = spmi_pmic_clkdiv_set_enable_state(clkdiv, false);\n> +\t\tif (rc)\n> +\t\t\treturn rc;\n\nThis races with clk_enable() on the same clk. You'll need some\nsort of local lock to make sure set_rate() and enable don't race\nwith each other.\n\n> +\t}\n> +\n> +\trc = regmap_update_bits(clkdiv->regmap,\n> +\t\t\t\tclkdiv->base + REG_DIV_CTL1,\n> +\t\t\t\tDIV_CTL1_DIV_FACTOR_MASK, div_factor);\n> +\tif (rc)\n> +\t\treturn rc;\n> +\n> +\tclkdiv->div_factor = div_factor;\n> +\n> +\tif (enabled)\n> +\t\trc = spmi_pmic_clkdiv_set_enable_state(clkdiv, true);\n> +\n> +\treturn rc;\n> +}\n> +\n> +static int clk_spmi_pmic_div_enable(struct clk_hw *hw)\n> +{\n> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n> +\n> +\treturn spmi_pmic_clkdiv_set_enable_state(clkdiv, true);\n> +}\n> +\n> +static void clk_spmi_pmic_div_disable(struct clk_hw *hw)\n> +{\n> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n> +\n> +\tspmi_pmic_clkdiv_set_enable_state(clkdiv, false);\n> +}\n> +\n> +static long clk_spmi_pmic_div_round_rate(struct clk_hw *hw, unsigned long rate,\n> +\t\t\tunsigned long *parent_rate)\n> +{\n> +\tunsigned long new_rate;\n> +\tunsigned int div, div_factor;\n> +\n> +\tdiv = DIV_ROUND_UP(*parent_rate, rate);\n> +\tdiv_factor = div_to_div_factor(div);\n> +\tif (div_factor >= CLKDIV_MAX_ALLOWED)\n> +\t\tdiv_factor = CLKDIV_MAX_ALLOWED - 1;\n\nmin()?\n\n> +\tnew_rate = *parent_rate / div_factor_to_div(div_factor);\n> +\n> +\treturn new_rate;\n> +}\n> +\n> +static unsigned long clk_spmi_pmic_div_recalc_rate(struct clk_hw *hw,\n> +\t\t\tunsigned long parent_rate)\n> +{\n> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n> +\tunsigned long rate;\n> +\n> +\trate = parent_rate / div_factor_to_div(clkdiv->div_factor);\n> +\n> +\treturn rate;\n> +}\n> +\n> +static int clk_spmi_pmic_div_set_rate(struct clk_hw *hw, unsigned long rate,\n> +\t\t\tunsigned long parent_rate)\n> +{\n> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n> +\tint rc = 0;\n\nPlease don't assign variables that are then reassigned right\nafter.\n\n> +\n> +\trc = spmi_pmic_clkdiv_config_freq_div(clkdiv, parent_rate / rate);\n> +\n\nAnd also just return things when you can instead of assigning\nthem to local variables to return after.\n\n> +\treturn rc;\n> +}\n> +\n> +static const struct clk_ops clk_spmi_pmic_div_ops = {\n> +\t.enable = clk_spmi_pmic_div_enable,\n> +\t.disable = clk_spmi_pmic_div_disable,\n> +\t.set_rate = clk_spmi_pmic_div_set_rate,\n> +\t.recalc_rate = clk_spmi_pmic_div_recalc_rate,\n> +\t.round_rate = clk_spmi_pmic_div_round_rate,\n> +};\n> +\n> +static struct clk_hw *spmi_pmic_div_clk_hw_get(struct of_phandle_args *clkspec,\n> +\t\t\t\t void *data)\n> +{\n> +\tstruct spmi_pmic_div_clk_cc *clk_cc = data;\n> +\tunsigned int idx = clkspec->args[0];\n> +\n> +\tif (idx >= clk_cc->nclks) {\n> +\t\tpr_err(\"%s: invalid index %u\\n\", __func__, idx);\n> +\t\treturn ERR_PTR(-EINVAL);\n> +\t}\n> +\n> +\treturn clk_cc->div_clks[idx];\n> +}\n> +\n> +#define SPMI_PMIC_DIV_CLK_ADDRESS_RANGE\t0x100\n\nSPMI_PMIC_DIV_CLK_SIZE?\n\n> +#define SPMI_PMIC_DIV_CLK_ID_OFFSET\t1\n> +\n> +static int spmi_pmic_clkdiv_probe(struct platform_device *pdev)\n> +{\n> +\tstruct spmi_pmic_div_clk_cc *clk_cc;\n> +\tstruct clk_init_data init = {0};\n\nDrop the 0 part.\n\n> +\tstruct clkdiv *clkdiv;\n> +\tstruct clk *cxo;\n> +\tstruct regmap *regmap;\n> +\tstruct device *dev = &pdev->dev;\n> +\tconst char *parent_name;\n> +\tint nclks, i, rc, cxo_hz;\n> +\tu32 reg[2], start, size;\n> +\n> +\trc = of_property_read_u32_array(dev->of_node, \"reg\", reg, 2);\n\nOh hm... I think I suggested we count the size, but that may not\nwork. The SPMI node has #address-cells = 1 and #size-cells = 0.\nSo the reg property will only have one cell and it will be the\noffset. We could count the interrupts. Or we could count\nclock-output-names. Or we can match a certain PMIC compatible\nstring and then know the size from some hardcoded number in this\ndriver.\n\n> +\tif (rc < 0) {\n> +\t\tdev_err(dev, \"reg property reading failed\\n\");\n> +\t\treturn rc;\n> +\t}\n> +\tstart = reg[0];\n> +\tsize = reg[1];\n> +\n> +\tnclks = size / SPMI_PMIC_DIV_CLK_ADDRESS_RANGE;\n> +\tif (nclks == 0) {\n> +\t\tdev_err(dev, \"no div clocks assigned\\n\");\n\nThis should never happen?\n\n> +\t\treturn -ENODEV;\n> +\t}\n> +\n> +\tregmap = dev_get_regmap(dev->parent, NULL);\n> +\tif (!regmap) {\n> +\t\tdev_err(dev, \"Couldn't get parent's regmap\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tclkdiv = devm_kcalloc(dev, nclks, sizeof(*clkdiv), GFP_KERNEL);\n> +\tif (!clkdiv)\n> +\t\treturn -ENOMEM;\n> +\n> +\tclk_cc = devm_kzalloc(&pdev->dev, sizeof(*clk_cc), GFP_KERNEL);\n> +\tif (!clk_cc)\n> +\t\treturn -ENOMEM;\n> +\n> +\tclk_cc->div_clks = devm_kcalloc(&pdev->dev, nclks,\n> +\t\t\t\t\tsizeof(*clk_cc->div_clks), GFP_KERNEL);\n> +\tif (!clk_cc->div_clks)\n> +\t\treturn -ENOMEM;\n> +\n> +\t/* Read parent clock rate */\n\nThat's obvious.\n\n> +\tcxo = of_clk_get(dev->of_node, 0);\n\nJust use clk_get() instead? This is a platform driver after all.\n\n> +\tif (IS_ERR(cxo)) {\n> +\t\trc = PTR_ERR(cxo);\n> +\t\tif (rc != -EPROBE_DEFER)\n> +\t\t\tdev_err(dev, \"failed to get xo clock\");\n> +\t\treturn rc;\n> +\t}\n> +\tcxo_hz = clk_get_rate(cxo);\n> +\tclk_put(cxo);\n> +\n> +\tparent_name = of_clk_get_parent_name(dev->of_node, 0);\n> +\tif (!parent_name) {\n> +\t\tdev_err(dev, \"missing parent clock\\n\");\n> +\t\treturn -ENODEV;\n> +\t}\n> +\tdev_dbg(dev, \"parent is: %s\\n\", parent_name);\n\nRemove?\n\n> +\n> +\tinit.parent_names = &parent_name;\n> +\tinit.num_parents = parent_name ? 1 : 0;\n> +\tinit.ops = &clk_spmi_pmic_div_ops;\n> +\tinit.flags = 0;\n> +\n> +\tfor (i = 0; i < nclks; i++) {\n> +\t\tclkdiv[i].base = start + i * SPMI_PMIC_DIV_CLK_ADDRESS_RANGE;\n> +\t\tclkdiv[i].regmap = regmap;\n> +\t\tclkdiv[i].cxo_period_ns = NSEC_PER_SEC / cxo_hz;\n> +\t\tinit.name = kasprintf(GFP_KERNEL, \"div_clk%d\",\n> +\t\t\t\t i + SPMI_PMIC_DIV_CLK_ID_OFFSET);\n\nWe don't need a define for 1.\n\n> +\t\tclkdiv[i].hw.init = &init;\n> +\t\trc = devm_clk_hw_register(dev, &clkdiv[i].hw);\n> +\t\tif (rc)\n\nWe leak init.name here.\n\n> +\t\t\treturn rc;\n> +\n> +\t\tclk_cc->div_clks[i] = &clkdiv[i].hw;\n> +\t\tkfree(init.name); /* clock framework made a copy of the name */\n> +\t}\n> +\n> +\tclk_cc->nclks = nclks;\n> +\trc = of_clk_add_hw_provider(pdev->dev.of_node, spmi_pmic_div_clk_hw_get,\n> +\t\t\t\t clk_cc);\n\nUrgh, we really need this devmified. Patch coming tomorrow. For\nnow, you need a remove function for driver unbinding to\nunregister the provider.\n\n> +\treturn rc;\n> +}\n> +\n> +static const struct of_device_id spmi_pmic_clkdiv_match_table[] = {\n> +\t{ .compatible = \"qcom,spmi-pmic-clkdiv\" },\n\nI'm not sure we call it this in other places. How about\nqcom,spmi-clkdiv and then have specific one for the pmic as well\nin case we ever care in the future like qcom,pm8916-clkdiv, etc.\n\n> +\t{}\n> +};\n> +MODULE_DEVICE_TABLE(of, spmi_pmic_clkdiv_match_table);\n> +\n> +static struct platform_driver spmi_pmic_clkdiv_driver = {\n> +\t.driver\t\t= {\n> +\t\t.name\t= \"qcom,spmi-pmic-clkdiv\",\n> +\t\t.of_match_table = spmi_pmic_clkdiv_match_table,\n> +\t},\n> +\t.probe\t\t= spmi_pmic_clkdiv_probe,\n> +};\n> +module_platform_driver(spmi_pmic_clkdiv_driver);\n> +\n> +MODULE_DESCRIPTION(\"spmi pmic clkdiv driver\");\n\nQCOM SPMI PMIC clkdiv driver?\n\n> +MODULE_LICENSE(\"GPL v2\");\n> diff --git a/include/dt-bindings/clock/spmi_pmic_clk_div.h b/include/dt-bindings/clock/spmi_pmic_clk_div.h\n> new file mode 100644\n> index 0000000..25d035d\n> --- /dev/null\n> +++ b/include/dt-bindings/clock/spmi_pmic_clk_div.h\n> @@ -0,0 +1,21 @@\n> +/*\n> + * Copyright (c) 2017, The Linux Foundation. All rights reserved.\n> + *\n> + * This software is licensed under the terms of the GNU General Public\n> + * License version 2, as published by the Free Software Foundation, and\n> + * may be copied, distributed, and modified under those terms.\n> + *\n> + * This program is distributed in the hope that it will be useful,\n> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n> + * GNU General Public License for more details.\n> + */\n> +\n> +#ifndef _DT_BINDINGS_SPMI_PMIC_CLK_DIV_H\n> +#define _DT_BINDINGS_SPMI_PMIC_CLK_DIV_H\n> +\n> +#define CLKDIV1\t\t0\n\n1 is 0...\n\n> +#define CLKDIV2\t\t1\n\n2 is 1...\n\n> +#define CLKDIV3\t\t2\n\nHow about we offset the index once and then drop the header file?","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=codeaurora.org header.i=@codeaurora.org\n\theader.b=\"lG+dy/4/\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key)\n\theader.d=codeaurora.org header.i=@codeaurora.org header.b=\"CnSUu+D0\"; \n\tdkim-atps=neutral","pdx-caf-mail.web.codeaurora.org;\n\tdmarc=none (p=none dis=none) header.from=codeaurora.org","pdx-caf-mail.web.codeaurora.org;\n\tspf=none smtp.mailfrom=sboyd@codeaurora.org"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xkW3V4HG4z9sPt\n\tfor ;\n\tSat, 2 Sep 2017 06:26:14 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1752350AbdIAU0M (ORCPT\n\t);\n\tFri, 1 Sep 2017 16:26:12 -0400","from smtp.codeaurora.org ([198.145.29.96]:48486 \"EHLO\n\tsmtp.codeaurora.org\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1752306AbdIAU0K (ORCPT\n\t); Fri, 1 Sep 2017 16:26:10 -0400","by smtp.codeaurora.org (Postfix, from userid 1000)\n\tid 2A803608CF; Fri, 1 Sep 2017 20:26:10 +0000 (UTC)","from localhost (i-global254.qualcomm.com [199.106.103.254])\n\t(using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits))\n\t(No client certificate requested)\n\t(Authenticated sender: sboyd@smtp.codeaurora.org)\n\tby smtp.codeaurora.org (Postfix) with ESMTPSA id 80054607C9;\n\tFri, 1 Sep 2017 20:26:07 +0000 (UTC)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org;\n\ts=default; t=1504297570;\n\tbh=t6F81s4wVknpci1vcx6lLeqhLO/RuIxEzVKdrB47gpE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lG+dy/4/fbu0MUYyT3V3KtXQq/YXghzDfH1VNXDv0Mi5WKQA65KZUjvy5qS/pbycj\n\t47n/hcnefbL2QQ2GfhOiteOnIjAXseNAWlJkDD1lHZPJZaNwPbfPKgu7CvpfhWljo/\n\t0kH/bUB6ffKW+U9tCSDhZJElp0DS0Gn0pKG6gUfs=","v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org;\n\ts=default; t=1504297567;\n\tbh=t6F81s4wVknpci1vcx6lLeqhLO/RuIxEzVKdrB47gpE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CnSUu+D08vMkuJyPHkUcWiG2aC7JV3lta7cojfYDn/Ev2CnGGLc6tB+DrcBaarmvO\n\t+FeKS6i0DLh2HlQgrv6AZ1PqopKJDRxb8zhHXaYsKmI7KujsZbT2PQ+LJIUjZgoUNA\n\tayWbCml+KyLl6c2eJiiE6+QJaSnRFWac4F0jx19A="],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tpdx-caf-mail.web.codeaurora.org","X-Spam-Level":"","X-Spam-Status":"No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00,\n\tDKIM_SIGNED,\n\tT_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0","DMARC-Filter":"OpenDMARC Filter v1.3.2 smtp.codeaurora.org 80054607C9","Date":"Fri, 1 Sep 2017 13:26:06 -0700","From":"Stephen Boyd ","To":"Tirupathi Reddy ","Cc":"mturquette@baylibre.com, robh+dt@kernel.org, mark.rutland@arm.com,\n\tandy.gross@linaro.org, david.brown@linaro.org,\n\tlinux-clk@vger.kernel.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,\n\tlinux-soc@vger.kernel.org","Subject":"Re: [PATCH V2] clk: qcom: Add spmi_pmic clock divider support","Message-ID":"<20170901202606.GX21656@codeaurora.org>","References":"<1504176090-11544-1-git-send-email-tirupath@codeaurora.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<1504176090-11544-1-git-send-email-tirupath@codeaurora.org>","User-Agent":"Mutt/1.5.21 (2010-09-15)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1764054,"web_url":"http://patchwork.ozlabs.org/comment/1764054/","msgid":"","list_archive_url":null,"date":"2017-09-06T12:00:25","subject":"Re: [PATCH V2] clk: qcom: Add spmi_pmic clock divider support","submitter":{"id":71968,"url":"http://patchwork.ozlabs.org/api/people/71968/","name":"Tirupathi Reddy","email":"tirupath@codeaurora.org"},"content":"On 9/2/2017 1:56 AM, Stephen Boyd wrote:\n> On 08/31, Tirupathi Reddy wrote:\n>> diff --git a/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt\n>> new file mode 100644\n>> index 0000000..48cb2f7\n>> --- /dev/null\n>> +++ b/Documentation/devicetree/bindings/clock/clk-spmi-pmic-div.txt\n>> @@ -0,0 +1,49 @@\n>> +Qualcomm Technologies, Inc. SPMI PMIC clock divider (clkdiv)\n>> +\n>> +clkdiv configures the clock frequency of a set of outputs on the PMIC.\n>> +These clocks are typically wired through alternate functions on\n>> +gpio pins.\n>> +\n>> +=======================\n>> +Supported Properties\n> Just \"Properties\".\naddress in next patch set.\n>\n>> +=======================\n>> +\n>> +- compatible\n>> +\tUsage: required\n>> +\tValue type: \n>> +\tDefinition: should be \"qcom,spmi-pmic-clkdiv\".\n>> +\n>> +- reg\n>> +\tUsage: required\n>> +\tValue type: \n>> +\tDefinition: Addresses and sizes for the memory of this CLKDIV\n>> +\t\t peripheral.\n>> +\n>> +- clocks:\n>> +\tUsage: required\n>> +\tValue type: \n>> +\tDefinition: reference to the xo clock.\n>> +\n>> +- clock-names:\n>> +\tUsage: required\n>> +\tValue type: \n>> +\tDefinition: must be \"xo\".\n>> +\n>> +=======\n>> +Example\n>> +=======\n>> +\n>> +pm8998_clk_divs: qcom,clkdiv@0 {\n> @5b00\naddress in next patch set.\n>\n>> +\tcompatible = \"qcom,spmi-pmic-clkdiv\";\n>> +\treg = <0x5b00 0x300>;\n> Drop the size\naddress in next patch set.\n>\n>> +\t#clock-cells = <1>;\n>> +\tclocks = <&xo_board>;\n>> +\tclock-names = \"xo\";\n>> +\n>> +\tassigned-clocks = <&pm8998_clk_divs CLKDIV1>,\n>> +\t\t\t <&pm8998_clk_divs CLKDIV2>,\n>> +\t\t\t <&pm8998_clk_divs CLKDIV3>;\n>> +\tassigned-clock-rates = <9600000>,\n>> +\t\t\t <9600000>,\n>> +\t\t\t <9600000>;\n> Not sure we need this part in the example, but OK.\n>\n>> +};\n>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig\n>> index 9f6c278..af5e489 100644\n>> --- a/drivers/clk/qcom/Kconfig\n>> +++ b/drivers/clk/qcom/Kconfig\n>> @@ -196,3 +196,12 @@ config MSM_MMCC_8996\n>> \t Support for the multimedia clock controller on msm8996 devices.\n>> \t Say Y if you want to support multimedia devices such as display,\n>> \t graphics, video encode/decode, camera, etc.\n>> +\n>> +config CLOCK_SPMI_PMIC_DIV\n> config SPMI_PMIC_CLKDIV?\naddress in next patch set.\n>\n>> +\ttristate \"spmi pmic clkdiv driver\"\n>> +\tdepends on (COMMON_CLK_QCOM && SPMI) || COMPILE_TEST\n>> +\thelp\n>> +\t This driver supports the clkdiv functionality on the Qualcomm\n>> +\t Technologies, Inc. SPMI PMIC. It configures the frequency of\n>> +\t clkdiv outputs on the PMIC. These clocks are typically wired\n>> +\t through alternate functions on gpio pins.\n>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile\n>> index 3f3aff2..ee8c91c 100644\n>> --- a/drivers/clk/qcom/Makefile\n>> +++ b/drivers/clk/qcom/Makefile\n>> @@ -15,6 +15,7 @@ clk-qcom-$(CONFIG_QCOM_GDSC) += gdsc.o\n>> # Keep alphabetically sorted by config\n>> obj-$(CONFIG_APQ_GCC_8084) += gcc-apq8084.o\n>> obj-$(CONFIG_APQ_MMCC_8084) += mmcc-apq8084.o\n>> +obj-$(CONFIG_CLOCK_SPMI_PMIC_DIV) += clk-spmi-pmic-div.o\n>> obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o\n>> obj-$(CONFIG_IPQ_GCC_806X) += gcc-ipq806x.o\n>> obj-$(CONFIG_IPQ_GCC_8074) += gcc-ipq8074.o\n>> diff --git a/drivers/clk/qcom/clk-spmi-pmic-div.c b/drivers/clk/qcom/clk-spmi-pmic-div.c\n>> new file mode 100644\n>> index 0000000..24461fb\n>> --- /dev/null\n>> +++ b/drivers/clk/qcom/clk-spmi-pmic-div.c\n>> @@ -0,0 +1,327 @@\n>> +/* Copyright (c) 2017, The Linux Foundation. All rights reserved.\n>> + *\n>> + * This program is free software; you can redistribute it and/or modify\n>> + * it under the terms of the GNU General Public License version 2 and\n>> + * only version 2 as published by the Free Software Foundation.\n>> + *\n>> + * This program is distributed in the hope that it will be useful,\n>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n>> + * GNU General Public License for more details.\n>> + */\n>> +\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +#include \n> Needed?\nremove it in next patch set.\n>\n>> +#include \n>> +#include \n>> +#include \n>> +#include \n>> +\n>> +#include \n>> +\n>> +#define REG_DIV_CTL1\t\t\t0x43\n>> +#define DIV_CTL1_DIV_FACTOR_MASK\tGENMASK(2, 0)\n>> +\n>> +#define REG_EN_CTL\t\t\t0x46\n>> +#define REG_EN_MASK\t\t\tBIT(7)\n>> +#define REG_EN_SET\t\t\tBIT(7)\n> Why do we need this define? Just test if the mask comes out to be\n> non-zero instead.\nokay. address in next patch set.\n>\n>> +\n>> +#define ENABLE_DELAY_NS(cxo_ns, div)\t((2 + 3 * div) * cxo_ns)\n>> +#define DISABLE_DELAY_NS(cxo_ns, div)\t((3 * div) * cxo_ns)\n>> +\n>> +#define CLK_SPMI_PMIC_DIV_OFFSET\t1\n>> +\n>> +#define CLKDIV_XO_DIV_1_0\t\t0\n>> +#define CLKDIV_XO_DIV_1\t\t\t1\n>> +#define CLKDIV_XO_DIV_2\t\t\t2\n>> +#define CLKDIV_XO_DIV_4\t\t\t3\n>> +#define CLKDIV_XO_DIV_8\t\t\t4\n>> +#define CLKDIV_XO_DIV_16\t\t5\n>> +#define CLKDIV_XO_DIV_32\t\t6\n>> +#define CLKDIV_XO_DIV_64\t\t7\n>> +#define CLKDIV_MAX_ALLOWED\t\t8\n>> +\n>> +struct clkdiv {\n>> +\tstruct regmap\t\t*regmap;\n>> +\tu16\t\t\tbase;\n>> +\n>> +\t/* clock properties */\n>> +\tstruct clk_hw\t\thw;\n>> +\tunsigned int\t\tdiv_factor;\n>> +\tunsigned int\t\tcxo_period_ns;\n>> +};\n>> +\n>> +struct spmi_pmic_div_clk_cc {\n>> +\tstruct clk_hw\t**div_clks;\n> I don't understand this design. Shouldn't we just allocate an\n> array of clkdiv structure?\nThis design is for addressing the invalid value passed in \nspmi_pmic_div_clk_hw_get().\n>\n>> +\tint\t\tnclks;\n>> +};\n>> +\n>> +static inline struct clkdiv *to_clkdiv(struct clk_hw *hw)\n>> +{\n>> +\treturn container_of(hw, struct clkdiv, hw);\n>> +}\n>> +\n>> +static inline unsigned int div_factor_to_div(unsigned int div_factor)\n>> +{\n>> +\tif (div_factor == CLKDIV_XO_DIV_1_0)\n>> +\t\treturn 1;\n>> +\n>> +\treturn 1 << (div_factor - CLK_SPMI_PMIC_DIV_OFFSET);\n>> +}\n>> +\n>> +static inline unsigned int div_to_div_factor(unsigned int div)\n>> +{\n>> +\treturn ilog2(div) + CLK_SPMI_PMIC_DIV_OFFSET;\n> Maybe we should do the clamp here?\nokay. address in next patch set.\n>\n>> +}\n>> +\n>> +static bool is_spmi_pmic_clkdiv_enabled(struct clkdiv *clkdiv)\n>> +{\n>> +\tunsigned int val = 0;\n>> +\n>> +\tregmap_read(clkdiv->regmap, clkdiv->base + REG_EN_CTL,\n>> +\t\t\t &val);\n>> +\treturn ((val & REG_EN_MASK) == REG_EN_SET) ? true : false;\n>> +}\n>> +\n>> +static int spmi_pmic_clkdiv_set_enable_state(struct clkdiv *clkdiv,\n>> +\t\t\tbool enable_state)\n>> +{\n>> +\tint rc;\n>> +\n>> +\trc = regmap_update_bits(clkdiv->regmap, clkdiv->base + REG_EN_CTL,\n>> +\t\t\t\tREG_EN_MASK,\n>> +\t\t\t\t(enable_state == true) ? REG_EN_SET : 0);\n>> +\tif (rc)\n>> +\t\treturn rc;\n>> +\n>> +\tif (enable_state == true)\n>> +\t\tndelay(ENABLE_DELAY_NS(clkdiv->cxo_period_ns,\n>> +\t\t\t\tdiv_factor_to_div(clkdiv->div_factor)));\n>> +\telse\n>> +\t\tndelay(DISABLE_DELAY_NS(clkdiv->cxo_period_ns,\n>> +\t\t\t\tdiv_factor_to_div(clkdiv->div_factor)));\n>> +\n>> +\treturn rc;\n>> +}\n>> +\n>> +static int spmi_pmic_clkdiv_config_freq_div(struct clkdiv *clkdiv,\n>> +\t\t\tunsigned int div)\n>> +{\n>> +\tunsigned int div_factor;\n>> +\tbool enabled;\n>> +\tint rc;\n>> +\n>> +\tdiv_factor = div_to_div_factor(div);\n>> +\tif (div_factor <= 0 || div_factor >= CLKDIV_MAX_ALLOWED)\n> div_factor is unsigned. It can't be less than 0.\nokay. address in next patch set.\n>\n>> +\t\treturn -EINVAL;\n>> +\n>> +\tenabled = is_spmi_pmic_clkdiv_enabled(clkdiv);\n>> +\tif (enabled) {\n>> +\t\trc = spmi_pmic_clkdiv_set_enable_state(clkdiv, false);\n>> +\t\tif (rc)\n>> +\t\t\treturn rc;\n> This races with clk_enable() on the same clk. You'll need some\n> sort of local lock to make sure set_rate() and enable don't race\n> with each other.\nokay. address in next patch set.\n>> +\t}\n>> +\n>> +\trc = regmap_update_bits(clkdiv->regmap,\n>> +\t\t\t\tclkdiv->base + REG_DIV_CTL1,\n>> +\t\t\t\tDIV_CTL1_DIV_FACTOR_MASK, div_factor);\n>> +\tif (rc)\n>> +\t\treturn rc;\n>> +\n>> +\tclkdiv->div_factor = div_factor;\n>> +\n>> +\tif (enabled)\n>> +\t\trc = spmi_pmic_clkdiv_set_enable_state(clkdiv, true);\n>> +\n>> +\treturn rc;\n>> +}\n>> +\n>> +static int clk_spmi_pmic_div_enable(struct clk_hw *hw)\n>> +{\n>> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n>> +\n>> +\treturn spmi_pmic_clkdiv_set_enable_state(clkdiv, true);\n>> +}\n>> +\n>> +static void clk_spmi_pmic_div_disable(struct clk_hw *hw)\n>> +{\n>> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n>> +\n>> +\tspmi_pmic_clkdiv_set_enable_state(clkdiv, false);\n>> +}\n>> +\n>> +static long clk_spmi_pmic_div_round_rate(struct clk_hw *hw, unsigned long rate,\n>> +\t\t\tunsigned long *parent_rate)\n>> +{\n>> +\tunsigned long new_rate;\n>> +\tunsigned int div, div_factor;\n>> +\n>> +\tdiv = DIV_ROUND_UP(*parent_rate, rate);\n>> +\tdiv_factor = div_to_div_factor(div);\n>> +\tif (div_factor >= CLKDIV_MAX_ALLOWED)\n>> +\t\tdiv_factor = CLKDIV_MAX_ALLOWED - 1;\n> min()?\nokay. address in next patch set.\n>\n>> +\tnew_rate = *parent_rate / div_factor_to_div(div_factor);\n>> +\n>> +\treturn new_rate;\n>> +}\n>> +\n>> +static unsigned long clk_spmi_pmic_div_recalc_rate(struct clk_hw *hw,\n>> +\t\t\tunsigned long parent_rate)\n>> +{\n>> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n>> +\tunsigned long rate;\n>> +\n>> +\trate = parent_rate / div_factor_to_div(clkdiv->div_factor);\n>> +\n>> +\treturn rate;\n>> +}\n>> +\n>> +static int clk_spmi_pmic_div_set_rate(struct clk_hw *hw, unsigned long rate,\n>> +\t\t\tunsigned long parent_rate)\n>> +{\n>> +\tstruct clkdiv *clkdiv = to_clkdiv(hw);\n>> +\tint rc = 0;\n> Please don't assign variables that are then reassigned right\n> after.\nokay. address in next patch set.\n>\n>> +\n>> +\trc = spmi_pmic_clkdiv_config_freq_div(clkdiv, parent_rate / rate);\n>> +\n> And also just return things when you can instead of assigning\n> them to local variables to return after.\nokay. address in next patch set.\n>\n>> +\treturn rc;\n>> +}\n>> +\n>> +static const struct clk_ops clk_spmi_pmic_div_ops = {\n>> +\t.enable = clk_spmi_pmic_div_enable,\n>> +\t.disable = clk_spmi_pmic_div_disable,\n>> +\t.set_rate = clk_spmi_pmic_div_set_rate,\n>> +\t.recalc_rate = clk_spmi_pmic_div_recalc_rate,\n>> +\t.round_rate = clk_spmi_pmic_div_round_rate,\n>> +};\n>> +\n>> +static struct clk_hw *spmi_pmic_div_clk_hw_get(struct of_phandle_args *clkspec,\n>> +\t\t\t\t void *data)\n>> +{\n>> +\tstruct spmi_pmic_div_clk_cc *clk_cc = data;\n>> +\tunsigned int idx = clkspec->args[0];\n>> +\n>> +\tif (idx >= clk_cc->nclks) {\n>> +\t\tpr_err(\"%s: invalid index %u\\n\", __func__, idx);\n>> +\t\treturn ERR_PTR(-EINVAL);\n>> +\t}\n>> +\n>> +\treturn clk_cc->div_clks[idx];\n>> +}\n>> +\n>> +#define SPMI_PMIC_DIV_CLK_ADDRESS_RANGE\t0x100\n> SPMI_PMIC_DIV_CLK_SIZE?\nokay. address in next patch set.\n>\n>> +#define SPMI_PMIC_DIV_CLK_ID_OFFSET\t1\n>> +\n>> +static int spmi_pmic_clkdiv_probe(struct platform_device *pdev)\n>> +{\n>> +\tstruct spmi_pmic_div_clk_cc *clk_cc;\n>> +\tstruct clk_init_data init = {0};\n> Drop the 0 part.\nokay. address in next patch set.\n>\n>> +\tstruct clkdiv *clkdiv;\n>> +\tstruct clk *cxo;\n>> +\tstruct regmap *regmap;\n>> +\tstruct device *dev = &pdev->dev;\n>> +\tconst char *parent_name;\n>> +\tint nclks, i, rc, cxo_hz;\n>> +\tu32 reg[2], start, size;\n>> +\n>> +\trc = of_property_read_u32_array(dev->of_node, \"reg\", reg, 2);\n> Oh hm... I think I suggested we count the size, but that may not\n> work. The SPMI node has #address-cells = 1 and #size-cells = 0.\n> So the reg property will only have one cell and it will be the\n> offset. We could count the interrupts. Or we could count\n> clock-output-names. Or we can match a certain PMIC compatible\n> string and then know the size from some hardcoded number in this\n> driver.\nokay. address in next patch set.\n>\n>> +\tif (rc < 0) {\n>> +\t\tdev_err(dev, \"reg property reading failed\\n\");\n>> +\t\treturn rc;\n>> +\t}\n>> +\tstart = reg[0];\n>> +\tsize = reg[1];\n>> +\n>> +\tnclks = size / SPMI_PMIC_DIV_CLK_ADDRESS_RANGE;\n>> +\tif (nclks == 0) {\n>> +\t\tdev_err(dev, \"no div clocks assigned\\n\");\n> This should never happen?\nokay. remove it in next patch set.\n>\n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\n>> +\tregmap = dev_get_regmap(dev->parent, NULL);\n>> +\tif (!regmap) {\n>> +\t\tdev_err(dev, \"Couldn't get parent's regmap\\n\");\n>> +\t\treturn -EINVAL;\n>> +\t}\n>> +\n>> +\tclkdiv = devm_kcalloc(dev, nclks, sizeof(*clkdiv), GFP_KERNEL);\n>> +\tif (!clkdiv)\n>> +\t\treturn -ENOMEM;\n>> +\n>> +\tclk_cc = devm_kzalloc(&pdev->dev, sizeof(*clk_cc), GFP_KERNEL);\n>> +\tif (!clk_cc)\n>> +\t\treturn -ENOMEM;\n>> +\n>> +\tclk_cc->div_clks = devm_kcalloc(&pdev->dev, nclks,\n>> +\t\t\t\t\tsizeof(*clk_cc->div_clks), GFP_KERNEL);\n>> +\tif (!clk_cc->div_clks)\n>> +\t\treturn -ENOMEM;\n>> +\n>> +\t/* Read parent clock rate */\n> That's obvious.\nokay. remove it in next patch set.\n>\n>> +\tcxo = of_clk_get(dev->of_node, 0);\n> Just use clk_get() instead? This is a platform driver after all.\nokay. address in next patch set.\n>\n>> +\tif (IS_ERR(cxo)) {\n>> +\t\trc = PTR_ERR(cxo);\n>> +\t\tif (rc != -EPROBE_DEFER)\n>> +\t\t\tdev_err(dev, \"failed to get xo clock\");\n>> +\t\treturn rc;\n>> +\t}\n>> +\tcxo_hz = clk_get_rate(cxo);\n>> +\tclk_put(cxo);\n>> +\n>> +\tparent_name = of_clk_get_parent_name(dev->of_node, 0);\n>> +\tif (!parent_name) {\n>> +\t\tdev_err(dev, \"missing parent clock\\n\");\n>> +\t\treturn -ENODEV;\n>> +\t}\n>> +\tdev_dbg(dev, \"parent is: %s\\n\", parent_name);\n> Remove?\nokay. address in next patch set.\n>\n>> +\n>> +\tinit.parent_names = &parent_name;\n>> +\tinit.num_parents = parent_name ? 1 : 0;\n>> +\tinit.ops = &clk_spmi_pmic_div_ops;\n>> +\tinit.flags = 0;\n>> +\n>> +\tfor (i = 0; i < nclks; i++) {\n>> +\t\tclkdiv[i].base = start + i * SPMI_PMIC_DIV_CLK_ADDRESS_RANGE;\n>> +\t\tclkdiv[i].regmap = regmap;\n>> +\t\tclkdiv[i].cxo_period_ns = NSEC_PER_SEC / cxo_hz;\n>> +\t\tinit.name = kasprintf(GFP_KERNEL, \"div_clk%d\",\n>> +\t\t\t\t i + SPMI_PMIC_DIV_CLK_ID_OFFSET);\n> We don't need a define for 1.\n>\n>> +\t\tclkdiv[i].hw.init = &init;\n>> +\t\trc = devm_clk_hw_register(dev, &clkdiv[i].hw);\n>> +\t\tif (rc)\n> We leak init.name here.\n>\n>> +\t\t\treturn rc;\n>> +\n>> +\t\tclk_cc->div_clks[i] = &clkdiv[i].hw;\n>> +\t\tkfree(init.name); /* clock framework made a copy of the name */\n>> +\t}\n>> +\n>> +\tclk_cc->nclks = nclks;\n>> +\trc = of_clk_add_hw_provider(pdev->dev.of_node, spmi_pmic_div_clk_hw_get,\n>> +\t\t\t\t clk_cc);\n> Urgh, we really need this devmified. Patch coming tomorrow. For\n> now, you need a remove function for driver unbinding to\n> unregister the provider.\nokay. address in next patch set.\n>> +\treturn rc;\n>> +}\n>> +\n>> +static const struct of_device_id spmi_pmic_clkdiv_match_table[] = {\n>> +\t{ .compatible = \"qcom,spmi-pmic-clkdiv\" },\n> I'm not sure we call it this in other places. How about\n> qcom,spmi-clkdiv and then have specific one for the pmic as well\n> in case we ever care in the future like qcom,pm8916-clkdiv, etc.\nokay. address in next patch set.\n>\n>> +\t{}\n>> +};\n>> +MODULE_DEVICE_TABLE(of, spmi_pmic_clkdiv_match_table);\n>> +\n>> +static struct platform_driver spmi_pmic_clkdiv_driver = {\n>> +\t.driver\t\t= {\n>> +\t\t.name\t= \"qcom,spmi-pmic-clkdiv\",\n>> +\t\t.of_match_table = spmi_pmic_clkdiv_match_table,\n>> +\t},\n>> +\t.probe\t\t= spmi_pmic_clkdiv_probe,\n>> +};\n>> +module_platform_driver(spmi_pmic_clkdiv_driver);\n>> +\n>> +MODULE_DESCRIPTION(\"spmi pmic clkdiv driver\");\n> QCOM SPMI PMIC clkdiv driver?\nokay. address in next patch set.\n>\n>> +MODULE_LICENSE(\"GPL v2\");\n>> diff --git a/include/dt-bindings/clock/spmi_pmic_clk_div.h b/include/dt-bindings/clock/spmi_pmic_clk_div.h\n>> new file mode 100644\n>> index 0000000..25d035d\n>> --- /dev/null\n>> +++ b/include/dt-bindings/clock/spmi_pmic_clk_div.h\n>> @@ -0,0 +1,21 @@\n>> +/*\n>> + * Copyright (c) 2017, The Linux Foundation. All rights reserved.\n>> + *\n>> + * This software is licensed under the terms of the GNU General Public\n>> + * License version 2, as published by the Free Software Foundation, and\n>> + * may be copied, distributed, and modified under those terms.\n>> + *\n>> + * This program is distributed in the hope that it will be useful,\n>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of\n>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the\n>> + * GNU General Public License for more details.\n>> + */\n>> +\n>> +#ifndef _DT_BINDINGS_SPMI_PMIC_CLK_DIV_H\n>> +#define _DT_BINDINGS_SPMI_PMIC_CLK_DIV_H\n>> +\n>> +#define CLKDIV1\t\t0\n> 1 is 0...\n>\n>> +#define CLKDIV2\t\t1\n> 2 is 1...\n>\n>> +#define CLKDIV3\t\t2\n> How about we offset the index once and then drop the header file?\nokay. address in next patch set.\n>\n\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=codeaurora.org header.i=@codeaurora.org\n\theader.b=\"UIDn7shQ\"; \n\tdkim=fail reason=\"signature verification failed\" (1024-bit key)\n\theader.d=codeaurora.org header.i=@codeaurora.org header.b=\"A/QuvLZR\"; \n\tdkim-atps=neutral","pdx-caf-mail.web.codeaurora.org;\n\tdmarc=none (p=none dis=none) header.from=codeaurora.org","pdx-caf-mail.web.codeaurora.org;\n\tspf=none smtp.mailfrom=tirupath@codeaurora.org"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xnMbp6XjTz9sCZ\n\tfor ;\n\tWed, 6 Sep 2017 22:00:38 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1753228AbdIFMAh (ORCPT\n\t);\n\tWed, 6 Sep 2017 08:00:37 -0400","from smtp.codeaurora.org ([198.145.29.96]:52070 \"EHLO\n\tsmtp.codeaurora.org\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1753347AbdIFMAe (ORCPT\n\t); Wed, 6 Sep 2017 08:00:34 -0400","by smtp.codeaurora.org (Postfix, from userid 1000)\n\tid E0372605A5; Wed, 6 Sep 2017 12:00:33 +0000 (UTC)","from [10.206.28.109]\n\t(blr-c-bdr-fw-01_globalnat_allzones-outside.qualcomm.com\n\t[103.229.19.19])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\t(Authenticated sender: tirupath@smtp.codeaurora.org)\n\tby smtp.codeaurora.org (Postfix) with ESMTPSA id D6D19605A5;\n\tWed, 6 Sep 2017 12:00:27 +0000 (UTC)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org;\n\ts=default; t=1504699233;\n\tbh=GAn/6TEpZ9AmVdku/92oKEaG492ULHl5WMjpjdE66ng=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=UIDn7shQoMqegPvwLSDqXeSdRdv69KmUFypRhRp41iCSWaN7urvXYdsAin+Whg2cJ\n\tBMyaemGhKMwXUp4lZ3T2ht+olApkFM07HlpPqXhX1KBAupAf88+NP+IRvIFVQeAgor\n\tlUdS3SadmQrQZNBi+K+eqhRy1Mtn/k+28gQdfHvc=","v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org;\n\ts=default; t=1504699231;\n\tbh=GAn/6TEpZ9AmVdku/92oKEaG492ULHl5WMjpjdE66ng=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=A/QuvLZRi66dr9z/Nni3U68a+MrFUNWXqpJ3jqcy4mFb4Xv4htrdpHq2gHJO5dLcG\n\tCqpVp+fpHgMTCPuWW8S4AJ0njLLKGQko5G5NZui2IEThtTOaulAlxU/M5zJbZr4+ra\n\txomUbpD8JbDEVp4FTnguSEcuziTdY+dU//t/il+w="],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tpdx-caf-mail.web.codeaurora.org","X-Spam-Level":"","X-Spam-Status":"No, score=-2.8 required=2.0 tests=ALL_TRUSTED,BAYES_00,\n\tDKIM_SIGNED,\n\tT_DKIM_INVALID autolearn=no autolearn_force=no version=3.4.0","DMARC-Filter":"OpenDMARC Filter v1.3.2 smtp.codeaurora.org D6D19605A5","Subject":"Re: [PATCH V2] clk: qcom: Add spmi_pmic clock divider support","To":"Stephen Boyd ","Cc":"mturquette@baylibre.com, robh+dt@kernel.org, mark.rutland@arm.com,\n\tandy.gross@linaro.org, david.brown@linaro.org,\n\tlinux-clk@vger.kernel.org, devicetree@vger.kernel.org,\n\tlinux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,\n\tlinux-soc@vger.kernel.org","References":"<1504176090-11544-1-git-send-email-tirupath@codeaurora.org>\n\t<20170901202606.GX21656@codeaurora.org>","From":"Tirupathi Reddy T ","Message-ID":"","Date":"Wed, 6 Sep 2017 17:30:25 +0530","User-Agent":"Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170901202606.GX21656@codeaurora.org>","Content-Type":"text/plain; charset=utf-8; format=flowed","Content-Transfer-Encoding":"7bit","Content-Language":"en-US","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"devicetree@vger.kernel.org"}}]