[PATCH 0/4] clk: 88pm800: Add new clk provider driver for 88PM800 MFD

[PATCH 0/4] clk: 88pm800: Add new clk provider driver for 88PM800 MFD

[Vaibhav Hiremath]

88PM800 family of devices provides multiple buffered 32.768 KHz clock
output.
88PM800 : CLK32k_1, CLK32k_2 and CLK32k_3
88PM860 : CLK32k_1 and CLK32k_2

This patch-series adds new clock provider driver for enabling/disabling
buffered 32Khz clock output from 88PM800 family of device. Adds clock device to
MFD and updates the binding documentation.

The PATCH [1/4], created separately, as it would be easy between maintainers to
queue it.


Note that, I have access to only 88PM800 and 88PM860 datasheet,
so enabling support for only above two devices.

Testing:
  - Boot tested on 88PM860 based platform
  - Clock enable/disable
  - Non PM860, to make sure that it won't lead to any issues


Vaibhav Hiremath (4):
  mfd: 88pm800: Update the header file with 32K clk related macros
  mfd: devicetree: bindings: Add clock subdevice node information
  clk: 88pm800: Add clk provider driver for 88pm800 family of devices
  mfd: 88pm800: Add support for clk subdevice

 Documentation/devicetree/bindings/mfd/88pm800.txt |  28 ++
 drivers/clk/Kconfig                               |   8 +
 drivers/clk/Makefile                              |   1 +
 drivers/clk/clk-88pm800.c                         | 345 ++++++++++++++++++++++
 drivers/mfd/88pm800.c                             |  25 ++
 include/linux/mfd/88pm80x.h                       |  12 +
 6 files changed, 419 insertions(+)
 create mode 100644 drivers/clk/clk-88pm800.c

-- 
1.9.1

[PATCH 1/4] mfd: 88pm800: Update the header file with 32K clk related macros

[Vaibhav Hiremath]

