[PATCH 0/2] clk: Support ROHM BD71837 (BD71847)

[PATCH 0/2] clk: Support ROHM BD71837 (BD71847)

[Matti Vaittinen]

This patch series adds support for clk gate included in ROHM BD71837
(and BD71847) Power Management ICs. Additionally patch series adds support
for new managed interfaces for clkdev lookup and parent_of_provider
registrations.

Please note that the patch has not been tested on BD71847 - I will
verify the BD71847 part and do appropriate documentation/naming changes
after verification. Hence only BD71837 support is announced here even
though the BD71847 should be identical what comes to clk registers.

BD71837 driver in the patch series depends on defines submitted as part of
this patch set:
https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/

The managed interfaces are continuation of discussion started here:
https://lore.kernel.org/lkml/20180730125550.GD8862@localhost.localdomain/
Difference to previous patch:
 - dropped devm_clk_register_clkdev
 - added new interfaces to Documentation/driver-model/devres.txt
 - did not rename __clk_register_clkdev
 - simplified IS_ERR - checks/code dublication by dropping checks from
   devm_clk_hw_register_clkdev.
   As negative point is this causes devres_alloc to be done before checking
   for IS_ERR(hw) inside do_clk_register_clkdev.

and BD71837 driver is continuation for patch discussion started here:
https://lore.kernel.org/lkml/d99c8762b0fbbcb18ec4f4d104191364c0ea798c.1528117485.git.matti.vaittinen@fi.rohmeurope.com/
Difference to previous patch:
 - Simply fail out if no parent clock is found from DT. This simplifies
   driver a lot and encourages proper clk tree modelling in DT.

patch series is based on linux-next tag next-20180823

Patch 1:
	Add new managed interfaces
Patch 2:
	Support BD71837

Matti Vaittinen (2):
  clk: clkdev/of_clk - add managed lookup and provider registrations
  clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

 Documentation/driver-model/devres.txt |   3 +
 drivers/clk/Kconfig                   |   6 ++
 drivers/clk/Makefile                  |   1 +
 drivers/clk/clk-bd718x7.c             | 131 ++++++++++++++++++++++++++++++++++
 drivers/clk/clk.c                     |  28 ++++++--
 drivers/clk/clkdev.c                  | 122 ++++++++++++++++++++++++-------
 include/linux/clk-provider.h          |   4 ++
 include/linux/clkdev.h                |   4 ++
 8 files changed, 267 insertions(+), 32 deletions(-)
 create mode 100644 drivers/clk/clk-bd718x7.c

-- 
2.14.3

[PATCH 1/2] clk: clkdev/of_clk - add managed lookup and provider registrations

[Matti Vaittinen]

With MFD devices the clk properties may be contained in MFD (parent) node.
Current devm_of_clk_add_hw_provider assumes the clk is bound to MFD subdevice
not to MFD device (parent). Add devm_of_clk_add_hw_provider_parent to tackle
this issue.

Also clkdev registration lacks of managed registration functions and it seems
quite many drivers do not drop clkdev lookups at exit. Add
devm_clk_hw_register_clkdev and devm_clk_release_clkdev to ease lookup
releasing at exit.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 Documentation/driver-model/devres.txt |   3 +
 drivers/clk/clk.c                     |  28 ++++++--
 drivers/clk/clkdev.c                  | 122 ++++++++++++++++++++++++++--------
 include/linux/clk-provider.h          |   4 ++
 include/linux/clkdev.h                |   4 ++
 5 files changed, 129 insertions(+), 32 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 15720cd17958..20dac9ffd3e0 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -238,6 +238,9 @@ CLOCK
   devm_clk_put()
   devm_clk_hw_register()
   devm_of_clk_add_hw_provider()
+  devm_of_clk_add_parent_hw_provider()
+  devm_clk_hw_register_clkdev()
+  devm_clk_release_clkdev()
 
 DMA
   dmaenginem_async_device_register()
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index d31055ae6ec6..fd0bc6421269 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3798,12 +3798,12 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
 	of_clk_del_provider(*(struct device_node **)res);
 }
 
-int devm_of_clk_add_hw_provider(struct device *dev,
+static int __devm_of_clk_add_hw_provider(struct device *dev,
 			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 					      void *data),
-			void *data)
+			struct device_node *of_node, void *data)
 {
-	struct device_node **ptr, *np;
+	struct device_node **ptr;
 	int ret;
 
 	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
@@ -3811,10 +3811,9 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 	if (!ptr)
 		return -ENOMEM;
 
-	np = dev->of_node;
-	ret = of_clk_add_hw_provider(np, get, data);
+	*ptr = of_node;
+	ret = of_clk_add_hw_provider(of_node, get, data);
 	if (!ret) {
-		*ptr = np;
 		devres_add(dev, ptr);
 	} else {
 		devres_free(ptr);
@@ -3822,8 +3821,25 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 
 	return ret;
 }
+int devm_of_clk_add_hw_provider(struct device *dev,
+			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+					      void *data),
+			void *data)
+{
+	return __devm_of_clk_add_hw_provider(dev, get, dev->of_node, data);
+}
 EXPORT_SYMBOL_GPL(devm_of_clk_add_hw_provider);
 