Update header file with required macros for 32KHz buffered clock
output of 88PM800 family of device.
These macros will be used in clk provider driver.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 include/linux/mfd/88pm80x.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
index 05d9bad..680e4eb 100644
--- a/include/linux/mfd/88pm80x.h
+++ b/include/linux/mfd/88pm80x.h
@@ -91,6 +91,7 @@ enum {
 /* Referance and low power registers */
 #define PM800_LOW_POWER1		(0x20)
 #define PM800_LOW_POWER2		(0x21)
+#define PM800_LOW_POWER2_XO_LJ_EN	BIT(5)
 
 #define PM800_LOW_POWER_CONFIG3		(0x22)
 #define PM800_LDOBK_FREEZE		BIT(7)
@@ -138,6 +139,13 @@ enum {
 #define PM800_ALARM			BIT(5)
 #define PM800_RTC1_USE_XO		BIT(7)
 
+#define PM800_32K_OUTX_SEL_MASK		(0x3)
+/* 32KHz clk output sel mode */
+#define PM800_32K_OUTX_SEL_ZERO		(0x0)
+#define PM800_32K_OUTX_SEL_INT_32KHZ	(0x1)
+#define PM800_32K_OUTX_SEL_XO_32KHZ	(0x2)
+#define PM800_32K_OUTX_SEL_HIZ		(0x3)
+
 /* Regulator Control Registers: BUCK1,BUCK5,LDO1 have DVC */
 
 /* buck registers */
@@ -208,6 +216,10 @@ enum {
 #define PM800_PMOD_MEAS1		0x52
 #define PM800_PMOD_MEAS2		0x53
 
+/* Oscillator control */
+#define PM800_OSC_CNTRL1		(0x50)
+#define PM800_OSC_CNTRL1_OSC_FREERUN_EN	BIT(1)
+
 #define PM800_GPADC0_MEAS1		0x54
 #define PM800_GPADC0_MEAS2		0x55
 #define PM800_GPADC1_MEAS1		0x56
-- 
1.9.1

[PATCH 2/4] mfd: devicetree: bindings: Add clock subdevice node information

[Vaibhav Hiremath]

This patch updates the binding documentation for optional
clocks node and related information for buffered 32KHz clock.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 Documentation/devicetree/bindings/mfd/88pm800.txt | 28 +++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/88pm800.txt b/Documentation/devicetree/bindings/mfd/88pm800.txt
index dec842f..60cab78 100644
--- a/Documentation/devicetree/bindings/mfd/88pm800.txt
+++ b/Documentation/devicetree/bindings/mfd/88pm800.txt
@@ -9,6 +9,27 @@ Required parent device properties:
 - #interrupt-cells 	: should be 1.
 			  The cell is the 88pm80x local IRQ number
 
+Optional nodes:
+- clocks: 88pm800 family of devices provide multiple buffered 32.768
+  KHz outputs, so to register these as clocks with common clock framework
+  instantiate a sub-node named "clocks". It uses the common clock binding
+  documented in :
+  [Documentation/devicetree/bindings/clock/clock-bindings.txt]
+
+  - #clock-cells: should be 1.
+
+  - The following is the list of clocks generated by the controller. Each clock
+    is assigned an identifier and client nodes use this identifier to specify
+    the clock which they consume.
+    Clock		ID           	Devices
+    ----------------------------------------------------------
+    pm800_clk32k_1	0		88PM800 and 88PM860
+    pm800_clk32k_2	1		88PM800 and 88PM860
+    pm800_clk32k_3	2		88PM800
+
+  - compatible: Should be : "marvell,88pm800-clk"
+
+
 88pm80x family of devices consists of varied group of sub-devices:
 
 Device		 	Supply Names	 Description
@@ -16,6 +37,7 @@ Device		 	Supply Names	 Description
 88pm80x-onkey		:		: On key
 88pm80x-rtc		:		: RTC
 88pm80x-regulator	:		: Regulators
+88pm80x-clk		:		: 32KHz Clk provider
 
 Example:
 
@@ -27,6 +49,12 @@ Example:
 		interrupt-controller;
 		#interrupt-cells = <1>;
 
+		pm800clk: clocks {
+			compatible = "marvell,88pm800-clk";
+			#clock-cells = <1>;
+			clock-output-names = "xx", "yy", "zz";
+		};
+
 		regulators {
 			compatible = "marvell,88pm80x-regulator";
 
-- 
1.9.1

[PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Vaibhav Hiremath]

88PM800 family of devices supports buffered 32KHz clock output,
for example,

88PM800: CLK32k_1, CLK32k_2 and CLK32k_3
88PM860: CLK32K_1 and CLK32K_2

This patch adds new clk provider driver to support enable/disable
of the 32KHz clock output from 88PM800 family of devices.

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/clk/Kconfig       |   8 ++
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-88pm800.c | 345 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 354 insertions(+)
 create mode 100644 drivers/clk/clk-88pm800.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 42f7120..c34c14d 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -167,6 +167,14 @@ config COMMON_CLK_CDCE706
 	---help---
 	  This driver supports TI CDCE706 programmable 3-PLL clock synthesizer.
 
+config COMMON_CLK_88PM800
+	tristate "Clock driver for 88PM800 MFD"
+	depends on MFD_88PM800
+	---help---
+	  This driver supports 88PM800 & 88PM860 crystal oscillator
+	  clock. These multi-function devices have two (88PM860) or three
+	  (88PM800) fixed-rate output, clocked at 32KHz each.
+
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
 source "drivers/clk/qcom/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index c4cf075..5248cd3 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -80,3 +80,4 @@ obj-$(CONFIG_X86)			+= x86/
 obj-$(CONFIG_ARCH_ZX)			+= zte/
 obj-$(CONFIG_ARCH_ZYNQ)			+= zynq/
 obj-$(CONFIG_H8300)		+= h8300/
+obj-$(CONFIG_COMMON_CLK_88PM800)	+= clk-88pm800.o
diff --git a/drivers/clk/clk-88pm800.c b/drivers/clk/clk-88pm800.c
new file mode 100644
index 0000000..cf1c162
--- /dev/null
+++ b/drivers/clk/clk-88pm800.c
@@ -0,0 +1,345 @@
+/*
+ * clk-88pm800.c - Clock driver for 88PM800 family of devices
+ *
+ * This driver is created based on clk-s2mps11.c
+ *
+ * Copyright (C) 2015 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/clkdev.h>
+#include <linux/regmap.h>
+#include <linux/clk-provider.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/88pm80x.h>
+
+/* Number of clocks output from 88pm800 family of device */
+enum {
+	PM800_CLK32K_1 = 0,
+	PM800_CLK32K_2,
+	PM800_CLK32K_3,
+	PM800_CLKS_NUM,
+};
+
+struct pm800_clk {
+	struct pm80x_chip *chip;
+	struct device_node *clk_np;
+	struct clk_hw hw;
+	struct clk *clk;
+	struct clk_lookup *lookup;
+	u32 mask;
+	u32 shift;
+	unsigned int reg;
+};
+
+static struct pm800_clk *to_pm800_clk(struct clk_hw *hw)
+{
+	return container_of(hw, struct pm800_clk, hw);
+}
+
+static int pm800_clk_prepare(struct clk_hw *hw)
+{
+	struct pm800_clk *pm800 = to_pm800_clk(hw);
+	int ret;
+
+	/* We always want to use XO clock */
+	ret = regmap_update_bits(pm800->chip->regmap,
+				 pm800->reg,
+				 pm800->mask,
+				 PM800_32K_OUTX_SEL_XO_32KHZ << pm800->shift);
+
+	return ret;
+}
+
+static void pm800_clk_unprepare(struct clk_hw *hw)
+{
+	struct pm800_clk *pm800 = to_pm800_clk(hw);
+	int ret;
+
+	ret = regmap_update_bits(pm800->chip->regmap, pm800->reg,
+				pm800->mask, ~pm800->mask);
+}
+
+static int pm800_clk_is_prepared(struct clk_hw *hw)
+{
+	int ret;
+	u32 val;
+	struct pm800_clk *pm800 = to_pm800_clk(hw);
+
+	ret = regmap_read(pm800->chip->regmap,
+				pm800->reg, &val);
+	if (ret < 0)
+		return -EINVAL;
+
+	return ((val & pm800->mask) >> pm800->shift);
+}
+
+static unsigned long pm800_clk_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	return 32768;
+}
+
+static struct clk_ops pm800_clk_ops = {
+	.prepare	= pm800_clk_prepare,
+	.unprepare	= pm800_clk_unprepare,
+	.is_prepared	= pm800_clk_is_prepared,
+	.recalc_rate	= pm800_clk_recalc_rate,
+};
+
+static struct clk_init_data pm800_clks_init[PM800_CLKS_NUM] = {
+	[PM800_CLK32K_1] = {
+		.name	= "pm800_clk32k_1",
+		.ops	= &pm800_clk_ops,
+		.flags	= CLK_IS_ROOT,
+	},
+	[PM800_CLK32K_2] = {
+		.name	= "pm800_clk32k_2",
+		.ops	= &pm800_clk_ops,
+		.flags	= CLK_IS_ROOT,
+	},
+	[PM800_CLK32K_3] = {
+		.name	= "pm800_clk32k_3",
+		.ops	= &pm800_clk_ops,
+		.flags	= CLK_IS_ROOT,
+	},
+};
+
+static struct clk_init_data pm860_clks_init[PM800_CLKS_NUM] = {
+	[PM800_CLK32K_1] = {
+		.name	= "pm800_clk32k_1",
+		.ops	= &pm800_clk_ops,
+		.flags	= CLK_IS_ROOT,
+	},
+	[PM800_CLK32K_2] = {
+		.name	= "pm800_clk32k_2",
+		.ops	= &pm800_clk_ops,
+		.flags	= CLK_IS_ROOT,
+	},
+};
+
+static int pm800_init_clk(struct pm800_clk *pm800_clks)
+{
+
+	int ret;
+
+	/* Enable XO_LJ : Low jitter clock of 32KHz from XO */
+	ret = regmap_update_bits(pm800_clks->chip->regmap, PM800_LOW_POWER2,
+			PM800_LOW_POWER2_XO_LJ_EN, PM800_LOW_POWER2_XO_LJ_EN);
+	if (ret)
+		return ret;
+
+	/* Enable USE_XO : Use XO clock for all internal timing reference */
+	ret = regmap_update_bits(pm800_clks->chip->regmap, PM800_RTC_CONTROL,
+			PM800_RTC1_USE_XO, PM800_RTC1_USE_XO);
+	if (ret)
+		return ret;
+
+	/* OSC_FREERUN: Enable Osc free running mode by clearing the bit */
+	ret = regmap_update_bits(pm800_clks->chip->regmap, PM800_OSC_CNTRL1,
+			PM800_OSC_CNTRL1_OSC_FREERUN_EN, 0);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct device_node *pm800_clk_parse_dt(struct platform_device *pdev,
+		struct clk_init_data *clks_init)
+{
+	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+	struct device_node *clk_np;
+	int i;
+
+	if (!chip->dev->of_node)
+		return ERR_PTR(-EINVAL);
+
+	clk_np = of_get_child_by_name(chip->dev->of_node, "clocks");
+	if (!clk_np) {
+		dev_err(&pdev->dev, "could not find clock sub-node\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	for (i = 0; i < PM800_CLKS_NUM; i++) {
+		if (!clks_init[i].name)
+			continue; /* Skip clocks not present in some devices */
+
+		of_property_read_string_index(clk_np, "clock-output-names", i,
+				&clks_init[i].name);
+	}
+
+	return clk_np;
+}
+
+static int pm800_clk_probe(struct platform_device *pdev)
+{
+	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
+	struct pm800_clk *pm800_clks;
+	struct clk_init_data *clks_init;
+	static struct clk **clk_table;
+	static struct clk_onecell_data *of_clk_data;
+	int i, ret = 0;
+
+	pm800_clks = devm_kzalloc(&pdev->dev,
+				sizeof(*pm800_clks) * PM800_CLKS_NUM,
+				GFP_KERNEL);
+	if (!pm800_clks)
+		return -ENOMEM;
+
+	clk_table = devm_kzalloc(&pdev->dev,
+				sizeof(struct clk *) * PM800_CLKS_NUM,
+				GFP_KERNEL);
+	if (!clk_table)
+		return -ENOMEM;
+
+	switch (platform_get_device_id(pdev)->driver_data) {
+	case CHIP_PM800:
+		clks_init = pm800_clks_init;
+		break;
+	case CHIP_PM860:
+		clks_init = pm860_clks_init;
+		break;
+	default:
+		dev_err(&pdev->dev, "Invalid device type\n");
+		return -EINVAL;
+	}
+
+	/* Store clocks of_node in first element of pm800_clks array */
+	pm800_clks->clk_np = pm800_clk_parse_dt(pdev, clks_init);
+	if (IS_ERR(pm800_clks->clk_np))
+		return PTR_ERR(pm800_clks->clk_np);
+
+	of_clk_data = devm_kzalloc(&pdev->dev,
+				sizeof(*of_clk_data),
+				GFP_KERNEL);
+	if (!of_clk_data)
+		return -ENOMEM;
+
+	for (i = 0; i < PM800_CLKS_NUM; i++) {
+		if (!clks_init[i].name)
+			continue; /* Skip clocks not present in some devices */
+
+		pm800_clks[i].chip = chip;
+		pm800_clks[i].hw.init = &clks_init[i];
+		/*
+		 * As of now, mask and shift formula below works for both
+		 * 88PM800 and it's family of devices,
+		 *
+		 *  PM800_RTC_MISC2:
+		 *      1:0	= CK_32K_OUT1_SEL
+		 *      3:2	= CK_32K_OUT2_SEL
+		 *      5:4	= CK_32K_OUT3_SEL
+		 */
+		pm800_clks[i].shift = (i * 2);
+		pm800_clks[i].mask = PM800_32K_OUTX_SEL_MASK << pm800_clks[i].shift;
+		pm800_clks[i].reg = PM800_RTC_MISC2;
+
+		pm800_clks[i].clk = devm_clk_register(&pdev->dev,
+							&pm800_clks[i].hw);
+		if (IS_ERR(pm800_clks[i].clk)) {
+			dev_err(&pdev->dev, "Fail to register : %s\n",
+						clks_init[i].name);
+			ret = PTR_ERR(pm800_clks[i].clk);
+			goto err;
+		}
+
+		pm800_clks[i].lookup = clkdev_create(pm800_clks[i].clk,
+					clks_init[i].name, NULL);
+		if (!pm800_clks[i].lookup) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		clk_table[i] = pm800_clks[i].clk;
+	}
+
+	of_clk_data->clks = clk_table;
+	of_clk_data->clk_num = PM800_CLKS_NUM;
+	ret = of_clk_add_provider(pm800_clks->clk_np, of_clk_src_onecell_get,
+			of_clk_data);
+	if (ret) {
+		dev_err(&pdev->dev, "Fail to add OF clk provider : %d\n", ret);
+		goto err;
+	}
+
+	/* Common for all 32KHz clock output */
+	ret = pm800_init_clk(&pm800_clks[0]);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialize clk : %d\n", ret);
+		goto err;
+	}
+
+	platform_set_drvdata(pdev, pm800_clks);
+
+	return 0;
+
+err:
+	for (i = 0; i < PM800_CLKS_NUM; i++) {
+		if (pm800_clks[i].lookup)
+			clkdev_drop(pm800_clks[i].lookup);
+	}
+
+	return ret;
+}
+
+static int pm800_clk_remove(struct platform_device *pdev)
+{
+	struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
+	int i;
+
+	of_clk_del_provider(pm800_clks[0].clk_np);
+	/* Drop the reference obtained in pm800_clk_parse_dt */
+	of_node_put(pm800_clks[0].clk_np);
+
+	for (i = 0; i < PM800_CLKS_NUM; i++) {
+		if (pm800_clks[i].lookup)
+			clkdev_drop(pm800_clks[i].lookup);
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id pm800_clk_id[] = {
+	{ "88pm800-clk", CHIP_PM800},
+	{ "88pm860-clk", CHIP_PM860},
+	{ },
+};
+MODULE_DEVICE_TABLE(platform, pm800_clk_id);
+
+static struct platform_driver pm800_clk_driver = {
+	.driver = {
+		.name  = "88pm80x-clk",
+	},
+	.probe = pm800_clk_probe,
+	.remove = pm800_clk_remove,
+	.id_table = pm800_clk_id,
+};
+
+static int __init pm800_clk_init(void)
+{
+	return platform_driver_register(&pm800_clk_driver);
+}
+subsys_initcall(pm800_clk_init);
+
+static void __init pm800_clk_cleanup(void)
+{
+	platform_driver_unregister(&pm800_clk_driver);
+}
+module_exit(pm800_clk_cleanup);
+
+MODULE_DESCRIPTION("88PM800 Clock Driver");
+MODULE_AUTHOR("Vaibhav Hiremath <vaibhav.hiremath@linaro.org>");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH 4/4] mfd: 88pm800: Add support for clk subdevice

[Vaibhav Hiremath]

This patch adds mfd_cell/clk-subdevice for 88PM800 MFD
(and family of devices).

Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
---
 drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
index f104a32..9723eac 100644
--- a/drivers/mfd/88pm800.c
+++ b/drivers/mfd/88pm800.c
@@ -173,6 +173,14 @@ static const struct mfd_cell regulator_devs[] = {
 	},
 };
 
+static struct mfd_cell clk_devs[] = {
+	{
+	 .name = "88pm80x-clk",
+	 .of_compatible = "marvell,88pm800-clk",
+	 .id = -1,
+	},
+};
+
 static const struct regmap_irq pm800_irqs[] = {
 	/* INT0 */
 	[PM800_IRQ_ONKEY] = {
@@ -344,6 +352,17 @@ static int device_regulator_init(struct pm80x_chip *chip)
 			      ARRAY_SIZE(regulator_devs), NULL, 0, NULL);
 }
 
+static int device_clk_init(struct pm80x_chip *chip)
+{
+	if (chip->type == CHIP_PM800)
+		clk_devs[0].name = "88pm800-clk";
+	else if (chip->type == CHIP_PM860)
+		clk_devs[0].name = "88pm860-clk";
+
+	return mfd_add_devices(chip->dev, 0, &clk_devs[0],
+			      ARRAY_SIZE(clk_devs), NULL, 0, NULL);
+}
+
 static int device_irq_init_800(struct pm80x_chip *chip)
 {
 	struct regmap *map = chip->regmap;
@@ -513,6 +532,12 @@ static int device_800_init(struct pm80x_chip *chip)
 		goto out;
 	}
 
+	ret = device_clk_init(chip);
+	if (ret) {
+		dev_err(chip->dev, "Failed to add clk subdev\n");
+		goto out;
+	}
+
 	return 0;
 out_dev:
 	mfd_remove_devices(chip->dev);
-- 
1.9.1

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Stephen Boyd]

On 07/21/2015 04:07 AM, Vaibhav Hiremath wrote:
> diff --git a/drivers/clk/clk-88pm800.c b/drivers/clk/clk-88pm800.c
> new file mode 100644
> index 0000000..cf1c162
> --- /dev/null
> +++ b/drivers/clk/clk-88pm800.c
> @@ -0,0 +1,345 @@
> +/*
> + * clk-88pm800.c - Clock driver for 88PM800 family of devices
> + *
> + * This driver is created based on clk-s2mps11.c
> + *
> + * Copyright (C) 2015 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/clkdev.h>
> +#include <linux/regmap.h>
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/88pm80x.h>
> +
> +/* Number of clocks output from 88pm800 family of device */
> +enum {
> +	PM800_CLK32K_1 = 0,
> +	PM800_CLK32K_2,
> +	PM800_CLK32K_3,
> +	PM800_CLKS_NUM,
> +};
> +
> +struct pm800_clk {
> +	struct pm80x_chip *chip;
> +	struct device_node *clk_np;
> +	struct clk_hw hw;
> +	struct clk *clk;
> +	struct clk_lookup *lookup;
> +	u32 mask;
> +	u32 shift;
> +	unsigned int reg;
> +};
> +
> +static struct pm800_clk *to_pm800_clk(struct clk_hw *hw)
> +{
> +	return container_of(hw, struct pm800_clk, hw);
> +}
> +
> +static int pm800_clk_prepare(struct clk_hw *hw)
> +{
> +	struct pm800_clk *pm800 = to_pm800_clk(hw);
> +	int ret;
> +
> +	/* We always want to use XO clock */
> +	ret = regmap_update_bits(pm800->chip->regmap,
> +				 pm800->reg,
> +				 pm800->mask,
> +				 PM800_32K_OUTX_SEL_XO_32KHZ << pm800->shift);
> +
> +	return ret;

Can be simplified to:

     return regmap_update_bits()

> +}
> +
> +static void pm800_clk_unprepare(struct clk_hw *hw)
> +{
> +	struct pm800_clk *pm800 = to_pm800_clk(hw);
> +	int ret;
> +
> +	ret = regmap_update_bits(pm800->chip->regmap, pm800->reg,
> +				pm800->mask, ~pm800->mask);

Why have ret at all in a void function?

> +}
> +
> +static int pm800_clk_is_prepared(struct clk_hw *hw)
> +{
> +	int ret;
> +	u32 val;
> +	struct pm800_clk *pm800 = to_pm800_clk(hw);
> +
> +	ret = regmap_read(pm800->chip->regmap,
> +				pm800->reg, &val);
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	return ((val & pm800->mask) >> pm800->shift);

One too many parentheses here.

> +}
> +
> +static unsigned long pm800_clk_recalc_rate(struct clk_hw *hw,
> +					     unsigned long parent_rate)
> +{
> +	return 32768;
> +}
> +
> +static struct clk_ops pm800_clk_ops = {

const?

> +	.prepare	= pm800_clk_prepare,
> +	.unprepare	= pm800_clk_unprepare,
> +	.is_prepared	= pm800_clk_is_prepared,
> +	.recalc_rate	= pm800_clk_recalc_rate,
> +};
> +
> +static struct clk_init_data pm800_clks_init[PM800_CLKS_NUM] = {
> +	[PM800_CLK32K_1] = {
> +		.name	= "pm800_clk32k_1",
> +		.ops	= &pm800_clk_ops,
> +		.flags	= CLK_IS_ROOT,
> +	},
> +	[PM800_CLK32K_2] = {
> +		.name	= "pm800_clk32k_2",
> +		.ops	= &pm800_clk_ops,
> +		.flags	= CLK_IS_ROOT,
> +	},
> +	[PM800_CLK32K_3] = {
> +		.name	= "pm800_clk32k_3",
> +		.ops	= &pm800_clk_ops,
> +		.flags	= CLK_IS_ROOT,
> +	},
> +};
> +
> +static struct clk_init_data pm860_clks_init[PM800_CLKS_NUM] = {
> +	[PM800_CLK32K_1] = {
> +		.name	= "pm800_clk32k_1",
> +		.ops	= &pm800_clk_ops,
> +		.flags	= CLK_IS_ROOT,
> +	},
> +	[PM800_CLK32K_2] = {
> +		.name	= "pm800_clk32k_2",
> +		.ops	= &pm800_clk_ops,
> +		.flags	= CLK_IS_ROOT,
> +	},
> +};
> +
> +static int pm800_init_clk(struct pm800_clk *pm800_clks)
> +{
> +
> +	int ret;
> +
> +	/* Enable XO_LJ : Low jitter clock of 32KHz from XO */
> +	ret = regmap_update_bits(pm800_clks->chip->regmap, PM800_LOW_POWER2,
> +			PM800_LOW_POWER2_XO_LJ_EN, PM800_LOW_POWER2_XO_LJ_EN);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable USE_XO : Use XO clock for all internal timing reference */
> +	ret = regmap_update_bits(pm800_clks->chip->regmap, PM800_RTC_CONTROL,
> +			PM800_RTC1_USE_XO, PM800_RTC1_USE_XO);
> +	if (ret)
> +		return ret;
> +
> +	/* OSC_FREERUN: Enable Osc free running mode by clearing the bit */
> +	ret = regmap_update_bits(pm800_clks->chip->regmap, PM800_OSC_CNTRL1,
> +			PM800_OSC_CNTRL1_OSC_FREERUN_EN, 0);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static struct device_node *pm800_clk_parse_dt(struct platform_device *pdev,
> +		struct clk_init_data *clks_init)
> +{
> +	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> +	struct device_node *clk_np;
> +	int i;
> +
> +	if (!chip->dev->of_node)
> +		return ERR_PTR(-EINVAL);
> +
> +	clk_np = of_get_child_by_name(chip->dev->of_node, "clocks");
> +	if (!clk_np) {
> +		dev_err(&pdev->dev, "could not find clock sub-node\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	for (i = 0; i < PM800_CLKS_NUM; i++) {
> +		if (!clks_init[i].name)
> +			continue; /* Skip clocks not present in some devices */
> +
> +		of_property_read_string_index(clk_np, "clock-output-names", i,
> +				&clks_init[i].name);
> +	}
> +
> +	return clk_np;
> +}
> +
> +static int pm800_clk_probe(struct platform_device *pdev)
> +{
> +	struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
> +	struct pm800_clk *pm800_clks;
> +	struct clk_init_data *clks_init;
> +	static struct clk **clk_table;
> +	static struct clk_onecell_data *of_clk_data;
> +	int i, ret = 0;

Drop assignment to ret please.

> +
> +	pm800_clks = devm_kzalloc(&pdev->dev,
> +				sizeof(*pm800_clks) * PM800_CLKS_NUM,
> +				GFP_KERNEL);

devm_kcalloc()

> +	if (!pm800_clks)
> +		return -ENOMEM;
> +
> +	clk_table = devm_kzalloc(&pdev->dev,
> +				sizeof(struct clk *) * PM800_CLKS_NUM,
> +				GFP_KERNEL);

devm_kcalloc()

> +	if (!clk_table)
> +		return -ENOMEM;
> +
> +	switch (platform_get_device_id(pdev)->driver_data) {
> +	case CHIP_PM800:
> +		clks_init = pm800_clks_init;
> +		break;
> +	case CHIP_PM860:
> +		clks_init = pm860_clks_init;
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "Invalid device type\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Store clocks of_node in first element of pm800_clks array */
> +	pm800_clks->clk_np = pm800_clk_parse_dt(pdev, clks_init);
> +	if (IS_ERR(pm800_clks->clk_np))
> +		return PTR_ERR(pm800_clks->clk_np);
> +
> +	of_clk_data = devm_kzalloc(&pdev->dev,
> +				sizeof(*of_clk_data),
> +				GFP_KERNEL);
> +	if (!of_clk_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < PM800_CLKS_NUM; i++) {
> +		if (!clks_init[i].name)
> +			continue; /* Skip clocks not present in some devices */
> +
> +		pm800_clks[i].chip = chip;
> +		pm800_clks[i].hw.init = &clks_init[i];
> +		/*
> +		 * As of now, mask and shift formula below works for both
> +		 * 88PM800 and it's family of devices,
> +		 *
> +		 *  PM800_RTC_MISC2:
> +		 *      1:0	= CK_32K_OUT1_SEL
> +		 *      3:2	= CK_32K_OUT2_SEL
> +		 *      5:4	= CK_32K_OUT3_SEL
> +		 */
> +		pm800_clks[i].shift = (i * 2);

Drop parentheses please.

> +		pm800_clks[i].mask = PM800_32K_OUTX_SEL_MASK << pm800_clks[i].shift;
> +		pm800_clks[i].reg = PM800_RTC_MISC2;
> +
> +		pm800_clks[i].clk = devm_clk_register(&pdev->dev,
> +							&pm800_clks[i].hw);
> +		if (IS_ERR(pm800_clks[i].clk)) {
> +			dev_err(&pdev->dev, "Fail to register : %s\n",
> +						clks_init[i].name);
> +			ret = PTR_ERR(pm800_clks[i].clk);
> +			goto err;
> +		}
> +
> +		pm800_clks[i].lookup = clkdev_create(pm800_clks[i].clk,
> +					clks_init[i].name, NULL);
> +		if (!pm800_clks[i].lookup) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +
> +		clk_table[i] = pm800_clks[i].clk;
> +	}
> +
> +	of_clk_data->clks = clk_table;
> +	of_clk_data->clk_num = PM800_CLKS_NUM;
> +	ret = of_clk_add_provider(pm800_clks->clk_np, of_clk_src_onecell_get,
> +			of_clk_data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Fail to add OF clk provider : %d\n", ret);
> +		goto err;
> +	}
> +
> +	/* Common for all 32KHz clock output */
> +	ret = pm800_init_clk(&pm800_clks[0]);

Shouldn't we do this before registering the clocks with the framework?

> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to initialize clk : %d\n", ret);
> +		goto err;
> +	}
> +
> +	platform_set_drvdata(pdev, pm800_clks);
> +
> +	return 0;
> +
> +err:
> +	for (i = 0; i < PM800_CLKS_NUM; i++) {
> +		if (pm800_clks[i].lookup)
> +			clkdev_drop(pm800_clks[i].lookup);
> +	}
> +
> +	return ret;
> +}
> +
> +static int pm800_clk_remove(struct platform_device *pdev)
> +{
> +	struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
> +	int i;
> +
> +	of_clk_del_provider(pm800_clks[0].clk_np);
> +	/* Drop the reference obtained in pm800_clk_parse_dt */
> +	of_node_put(pm800_clks[0].clk_np);

This is odd. Why are we keeping the reference in the driver?

> +
> +	for (i = 0; i < PM800_CLKS_NUM; i++) {
> +		if (pm800_clks[i].lookup)
> +			clkdev_drop(pm800_clks[i].lookup);
> +	}
> +
> +	return 0;
> +}
> +
> [..]
> +
> +static int __init pm800_clk_init(void)
> +{
> +	return platform_driver_register(&pm800_clk_driver);
> +}
> +subsys_initcall(pm800_clk_init);
> +
> +static void __init pm800_clk_cleanup(void)

s/__init/__exit/

> +{
> +	platform_driver_unregister(&pm800_clk_driver);
> +}
> +module_exit(pm800_clk_cleanup);
> +
> +MODULE_DESCRIPTION("88PM800 Clock Driver");
> +MODULE_AUTHOR("Vaibhav Hiremath <vaibhav.hiremath@linaro.org>");
> +MODULE_LICENSE("GPL");


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Vaibhav Hiremath]

On Wednesday 22 July 2015 12:40 AM, Stephen Boyd wrote:
> On 07/21/2015 04:07 AM, Vaibhav Hiremath wrote:
>> diff --git a/drivers/clk/clk-88pm800.c b/drivers/clk/clk-88pm800.c
>> new file mode 100644
>> index 0000000..cf1c162
>> --- /dev/null
>> +++ b/drivers/clk/clk-88pm800.c
>> @@ -0,0 +1,345 @@
>> +/*
>> + * clk-88pm800.c - Clock driver for 88PM800 family of devices
>> + *
>> + * This driver is created based on clk-s2mps11.c
>> + *
>> + * Copyright (C) 2015 Linaro Ltd.
>> + *
>> + * This program is free software; you can redistribute  it and/or
>> modify it
>> + * under  the terms of  the GNU General  Public License as published
>> by the
>> + * Free Software Foundation;  either version 2 of the  License, or
>> (at your
>> + * option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +

<snip>

>> +    struct pm80x_chip *chip = dev_get_drvdata(pdev->dev.parent);
>> +    struct pm800_clk *pm800_clks;
>> +    struct clk_init_data *clks_init;
>> +    static struct clk **clk_table;
>> +    static struct clk_onecell_data *of_clk_data;
>> +    int i, ret = 0;
>
> Drop assignment to ret please.
>

Agreed to this and all above comments.

>> +
>> +    pm800_clks = devm_kzalloc(&pdev->dev,
>> +                sizeof(*pm800_clks) * PM800_CLKS_NUM,
>> +                GFP_KERNEL);
>
> devm_kcalloc()
>
>> +    if (!pm800_clks)
>> +        return -ENOMEM;
>> +
>> +    clk_table = devm_kzalloc(&pdev->dev,
>> +                sizeof(struct clk *) * PM800_CLKS_NUM,
>> +                GFP_KERNEL);
>
> devm_kcalloc()
>

why kcalloc?
Is it because it take another argument for to allocate array?


>> +    if (!clk_table)
>> +        return -ENOMEM;
>> +


< snip >

>> +            of_clk_data);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Fail to add OF clk provider : %d\n", ret);
>> +        goto err;
>> +    }
>> +
>> +    /* Common for all 32KHz clock output */
>> +    ret = pm800_init_clk(&pm800_clks[0]);
>
> Shouldn't we do this before registering the clocks with the framework?
>

Actually I thought of this, but moved it at the end because, I feel
this init should happen only when we are sure that clock is ready for
consumption. This is HW initialization where we will be setting
FREERUNNIG mode (and similar stuffs), so thought it would be bad idea
to set it first and then on error (later in probe) clear it. Not sure
whether it has any impact on HW behaviour.
Also, specially internal reference counter is changing in the init.
Just tried to avoid back-n-forth set-n-clear of this.

>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Failed to initialize clk : %d\n", ret);
>> +        goto err;
>> +    }
>> +
>> +    platform_set_drvdata(pdev, pm800_clks);
>> +
>> +    return 0;
>> +
>> +err:
>> +    for (i = 0; i < PM800_CLKS_NUM; i++) {
>> +        if (pm800_clks[i].lookup)
>> +            clkdev_drop(pm800_clks[i].lookup);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>> +static int pm800_clk_remove(struct platform_device *pdev)
>> +{
>> +    struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
>> +    int i;
>> +
>> +    of_clk_del_provider(pm800_clks[0].clk_np);
>> +    /* Drop the reference obtained in pm800_clk_parse_dt */
>> +    of_node_put(pm800_clks[0].clk_np);
>
> This is odd. Why are we keeping the reference in the driver?
>

Honestly I do not have any good answer here. I have to admit that it is
getting carry forwarded from legacy driver.

>> +
>> +    for (i = 0; i < PM800_CLKS_NUM; i++) {
>> +        if (pm800_clks[i].lookup)
>> +            clkdev_drop(pm800_clks[i].lookup);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> [..]
>> +
>> +static int __init pm800_clk_init(void)
>> +{
>> +    return platform_driver_register(&pm800_clk_driver);
>> +}
>> +subsys_initcall(pm800_clk_init);
>> +
>> +static void __init pm800_clk_cleanup(void)
>
> s/__init/__exit/
>

I will all other comments.  Thanks for your review.

Thanks,
Vaibhav

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Stephen Boyd]

On 07/21/2015 12:36 PM, Vaibhav Hiremath wrote:
>
> On Wednesday 22 July 2015 12:40 AM, Stephen Boyd wrote:
>> On 07/21/2015 04:07 AM, Vaibhav Hiremath wrote:
>>> +
>>> +    pm800_clks = devm_kzalloc(&pdev->dev,
>>> +                sizeof(*pm800_clks) * PM800_CLKS_NUM,
>>> +                GFP_KERNEL);
>>
>> devm_kcalloc()
>>
>>> +    if (!pm800_clks)
>>> +        return -ENOMEM;
>>> +
>>> +    clk_table = devm_kzalloc(&pdev->dev,
>>> +                sizeof(struct clk *) * PM800_CLKS_NUM,
>>> +                GFP_KERNEL);
>>
>> devm_kcalloc()
>>
>
> why kcalloc?
> Is it because it take another argument for to allocate array?
>

Yes. See Chapter 14 of Documentation/CodingStyle for why devm_kcalloc() 
is preferred.

>
>>> +    if (!clk_table)
>>> +        return -ENOMEM;
>>> +
>
>
> < snip >
>
>>> +            of_clk_data);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Fail to add OF clk provider : %d\n", 
>>> ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    /* Common for all 32KHz clock output */
>>> +    ret = pm800_init_clk(&pm800_clks[0]);
>>
>> Shouldn't we do this before registering the clocks with the framework?
>>
>
> Actually I thought of this, but moved it at the end because, I feel
> this init should happen only when we are sure that clock is ready for
> consumption. This is HW initialization where we will be setting
> FREERUNNIG mode (and similar stuffs), so thought it would be bad idea
> to set it first and then on error (later in probe) clear it. Not sure
> whether it has any impact on HW behaviour.
> Also, specially internal reference counter is changing in the init.
> Just tried to avoid back-n-forth set-n-clear of this.

Ok.

>
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Failed to initialize clk : %d\n", ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    platform_set_drvdata(pdev, pm800_clks);
>>> +
>>> +    return 0;
>>> +
>>> +err:
>>> +    for (i = 0; i < PM800_CLKS_NUM; i++) {
>>> +        if (pm800_clks[i].lookup)
>>> +            clkdev_drop(pm800_clks[i].lookup);
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int pm800_clk_remove(struct platform_device *pdev)
>>> +{
>>> +    struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
>>> +    int i;
>>> +
>>> +    of_clk_del_provider(pm800_clks[0].clk_np);
>>> +    /* Drop the reference obtained in pm800_clk_parse_dt */
>>> +    of_node_put(pm800_clks[0].clk_np);
>>
>> This is odd. Why are we keeping the reference in the driver?
>>
>
> Honestly I do not have any good answer here. I have to admit that it is
> getting carry forwarded from legacy driver.
>

Well we shouldn't do things if we don't know why we're doing them. 
Krzysztof?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Vaibhav Hiremath]

On Wednesday 22 July 2015 02:22 AM, Stephen Boyd wrote:
> On 07/21/2015 12:36 PM, Vaibhav Hiremath wrote:
>>
>> On Wednesday 22 July 2015 12:40 AM, Stephen Boyd wrote:
>>> On 07/21/2015 04:07 AM, Vaibhav Hiremath wrote:
>>>> +

<snip>

>>>> +static int pm800_clk_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
>>>> +    int i;
>>>> +
>>>> +    of_clk_del_provider(pm800_clks[0].clk_np);
>>>> +    /* Drop the reference obtained in pm800_clk_parse_dt */
>>>> +    of_node_put(pm800_clks[0].clk_np);
>>>
>>> This is odd. Why are we keeping the reference in the driver?
>>>
>>
>> Honestly I do not have any good answer here. I have to admit that it is
>> getting carry forwarded from legacy driver.
>>
>
> Well we shouldn't do things if we don't know why we're doing them.
> Krzysztof?
>

Hold on,
After looking more in to this, it seems we really do not need it.
It is already taken care by

of_clk_add_provider() and
of_clk_del_provider()

Sorry for not investigating this before. Just left out from my eyes
somehow.


Actually I can cleanup clk-s2mps11.c driver as well, but only thing is
I can validate it, as I do not have platform to test it.
It should be trivial changes.

If somebody can help me out in validation I can submit the patch for
clk-s2mps11.c driver as well.

Thanks,
Vaibhav

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Krzysztof Kozlowski]

On 22.07.2015 15:27, Vaibhav Hiremath wrote:
> 
> 
> On Wednesday 22 July 2015 02:22 AM, Stephen Boyd wrote:
>> On 07/21/2015 12:36 PM, Vaibhav Hiremath wrote:
>>>
>>> On Wednesday 22 July 2015 12:40 AM, Stephen Boyd wrote:
>>>> On 07/21/2015 04:07 AM, Vaibhav Hiremath wrote:
>>>>> +
> 
> <snip>
> 
>>>>> +static int pm800_clk_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
>>>>> +    int i;
>>>>> +
>>>>> +    of_clk_del_provider(pm800_clks[0].clk_np);
>>>>> +    /* Drop the reference obtained in pm800_clk_parse_dt */
>>>>> +    of_node_put(pm800_clks[0].clk_np);
>>>>
>>>> This is odd. Why are we keeping the reference in the driver?
>>>>
>>>
>>> Honestly I do not have any good answer here. I have to admit that it is
>>> getting carry forwarded from legacy driver.
>>>
>>
>> Well we shouldn't do things if we don't know why we're doing them.
>> Krzysztof?

I am really busy now so I am not following closely other discussions. I
assume you are referring to clk-s2mps11.c. The of_node_put() matches
of_get_child_by_name() when parsing DT.

So why not of_node_put() just after parsing DT? Well, the result of
of_get_child_by_name() is stored in state container for entire device
life-cycle so we can use it in of_clk_del_provider().

That was the idea behind it. If it looks incorrect I would be happy to
see a patch :) .

>>
> 
> Hold on,
> After looking more in to this, it seems we really do not need it.
> It is already taken care by
> 
> of_clk_add_provider() and
> of_clk_del_provider()
> 
> Sorry for not investigating this before. Just left out from my eyes
> somehow.
> 
> 
> Actually I can cleanup clk-s2mps11.c driver as well, but only thing is
> I can validate it, as I do not have platform to test it.
> It should be trivial changes.
> 
> If somebody can help me out in validation I can submit the patch for
> clk-s2mps11.c driver as well.

Sure, I can do this. The clock is present on few devices I can test.
Depending on the current workqueue it may take few days. Just please
mark the patch RFT so it won't be applied before receiving
reviewed/tested tags.

Best regards,
Krzysztof

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Vaibhav Hiremath]

On Wednesday 22 July 2015 12:16 PM, Krzysztof Kozlowski wrote:
> On 22.07.2015 15:27, Vaibhav Hiremath wrote:
>>
>>
>> On Wednesday 22 July 2015 02:22 AM, Stephen Boyd wrote:
>>> On 07/21/2015 12:36 PM, Vaibhav Hiremath wrote:
>>>>
>>>> On Wednesday 22 July 2015 12:40 AM, Stephen Boyd wrote:
>>>>> On 07/21/2015 04:07 AM, Vaibhav Hiremath wrote:
>>>>>> +
>>
>> <snip>
>>
>>>>>> +static int pm800_clk_remove(struct platform_device *pdev)
>>>>>> +{
>>>>>> +    struct pm800_clk *pm800_clks = platform_get_drvdata(pdev);
>>>>>> +    int i;
>>>>>> +
>>>>>> +    of_clk_del_provider(pm800_clks[0].clk_np);
>>>>>> +    /* Drop the reference obtained in pm800_clk_parse_dt */
>>>>>> +    of_node_put(pm800_clks[0].clk_np);
>>>>>
>>>>> This is odd. Why are we keeping the reference in the driver?
>>>>>
>>>>
>>>> Honestly I do not have any good answer here. I have to admit that it is
>>>> getting carry forwarded from legacy driver.
>>>>
>>>
>>> Well we shouldn't do things if we don't know why we're doing them.
>>> Krzysztof?
>
> I am really busy now so I am not following closely other discussions. I
> assume you are referring to clk-s2mps11.c. The of_node_put() matches
> of_get_child_by_name() when parsing DT.
>
> So why not of_node_put() just after parsing DT? Well, the result of
> of_get_child_by_name() is stored in state container for entire device
> life-cycle so we can use it in of_clk_del_provider().
>
> That was the idea behind it. If it looks incorrect I would be happy to
> see a patch :) .
>

About to respond, I digged more on kobject stuff and sequence in
of/dynamic.c and

I think you are right, we need of_node_put, as a result of
of_get_child_by_name().

Stephen,
Please let me know if you think otherwise.

Thanks,
Vaibhav

Re: [PATCH 3/4] clk: 88pm800: Add clk provider driver for 88pm800 family of devices

[Stephen Boyd]

On 07/22/2015 01:16 AM, Vaibhav Hiremath wrote:
>
>
> On Wednesday 22 July 2015 12:16 PM, Krzysztof Kozlowski wrote:
>>
>> I am really busy now so I am not following closely other discussions. I
>> assume you are referring to clk-s2mps11.c. The of_node_put() matches
>> of_get_child_by_name() when parsing DT.
>>
>> So why not of_node_put() just after parsing DT? Well, the result of
>> of_get_child_by_name() is stored in state container for entire device
>> life-cycle so we can use it in of_clk_del_provider().
>>
>> That was the idea behind it. If it looks incorrect I would be happy to
>> see a patch :) .
>>
>
> About to respond, I digged more on kobject stuff and sequence in
> of/dynamic.c and
>
> I think you are right, we need of_node_put, as a result of
> of_get_child_by_name().
>
> Stephen,
> Please let me know if you think otherwise.
>

Yes, sounds fine. I was thinking that we grab the reference to the node 
in of_clk_add_provider() so dropping it here was to undo that, but that 
isn't true. It probably can be dropped after we register the provider 
because adding the provider will keep it pinned, but this way is more 
symmetric so it's fine.

Either way, the error path on probe doesn't call of_node_put(), so 
that's a leak.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 4/4] mfd: 88pm800: Add support for clk subdevice

[Krzysztof Kozlowski]

2015-07-21 20:07 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
> This patch adds mfd_cell/clk-subdevice for 88PM800 MFD
> (and family of devices).
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>

Looks OK:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH 2/4] mfd: devicetree: bindings: Add clock subdevice node information

[Krzysztof Kozlowski]

2015-07-21 20:07 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
> This patch updates the binding documentation for optional
> clocks node and related information for buffered 32KHz clock.
>
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  Documentation/devicetree/bindings/mfd/88pm800.txt | 28 +++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

+Cc Stephen Boyd (clocks)

We had a discussion whether clock bindings should be put in MFD
bindings documentation or into separate file in bindings/clock but
either way is fine for me:

Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH 4/4] mfd: 88pm800: Add support for clk subdevice

[Lee Jones]

On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:

> This patch adds mfd_cell/clk-subdevice for 88PM800 MFD
> (and family of devices).
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
> index f104a32..9723eac 100644
> --- a/drivers/mfd/88pm800.c
> +++ b/drivers/mfd/88pm800.c
> @@ -173,6 +173,14 @@ static const struct mfd_cell regulator_devs[] = {
>  	},
>  };
>  
> +static struct mfd_cell clk_devs[] = {
> +	{
> +	 .name = "88pm80x-clk",
> +	 .of_compatible = "marvell,88pm800-clk",
> +	 .id = -1,
> +	},
> +};

Why does each device in 88pm800.c have it's own mfd_cell?

Take the opportunity to correct the tabbing in the remainder of the
file.  Make that fix-up patch 1 of this set.  Then fixup this patch.