+int devm_of_clk_add_parent_hw_provider(struct device *dev,
+			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+					      void *data),
+			void *data)
+{
+	return __devm_of_clk_add_hw_provider(dev, get, dev->parent->of_node,
+					     data);
+}
+EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider);
+
 /**
  * of_clk_del_provider() - Remove a previously registered clock provider
  * @np: Device node pointer associated with clock provider
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..f6100b6e06fd 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -401,6 +401,25 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
 	return cl;
 }
 
+static int do_clk_register_clkdev(struct clk_hw *hw,
+	struct clk_lookup **cl, const char *con_id, const char *dev_id)
+{
+
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+	/*
+	 * Since dev_id can be NULL, and NULL is handled specially, we must
+	 * pass it as either a NULL format string, or with "%s".
+	 */
+	if (dev_id)
+		*cl = __clk_register_clkdev(hw, con_id, "%s",
+					   dev_id);
+	else
+		*cl = __clk_register_clkdev(hw, con_id, NULL);
+
+	return *cl ? 0 : -ENOMEM;
+}
+
 /**
  * clk_register_clkdev - register one clock lookup for a struct clk
  * @clk: struct clk to associate with all clk_lookups
@@ -420,20 +439,10 @@ int clk_register_clkdev(struct clk *clk, const char *con_id,
 {
 	struct clk_lookup *cl;
 
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
-
-	/*
-	 * Since dev_id can be NULL, and NULL is handled specially, we must
-	 * pass it as either a NULL format string, or with "%s".
-	 */
-	if (dev_id)
-		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, "%s",
-					   dev_id);
-	else
-		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, NULL);
-
-	return cl ? 0 : -ENOMEM;
+	if (!IS_ERR(clk))
+		return do_clk_register_clkdev(__clk_get_hw(clk), &cl, con_id,
+					      dev_id);
+	return PTR_ERR(clk);
 }
 EXPORT_SYMBOL(clk_register_clkdev);
 
@@ -456,18 +465,79 @@ int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id,
 {
 	struct clk_lookup *cl;
 
-	if (IS_ERR(hw))
-		return PTR_ERR(hw);
+	return do_clk_register_clkdev(hw, &cl, con_id, dev_id);
+}
+EXPORT_SYMBOL(clk_hw_register_clkdev);
 
-	/*
-	 * Since dev_id can be NULL, and NULL is handled specially, we must
-	 * pass it as either a NULL format string, or with "%s".
-	 */
-	if (dev_id)
-		cl = __clk_register_clkdev(hw, con_id, "%s", dev_id);
-	else
-		cl = __clk_register_clkdev(hw, con_id, NULL);
+static void devm_clkdev_release(struct device *dev, void *res)
+{
+	clkdev_drop(*(struct clk_lookup **)res);
+}
 
-	return cl ? 0 : -ENOMEM;
+static int devm_clk_match_clkdev(struct device *dev, void *res, void *data)
+{
+	struct clk_lookup **l = res;
+
+	if (!l || !*l) {
+		WARN_ON(!l || !*l);
+		return 0;
+	}
+	return *l == data;
 }
-EXPORT_SYMBOL(clk_hw_register_clkdev);
+
+/**
+ * devm_clk_release_clkdev - Resource managed clkdev lookup release
+ * @dev: device this lookup is bound
+ * @con_id: connection ID string on device
+ * @dev_id: format string describing device name
+ *
+ * Drop the clkdev lookup created with devm_clk_hw_register_clkdev.
+ * Normally this function will not need to be called and the resource
+ * management code will ensure that the resource is freed.
+ */
+void devm_clk_release_clkdev(struct device *dev, const char *con_id,
+			     const char *dev_id)
+{
+	struct clk_lookup *cl;
+	int rval;
+
+	cl = clk_find(dev_id, con_id);
+	WARN_ON(!cl);
+	rval = devres_release(dev, devm_clkdev_release,
+			      &devm_clk_match_clkdev, cl);
+	WARN_ON(rval);
+}
+EXPORT_SYMBOL(devm_clk_release_clkdev);
+
+/**
+ * devm_clk_hw_register_clkdev - managed clk lookup registration for clk_hw
+ * @dev: device this lookup is bound
+ * @hw: struct clk_hw to associate with all clk_lookups
+ * @con_id: connection ID string on device
+ * @dev_id: format string describing device name
+ *
+ * con_id or dev_id may be NULL as a wildcard, just as in the rest of
+ * clkdev.
+ *
+ * To make things easier for mass registration, we detect error clk_hws
+ * from a previous clk_hw_register_*() call, and return the error code for
+ * those.  This is to permit this function to be called immediately
+ * after clk_hw_register_*().
+ */
+int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw,
+				const char *con_id, const char *dev_id)
+{
+	int rval = -ENOMEM;
+	struct clk_lookup **cl;
+
+	cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL);
+	if (cl) {
+		rval = do_clk_register_clkdev(hw, cl, con_id, dev_id);
+		if (!rval)
+			devres_add(dev, cl);
+		else
+			devres_free(cl);
+	}
+	return rval;
+}
+EXPORT_SYMBOL(devm_clk_hw_register_clkdev);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 08b1aa70a38d..94f9bae6490a 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -909,6 +909,10 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 			   struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 						 void *data),
 			   void *data);