>  static const struct regmap_irq pm800_irqs[] = {
>  	/* INT0 */
>  	[PM800_IRQ_ONKEY] = {
> @@ -344,6 +352,17 @@ static int device_regulator_init(struct pm80x_chip *chip)
>  			      ARRAY_SIZE(regulator_devs), NULL, 0, NULL);
>  }
>  
> +static int device_clk_init(struct pm80x_chip *chip)
> +{
> +	if (chip->type == CHIP_PM800)
> +		clk_devs[0].name = "88pm800-clk";
> +	else if (chip->type == CHIP_PM860)
> +		clk_devs[0].name = "88pm860-clk";
> +
> +	return mfd_add_devices(chip->dev, 0, &clk_devs[0],
> +			      ARRAY_SIZE(clk_devs), NULL, 0, NULL);
> +}
> +
>  static int device_irq_init_800(struct pm80x_chip *chip)
>  {
>  	struct regmap *map = chip->regmap;
> @@ -513,6 +532,12 @@ static int device_800_init(struct pm80x_chip *chip)
>  		goto out;
>  	}
>  
> +	ret = device_clk_init(chip);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to add clk subdev\n");
> +		goto out;
> +	}

Why do these have to be seperate?

>  	return 0;
>  out_dev:
>  	mfd_remove_devices(chip->dev);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] mfd: 88pm800: Update the header file with 32K clk related macros