+int devm_of_clk_add_parent_hw_provider(struct device *dev,
+			   struct clk_hw *(*get)(struct of_phandle_args *clkspec,
+						 void *data),
+			   void *data);
 void of_clk_del_provider(struct device_node *np);
 void devm_of_clk_del_provider(struct device *dev);
 struct clk *of_clk_src_simple_get(struct of_phandle_args *clkspec,
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index 4890ff033220..ccb32af5848b 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -52,4 +52,8 @@ int clk_add_alias(const char *, const char *, const char *, struct device *);
 int clk_register_clkdev(struct clk *, const char *, const char *);
 int clk_hw_register_clkdev(struct clk_hw *, const char *, const char *);
 
+int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw,
+				const char *con_id, const char *dev_id);
+void devm_clk_release_clkdev(struct device *dev, const char *con_id,
+			     const char *dev_id);
 #endif
-- 
2.14.3

[PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Matti Vaittinen]

ROHM bd71837 and bd71847 contain 32768Hz clock gate. Support the clock
using generic clock framework. Note, only bd71837 is tested but bd71847
should be identical what comes to clk parts.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/Kconfig       |   6 +++
 drivers/clk/Makefile      |   1 +
 drivers/clk/clk-bd718x7.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 drivers/clk/clk-bd718x7.c

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 292056bbb30e..b2aee8fa4445 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -283,6 +283,12 @@ config COMMON_CLK_STM32H7
 	---help---
 	  Support for stm32h7 SoC family clocks
 
+config COMMON_CLK_BD718XX
+	tristate "Clock driver for ROHM BD71837 PMIC MFD"
+	depends on MFD_ROHM_BD718XX
+	help
+	  This driver supports ROHM BD71837 PMIC clock.
+
 source "drivers/clk/actions/Kconfig"
 source "drivers/clk/bcm/Kconfig"
 source "drivers/clk/hisilicon/Kconfig"
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index a84c5573cabe..886dce9f9bec 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -21,6 +21,7 @@ endif
 obj-$(CONFIG_MACH_ASM9260)		+= clk-asm9260.o
 obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)	+= clk-axi-clkgen.o
 obj-$(CONFIG_ARCH_AXXIA)		+= clk-axm5516.o
+obj-$(CONFIG_COMMON_CLK_BD718XX)	+= clk-bd718x7.o
 obj-$(CONFIG_COMMON_CLK_CDCE706)	+= clk-cdce706.o
 obj-$(CONFIG_COMMON_CLK_CDCE925)	+= clk-cdce925.o
 obj-$(CONFIG_ARCH_CLPS711X)		+= clk-clps711x.o
diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
new file mode 100644
index 000000000000..df5f1068ce8e
--- /dev/null
+++ b/drivers/clk/clk-bd718x7.c
@@ -0,0 +1,131 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// bd71837.c  -- ROHM BD71837MWV clock driver
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/mfd/rohm-bd718x7.h>
+#include <linux/clk-provider.h>
+#include <linux/clkdev.h>
+#include <linux/regmap.h>
+
+struct bd718xx_clk {
+	struct clk_hw hw;
+	u8 reg;
+	u8 mask;
+	struct platform_device *pdev;
+	struct bd718xx *mfd;
+};
+
+static int bd71837_clk_set(struct clk_hw *hw, int status)
+{
+	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+	return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
+}
+
+static void bd71837_clk_disable(struct clk_hw *hw)
+{
+	int rv;
+	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+	rv = bd71837_clk_set(hw, 0);
+	if (rv)
+		dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
+}
+
+static int bd71837_clk_enable(struct clk_hw *hw)
+{
+	return bd71837_clk_set(hw, 1);
+}
+
+static int bd71837_clk_is_enabled(struct clk_hw *hw)
+{
+	int enabled;
+	int rval;
+	struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
+
+	rval = regmap_read(c->mfd->regmap, c->reg, &enabled);
+
+	if (rval)
+		return rval;
+
+	return enabled & c->mask;
+}
+
+static const struct clk_ops bd71837_clk_ops = {
+	.prepare = &bd71837_clk_enable,
+	.unprepare = &bd71837_clk_disable,
+	.is_prepared = &bd71837_clk_is_enabled,
+};
+
+static int bd71837_clk_probe(struct platform_device *pdev)
+{
+	struct bd718xx_clk *c;
+	int rval = -ENOMEM;
+	const char *parent_clk;
+	struct device *parent = pdev->dev.parent;
+	struct bd718xx *mfd = dev_get_drvdata(parent);
+	struct clk_init_data init = {
+		.name = "bd718xx-32k-out",
+		.ops = &bd71837_clk_ops,
+	};
+
+	c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
+	if (!c)
+		return -ENOMEM;
+
+	init.num_parents = 1;
+	parent_clk = of_clk_get_parent_name(parent->of_node, 0);
+
+	init.parent_names = &parent_clk;
+	if (!parent_clk) {
+		dev_err(&pdev->dev, "No parent clk found\n");
+		return -EINVAL;
+	}
+
+	c->reg = BD718XX_REG_OUT32K;
+	c->mask = BD718XX_OUT32K_EN;
+	c->mfd = mfd;
+	c->pdev = pdev;
+	c->hw.init = &init;
+
+	of_property_read_string_index(parent->of_node,
+				      "clock-output-names", 0, &init.name);
+
+	rval = devm_clk_hw_register(&pdev->dev, &c->hw);
+	if (!rval) {
+		rval = devm_clk_hw_register_clkdev(&pdev->dev,
+						   &c->hw, init.name, NULL);
+		if (rval)
+			dev_warn(&pdev->dev, "Failed to register clkdev\n");
+		if (parent->of_node) {
+			rval = devm_of_clk_add_parent_hw_provider(&pdev->dev,
+					     of_clk_hw_simple_get, &c->hw);
+			if (rval)
+				dev_err(&pdev->dev,
+					"adding clk provider failed\n");
+		}
+	} else {
+		dev_err(&pdev->dev, "failed to register 32K clk");
+	}
+
+	return rval;
+}
+
+static struct platform_driver bd71837_clk = {
+	.driver = {
+		.name = "bd718xx-clk",
+	},
+	.probe = bd71837_clk_probe,
+};
+
+module_platform_driver(bd71837_clk);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD71837 chip clk driver");
+MODULE_LICENSE("GPL");
-- 
2.14.3

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5793 bytes --]