[Lee Jones]

On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:

> Update header file with required macros for 32KHz buffered clock
> output of 88PM800 family of device.
> These macros will be used in clk provider driver.
> 
> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
> ---
>  include/linux/mfd/88pm80x.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
> index 05d9bad..680e4eb 100644
> --- a/include/linux/mfd/88pm80x.h
> +++ b/include/linux/mfd/88pm80x.h
> @@ -91,6 +91,7 @@ enum {
>  /* Referance and low power registers */
>  #define PM800_LOW_POWER1		(0x20)
>  #define PM800_LOW_POWER2		(0x21)
> +#define PM800_LOW_POWER2_XO_LJ_EN	BIT(5)
>  
>  #define PM800_LOW_POWER_CONFIG3		(0x22)
>  #define PM800_LDOBK_FREEZE		BIT(7)
> @@ -138,6 +139,13 @@ enum {
>  #define PM800_ALARM			BIT(5)
>  #define PM800_RTC1_USE_XO		BIT(7)
>  
> +#define PM800_32K_OUTX_SEL_MASK		(0x3)
> +/* 32KHz clk output sel mode */
> +#define PM800_32K_OUTX_SEL_ZERO		(0x0)
> +#define PM800_32K_OUTX_SEL_INT_32KHZ	(0x1)
> +#define PM800_32K_OUTX_SEL_XO_32KHZ	(0x2)
> +#define PM800_32K_OUTX_SEL_HIZ		(0x3)

Why do these need to be in brackets?

>  /* Regulator Control Registers: BUCK1,BUCK5,LDO1 have DVC */
>  
>  /* buck registers */
> @@ -208,6 +216,10 @@ enum {
>  #define PM800_PMOD_MEAS1		0x52
>  #define PM800_PMOD_MEAS2		0x53
>  
> +/* Oscillator control */
> +#define PM800_OSC_CNTRL1		(0x50)
> +#define PM800_OSC_CNTRL1_OSC_FREERUN_EN	BIT(1)
> +
>  #define PM800_GPADC0_MEAS1		0x54
>  #define PM800_GPADC0_MEAS2		0x55
>  #define PM800_GPADC1_MEAS1		0x56

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/4] mfd: devicetree: bindings: Add clock subdevice node information

[Stephen Boyd]

On 07/22/2015 10:08 PM, Krzysztof Kozlowski wrote:
> 2015-07-21 20:07 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>> This patch updates the binding documentation for optional
>> clocks node and related information for buffered 32KHz clock.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 28 +++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
> +Cc Stephen Boyd (clocks)
>
> We had a discussion whether clock bindings should be put in MFD
> bindings documentation or into separate file in bindings/clock but
> either way is fine for me:

No patch? :)

I think Rob has been pushing for MFDs to document everything within one 
file, but either way is fine from clk perspective.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/4] mfd: devicetree: bindings: Add clock subdevice node information

[Rob Herring]

On Thu, Jul 30, 2015 at 5:13 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 07/22/2015 10:08 PM, Krzysztof Kozlowski wrote:
>>
>> 2015-07-21 20:07 GMT+09:00 Vaibhav Hiremath <vaibhav.hiremath@linaro.org>:
>>>
>>> This patch updates the binding documentation for optional
>>> clocks node and related information for buffered 32KHz clock.
>>>
>>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>>> ---
>>>   Documentation/devicetree/bindings/mfd/88pm800.txt | 28
>>> +++++++++++++++++++++++
>>>   1 file changed, 28 insertions(+)
>>
>> +Cc Stephen Boyd (clocks)
>>
>> We had a discussion whether clock bindings should be put in MFD
>> bindings documentation or into separate file in bindings/clock but
>> either way is fine for me:
>
>
> No patch? :)
>
> I think Rob has been pushing for MFDs to document everything within one
> file, but either way is fine from clk perspective.

I don't recall that exactly. I did say that child bindings need to
point back to the MFD binding. So this is fine with me:

Acked-by: Rob Herring <robh@kernel.org>

Rob

Re: [PATCH 1/4] mfd: 88pm800: Update the header file with 32K clk related macros

[Vaibhav Hiremath]

On Thursday 23 July 2015 09:22 PM, Lee Jones wrote:
> On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:
>
>> Update header file with required macros for 32KHz buffered clock
>> output of 88PM800 family of device.
>> These macros will be used in clk provider driver.
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   include/linux/mfd/88pm80x.h | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/include/linux/mfd/88pm80x.h b/include/linux/mfd/88pm80x.h
>> index 05d9bad..680e4eb 100644
>> --- a/include/linux/mfd/88pm80x.h
>> +++ b/include/linux/mfd/88pm80x.h
>> @@ -91,6 +91,7 @@ enum {
>>   /* Referance and low power registers */
>>   #define PM800_LOW_POWER1		(0x20)
>>   #define PM800_LOW_POWER2		(0x21)
>> +#define PM800_LOW_POWER2_XO_LJ_EN	BIT(5)
>>
>>   #define PM800_LOW_POWER_CONFIG3		(0x22)
>>   #define PM800_LDOBK_FREEZE		BIT(7)
>> @@ -138,6 +139,13 @@ enum {
>>   #define PM800_ALARM			BIT(5)
>>   #define PM800_RTC1_USE_XO		BIT(7)
>>
>> +#define PM800_32K_OUTX_SEL_MASK		(0x3)
>> +/* 32KHz clk output sel mode */
>> +#define PM800_32K_OUTX_SEL_ZERO		(0x0)
>> +#define PM800_32K_OUTX_SEL_INT_32KHZ	(0x1)
>> +#define PM800_32K_OUTX_SEL_XO_32KHZ	(0x2)
>> +#define PM800_32K_OUTX_SEL_HIZ		(0x3)
>
> Why do these need to be in brackets?
>

No specific reason, just made it consistent with other definitions in
the file.

I will fix in V2.

Thanks,
Vaibhav

Re: [PATCH 4/4] mfd: 88pm800: Add support for clk subdevice

[Vaibhav Hiremath]

On Thursday 23 July 2015 09:20 PM, Lee Jones wrote:
> On Tue, 21 Jul 2015, Vaibhav Hiremath wrote:
>
>> This patch adds mfd_cell/clk-subdevice for 88PM800 MFD
>> (and family of devices).
>>
>> Signed-off-by: Vaibhav Hiremath <vaibhav.hiremath@linaro.org>
>> ---
>>   drivers/mfd/88pm800.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/mfd/88pm800.c b/drivers/mfd/88pm800.c
>> index f104a32..9723eac 100644
>> --- a/drivers/mfd/88pm800.c
>> +++ b/drivers/mfd/88pm800.c
>> @@ -173,6 +173,14 @@ static const struct mfd_cell regulator_devs[] = {
>>   	},
>>   };
>>
>> +static struct mfd_cell clk_devs[] = {
>> +	{
>> +	 .name = "88pm80x-clk",
>> +	 .of_compatible = "marvell,88pm800-clk",
>> +	 .id = -1,
>> +	},
>> +};
>
> Why does each device in 88pm800.c have it's own mfd_cell?
>

Probably, the author of the driver can comment on this. I have cc'ed
all of them.

But looking at the driver it looks like, except onkey and regulator all
other devices need per-initialization. For example,

in case of clock,

static int device_clk_init(struct pm80x_chip *chip)
{
         if (chip->type == CHIP_PM800)
                 clk_devs[0].name = "88pm800-clk";
         else if (chip->type == CHIP_PM860)
                 clk_devs[0].name = "88pm860-clk";

         return mfd_add_devices(chip->dev, 0, &clk_devs[0],
                               ARRAY_SIZE(clk_devs), NULL, 0, NULL);
}



> Take the opportunity to correct the tabbing in the remainder of the
> file.  Make that fix-up patch 1 of this set.  Then fixup this patch.
>

Ok, I will.

Thanks,
Vaibhav