Hi Matti,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.19-rc1 next-20180830]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matti-Vaittinen/clk-clkdev-of_clk-add-managed-lookup-and-provider-registrations/20180831-045158
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/mfd/rohm-bd718x7.h:7:0,
                    from drivers/clk/clk-bd718x7.c:11:
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
>> drivers/clk/clk-bd718x7.c:28:34: error: dereferencing pointer to incomplete type 'struct bd718xx'
     return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
                                     ^
   include/linux/regmap.h:77:26: note: in definition of macro 'regmap_update_bits'
     regmap_update_bits_base(map, reg, mask, val, NULL, false, false)
                             ^~~
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_probe':
>> drivers/clk/clk-bd718x7.c:91:11: error: 'BD718XX_REG_OUT32K' undeclared (first use in this function); did you mean 'BD71837_REG_OUT32K'?
     c->reg = BD718XX_REG_OUT32K;
              ^~~~~~~~~~~~~~~~~~
              BD71837_REG_OUT32K
   drivers/clk/clk-bd718x7.c:91:11: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/clk/clk-bd718x7.c:92:12: error: 'BD718XX_OUT32K_EN' undeclared (first use in this function); did you mean 'BD71837_OUT32K_EN'?
     c->mask = BD718XX_OUT32K_EN;
               ^~~~~~~~~~~~~~~~~
               BD71837_OUT32K_EN
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
>> drivers/clk/clk-bd718x7.c:29:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +28 drivers/clk/clk-bd718x7.c

     4	
     5	#include <linux/kernel.h>
     6	#include <linux/module.h>
     7	#include <linux/init.h>
     8	#include <linux/err.h>
     9	#include <linux/platform_device.h>
    10	#include <linux/slab.h>
  > 11	#include <linux/mfd/rohm-bd718x7.h>
    12	#include <linux/clk-provider.h>
    13	#include <linux/clkdev.h>
    14	#include <linux/regmap.h>
    15	
    16	struct bd718xx_clk {
    17		struct clk_hw hw;
    18		u8 reg;
    19		u8 mask;
    20		struct platform_device *pdev;
    21		struct bd718xx *mfd;
    22	};
    23	
    24	static int bd71837_clk_set(struct clk_hw *hw, int status)
    25	{
    26		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    27	
  > 28		return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
  > 29	}
    30	
    31	static void bd71837_clk_disable(struct clk_hw *hw)
    32	{
    33		int rv;
    34		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    35	
    36		rv = bd71837_clk_set(hw, 0);
    37		if (rv)
    38			dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
    39	}
    40	
    41	static int bd71837_clk_enable(struct clk_hw *hw)
    42	{
    43		return bd71837_clk_set(hw, 1);
    44	}
    45	
    46	static int bd71837_clk_is_enabled(struct clk_hw *hw)
    47	{
    48		int enabled;
    49		int rval;
    50		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    51	
    52		rval = regmap_read(c->mfd->regmap, c->reg, &enabled);
    53	
    54		if (rval)
    55			return rval;
    56	
    57		return enabled & c->mask;
    58	}
    59	
    60	static const struct clk_ops bd71837_clk_ops = {
    61		.prepare = &bd71837_clk_enable,
    62		.unprepare = &bd71837_clk_disable,
    63		.is_prepared = &bd71837_clk_is_enabled,
    64	};
    65	
    66	static int bd71837_clk_probe(struct platform_device *pdev)
    67	{
    68		struct bd718xx_clk *c;
    69		int rval = -ENOMEM;
    70		const char *parent_clk;
    71		struct device *parent = pdev->dev.parent;
    72		struct bd718xx *mfd = dev_get_drvdata(parent);
    73		struct clk_init_data init = {
    74			.name = "bd718xx-32k-out",
    75			.ops = &bd71837_clk_ops,
    76		};
    77	
    78		c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
    79		if (!c)
    80			return -ENOMEM;
    81	
    82		init.num_parents = 1;
    83		parent_clk = of_clk_get_parent_name(parent->of_node, 0);
    84	
    85		init.parent_names = &parent_clk;
    86		if (!parent_clk) {
    87			dev_err(&pdev->dev, "No parent clk found\n");
    88			return -EINVAL;
    89		}
    90	
  > 91		c->reg = BD718XX_REG_OUT32K;
  > 92		c->mask = BD718XX_OUT32K_EN;
    93		c->mfd = mfd;
    94		c->pdev = pdev;
    95		c->hw.init = &init;
    96	
    97		of_property_read_string_index(parent->of_node,
    98					      "clock-output-names", 0, &init.name);
    99	
   100		rval = devm_clk_hw_register(&pdev->dev, &c->hw);
   101		if (!rval) {
   102			rval = devm_clk_hw_register_clkdev(&pdev->dev,
   103							   &c->hw, init.name, NULL);
   104			if (rval)
   105				dev_warn(&pdev->dev, "Failed to register clkdev\n");
   106			if (parent->of_node) {
   107				rval = devm_of_clk_add_parent_hw_provider(&pdev->dev,
   108						     of_clk_hw_simple_get, &c->hw);
   109				if (rval)
   110					dev_err(&pdev->dev,
   111						"adding clk provider failed\n");
   112			}
   113		} else {
   114			dev_err(&pdev->dev, "failed to register 32K clk");
   115		}
   116	
   117		return rval;
   118	}
   119	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 65707 bytes --]

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Matti Vaittinen]

Hello All,

Just wanted to point out for the reviewers that this patch depends on
not yet accepted MFD/regulator patch. (struct/defines in
include/linux/mfd/rohm-bd718x7.h were changed)
https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/

On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> Hi Matti,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on clk/clk-next]
> [also build test ERROR on v4.19-rc1 next-20180830]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

Thus this error was expected. What is the generally best way to work
when there is changes to more than one subsystem? With this approach
the patch set here won't compile until MFD part gets applied. But if
I use old definitions/structs here, then clk tree gets broken when
MFD/regulator part changes defines. I see only bad and worse options =)
Anyways, I guess sending this patch with new defines (and applying it
only after MFD) is still better than applying this with old defines and
breaking it when MFD changes. (Assuming all of the patches get applied
at some point).

Br,
	Matti Vaittinen
> 

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4957 bytes --]

Hi Matti,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on clk/clk-next]
[also build test ERROR on v4.19-rc1 next-20180831]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matti-Vaittinen/clk-clkdev-of_clk-add-managed-lookup-and-provider-registrations/20180831-045158
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
   drivers/clk/clk-bd718x7.c:28:39: error: dereferencing pointer to incomplete type 'struct bd718xx'
     return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
                                          ^~
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_probe':
   drivers/clk/clk-bd718x7.c:91:11: error: 'BD718XX_REG_OUT32K' undeclared (first use in this function); did you mean 'BD71837_REG_OUT32K'?
     c->reg = BD718XX_REG_OUT32K;
              ^~~~~~~~~~~~~~~~~~
              BD71837_REG_OUT32K
   drivers/clk/clk-bd718x7.c:91:11: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/clk/clk-bd718x7.c:92:12: error: 'BD718XX_OUT32K_EN' undeclared (first use in this function); did you mean 'BD718XX_REG_OUT32K'?
     c->mask = BD718XX_OUT32K_EN;
               ^~~~~~~~~~~~~~~~~
               BD718XX_REG_OUT32K
   drivers/clk/clk-bd718x7.c: In function 'bd71837_clk_set':
   drivers/clk/clk-bd718x7.c:29:1: warning: control reaches end of non-void function [-Wreturn-type]
    }
    ^

vim +92 drivers/clk/clk-bd718x7.c

    23	
    24	static int bd71837_clk_set(struct clk_hw *hw, int status)
    25	{
    26		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    27	
  > 28		return regmap_update_bits(c->mfd->regmap, c->reg, c->mask, status);
    29	}
    30	
    31	static void bd71837_clk_disable(struct clk_hw *hw)
    32	{
    33		int rv;
    34		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    35	
    36		rv = bd71837_clk_set(hw, 0);
    37		if (rv)
    38			dev_dbg(&c->pdev->dev, "Failed to disable 32K clk (%d)\n", rv);
    39	}
    40	
    41	static int bd71837_clk_enable(struct clk_hw *hw)
    42	{
    43		return bd71837_clk_set(hw, 1);
    44	}
    45	
    46	static int bd71837_clk_is_enabled(struct clk_hw *hw)
    47	{
    48		int enabled;
    49		int rval;
    50		struct bd718xx_clk *c = container_of(hw, struct bd718xx_clk, hw);
    51	
    52		rval = regmap_read(c->mfd->regmap, c->reg, &enabled);
    53	
    54		if (rval)
    55			return rval;
    56	
    57		return enabled & c->mask;
    58	}
    59	
    60	static const struct clk_ops bd71837_clk_ops = {
    61		.prepare = &bd71837_clk_enable,
    62		.unprepare = &bd71837_clk_disable,
    63		.is_prepared = &bd71837_clk_is_enabled,
    64	};
    65	
    66	static int bd71837_clk_probe(struct platform_device *pdev)
    67	{
    68		struct bd718xx_clk *c;
    69		int rval = -ENOMEM;
    70		const char *parent_clk;
    71		struct device *parent = pdev->dev.parent;
    72		struct bd718xx *mfd = dev_get_drvdata(parent);
    73		struct clk_init_data init = {
    74			.name = "bd718xx-32k-out",
    75			.ops = &bd71837_clk_ops,
    76		};
    77	
    78		c = devm_kzalloc(&pdev->dev, sizeof(*c), GFP_KERNEL);
    79		if (!c)
    80			return -ENOMEM;
    81	
    82		init.num_parents = 1;
    83		parent_clk = of_clk_get_parent_name(parent->of_node, 0);
    84	
    85		init.parent_names = &parent_clk;
    86		if (!parent_clk) {
    87			dev_err(&pdev->dev, "No parent clk found\n");
    88			return -EINVAL;
    89		}
    90	
    91		c->reg = BD718XX_REG_OUT32K;
  > 92		c->mask = BD718XX_OUT32K_EN;
    93		c->mfd = mfd;
    94		c->pdev = pdev;
    95		c->hw.init = &init;
    96	
    97		of_property_read_string_index(parent->of_node,
    98					      "clock-output-names", 0, &init.name);
    99	
   100		rval = devm_clk_hw_register(&pdev->dev, &c->hw);
   101		if (!rval) {
   102			rval = devm_clk_hw_register_clkdev(&pdev->dev,
   103							   &c->hw, init.name, NULL);
   104			if (rval)
   105				dev_warn(&pdev->dev, "Failed to register clkdev\n");
   106			if (parent->of_node) {
   107				rval = devm_of_clk_add_parent_hw_provider(&pdev->dev,
   108						     of_clk_hw_simple_get, &c->hw);
   109				if (rval)
   110					dev_err(&pdev->dev,
   111						"adding clk provider failed\n");
   112			}
   113		} else {
   114			dev_err(&pdev->dev, "failed to register 32K clk");
   115		}
   116	
   117		return rval;
   118	}
   119	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 64474 bytes --]

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Stephen Boyd]

Quoting Matti Vaittinen (2018-08-31 03:21:23)
> Hello All,
> 
> Just wanted to point out for the reviewers that this patch depends on
> not yet accepted MFD/regulator patch. (struct/defines in
> include/linux/mfd/rohm-bd718x7.h were changed)
> https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> 
> On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> > Hi Matti,
> > 
> > Thank you for the patch! Yet something to improve:
> > 
> > [auto build test ERROR on clk/clk-next]
> > [also build test ERROR on v4.19-rc1 next-20180830]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> Thus this error was expected. What is the generally best way to work
> when there is changes to more than one subsystem? With this approach
> the patch set here won't compile until MFD part gets applied. But if
> I use old definitions/structs here, then clk tree gets broken when
> MFD/regulator part changes defines. I see only bad and worse options =)
> Anyways, I guess sending this patch with new defines (and applying it
> only after MFD) is still better than applying this with old defines and
> breaking it when MFD changes. (Assuming all of the patches get applied
> at some point).
> 

Does anything besides the clk driver need the defines that are in the
header which are used in this file? If not, then it's better to put
those defines in the C file for the clk driver so we don't have to hop
between files and have merge dependencies. Also, the regmap could
possibly by grabbed from the dev->parent pointer instead of passing it
down through an mfd structure, allowing this driver to be more generic
assuming it is always a child of some mfd device that has a regmap.

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Matti Vaittinen]

Hello Stephen

On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > Hello All,
> > 
> > Just wanted to point out for the reviewers that this patch depends on
> > not yet accepted MFD/regulator patch. (struct/defines in
> > include/linux/mfd/rohm-bd718x7.h were changed)
> > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> > 
> > On Fri, Aug 31, 2018 at 09:18:19AM +0800, kbuild test robot wrote:
> > > Hi Matti,
> > > 
> > > Thank you for the patch! Yet something to improve:
> > > 
> > > [auto build test ERROR on clk/clk-next]
> > > [also build test ERROR on v4.19-rc1 next-20180830]
> > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> > 
> > Thus this error was expected. What is the generally best way to work
> > when there is changes to more than one subsystem? With this approach
> > the patch set here won't compile until MFD part gets applied. But if
> > I use old definitions/structs here, then clk tree gets broken when
> > MFD/regulator part changes defines. I see only bad and worse options =)
> > Anyways, I guess sending this patch with new defines (and applying it
> > only after MFD) is still better than applying this with old defines and
> > breaking it when MFD changes. (Assuming all of the patches get applied
> > at some point).
> > 
> 
> Does anything besides the clk driver need the defines that are in the
> header which are used in this file?

The register address for clk enable and bit mask are not used by any
other driver. But I'd like to keep all the register addresses in one
enum. That eases checking changes when new chips come.

The private data for MFD (which contains for example the regmap and chip
type) are required by other drivers too.

> If not, then it's better to put
> those defines in the C file for the clk driver so we don't have to hop
> between files and have merge dependencies. Also, the regmap could
> possibly by grabbed from the dev->parent pointer instead of passing it
> down through an mfd structure, allowing this driver to be more generic
> assuming it is always a child of some mfd device that has a regmap.

Currently clk only uses regmap directly from the struct bd718xx. But I
would like to have the access to chip_type as well. And even if we forget
the chip_type. Well, I only see two bad ways of omitting the struct
bd718xx:

1. Always keep the regmap as first member in parent device's driver_data.
Then we can just get the driver data and do cast to regmap. I am not fan
of this as it breaks as fast as someone changes the struct bd718xx - and
as this is part of MFD - well, there is no guarantees the clk people are
even reviewing such change. Also if we need the chip type we are back
using the struct.

2. Use some regmnap wrapper functions which would only take the parent
dev pointer and dig out the regmap details. This would be doable but
then we still have dependency to these wrappers (so dependency is not
solved). Additionally these wrappers are something we decided to get rid
of few patch versions ago.

I don't expect much renamings to take place after the latest patch set
to MFD/regulator tree has been applied. So maybe the clk patch set can
just wait untill MFD/regulator stuff gets integrated. I would be
gratefull if we could review the clk patch set already now though so I
could do improvements/changes to clk side while waiting for
MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
not depending on MFD - so maybe we can get them reviewed/applied? If I
have the spare time I can look if I can clean up some of those not freed
clkdev lookups.

Br,
	Matti Vaittinen
> 

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Matti Vaittinen]

Hello Stephen & All,

On Mon, Sep 03, 2018 at 09:38:43AM +0300, Matti Vaittinen wrote:
> 
> On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > > Hello All,
> > > 
> > > Just wanted to point out for the reviewers that this patch depends on
> > > not yet accepted MFD/regulator patch. (struct/defines in
> > > include/linux/mfd/rohm-bd718x7.h were changed)
> > > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> > 
> > Does anything besides the clk driver need the defines that are in the
> > header which are used in this file?
> 
> The register address for clk enable and bit mask are not used by any
> other driver. But I'd like to keep all the register addresses in one
> enum. That eases checking changes when new chips come.
> 
> The private data for MFD (which contains for example the regmap and chip
> type) are required by other drivers too.
> 
> > If not, then it's better to put
> > those defines in the C file for the clk driver so we don't have to hop
> > between files and have merge dependencies. Also, the regmap could
> > possibly by grabbed from the dev->parent pointer instead of passing it
> > down through an mfd structure, allowing this driver to be more generic
> > assuming it is always a child of some mfd device that has a regmap.
> 
> Currently clk only uses regmap directly from the struct bd718xx. But I
> would like to have the access to chip_type as well. And even if we forget
> the chip_type. Well, I only see two bad ways of omitting the struct
> bd718xx:
> 
> 1. Always keep the regmap as first member in parent device's driver_data.
> Then we can just get the driver data and do cast to regmap. I am not fan
> of this as it breaks as fast as someone changes the struct bd718xx - and
> as this is part of MFD - well, there is no guarantees the clk people are
> even reviewing such change. Also if we need the chip type we are back
> using the struct.
> 
> 2. Use some regmnap wrapper functions which would only take the parent
> dev pointer and dig out the regmap details. This would be doable but
> then we still have dependency to these wrappers (so dependency is not
> solved). Additionally these wrappers are something we decided to get rid
> of few patch versions ago.
> 
> I don't expect much renamings to take place after the latest patch set
> to MFD/regulator tree has been applied. So maybe the clk patch set can
> just wait untill MFD/regulator stuff gets integrated. I would be
> gratefull if we could review the clk patch set already now though so I
> could do improvements/changes to clk side while waiting for
> MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
> not depending on MFD - so maybe we can get them reviewed/applied? If I
> have the spare time I can look if I can clean up some of those not freed
> clkdev lookups.
> 
The MFD renaming patch has now been merged to clk-next. So dependencies
to MFD should now be in-tree. I will rebase this to clk-next and do
resend. This patch still requires the devm variants for clkdev lookups
an devm variant of "parent of provider"-registration. I did submit those
as part of the cleanup series:
https://lore.kernel.org/linux-clk/cover.1541161503.git.matti.vaittinen@fi.rohmeurope.com/

My question is if you prefer me to add this driver in that series and
resend the series as v4? Or do you want me to send this in own
individual patch with just a note about the dependency?

Best Regards
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

Re: [PATCH 2/2] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Stephen Boyd]

Quoting Matti Vaittinen (2018-11-11 23:45:53)
> Hello Stephen & All,
> 
> On Mon, Sep 03, 2018 at 09:38:43AM +0300, Matti Vaittinen wrote:
> > 
> > On Sat, Sep 01, 2018 at 12:13:26PM -0700, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2018-08-31 03:21:23)
> > > > Hello All,
> > > > 
> > > > Just wanted to point out for the reviewers that this patch depends on
> > > > not yet accepted MFD/regulator patch. (struct/defines in
> > > > include/linux/mfd/rohm-bd718x7.h were changed)
> > > > https://lore.kernel.org/lkml/cover.1535545377.git.matti.vaittinen@fi.rohmeurope.com/
> > > 
> > > Does anything besides the clk driver need the defines that are in the
> > > header which are used in this file?
> > 
> > The register address for clk enable and bit mask are not used by any
> > other driver. But I'd like to keep all the register addresses in one
> > enum. That eases checking changes when new chips come.
> > 
> > The private data for MFD (which contains for example the regmap and chip
> > type) are required by other drivers too.
> > 
> > > If not, then it's better to put
> > > those defines in the C file for the clk driver so we don't have to hop
> > > between files and have merge dependencies. Also, the regmap could
> > > possibly by grabbed from the dev->parent pointer instead of passing it
> > > down through an mfd structure, allowing this driver to be more generic
> > > assuming it is always a child of some mfd device that has a regmap.
> > 
> > Currently clk only uses regmap directly from the struct bd718xx. But I
> > would like to have the access to chip_type as well. And even if we forget
> > the chip_type. Well, I only see two bad ways of omitting the struct
> > bd718xx:
> > 
> > 1. Always keep the regmap as first member in parent device's driver_data.
> > Then we can just get the driver data and do cast to regmap. I am not fan
> > of this as it breaks as fast as someone changes the struct bd718xx - and
> > as this is part of MFD - well, there is no guarantees the clk people are
> > even reviewing such change. Also if we need the chip type we are back
> > using the struct.
> > 
> > 2. Use some regmnap wrapper functions which would only take the parent
> > dev pointer and dig out the regmap details. This would be doable but
> > then we still have dependency to these wrappers (so dependency is not
> > solved). Additionally these wrappers are something we decided to get rid
> > of few patch versions ago.
> > 
> > I don't expect much renamings to take place after the latest patch set
> > to MFD/regulator tree has been applied. So maybe the clk patch set can
> > just wait untill MFD/regulator stuff gets integrated. I would be
> > gratefull if we could review the clk patch set already now though so I
> > could do improvements/changes to clk side while waiting for
> > MFD/regulator to get applied. Also, the devm_ functions (patch 1) are
> > not depending on MFD - so maybe we can get them reviewed/applied? If I
> > have the spare time I can look if I can clean up some of those not freed
> > clkdev lookups.
> > 
> The MFD renaming patch has now been merged to clk-next. So dependencies
> to MFD should now be in-tree. I will rebase this to clk-next and do
> resend. This patch still requires the devm variants for clkdev lookups
> an devm variant of "parent of provider"-registration. I did submit those
> as part of the cleanup series:
> https://lore.kernel.org/linux-clk/cover.1541161503.git.matti.vaittinen@fi.rohmeurope.com/
> 
> My question is if you prefer me to add this driver in that series and
> resend the series as v4? Or do you want me to send this in own
> individual patch with just a note about the dependency?
> 

Either way is fine. Just be explicit and we'll figure it out. Of course,
be aware that kbuild robot may complain if it can't figure out the
dependency and something fails to compile so it may be easier to resend
the whole combined series as something that compiles.