[PATCH v4 0/8] clk: clkdev: managed clk lookup and provider registrations

[PATCH v4 0/8] clk: clkdev: managed clk lookup and provider registrations

[Matti Vaittinen]

Series add bd71837/bd71837 PMIC clock support + managed interfaces

Few clk drivers appear to be leaking clkdev lookup registrations at
driver remove. The patch series adds devm versions of lookup
registrations and cleans up few drivers. Driver clean-up patches have
not been tested as I lack the HW. All testing and comments if
driver/device removal is even possible for changed drivers is highly
appreciated. If removal is not possible I will gladly drop the patches
from series - although leaking lookups may serve as bad example for new
developers =)

Patch 8 adds support for clock gate in ROHM bd71837 and bd71847 PMICs.
This change is included in the series because it depends on new managed
interfaces introduced in this series.

bd718x7 driver and devm interfaces are tested on BeagleBoneBlack and 
bd71837 break-out board. Clk area register interface of bd71847 is
identical to bd71837.

Changed drivers are:
clk-max77686, clk-st, clk-hi655x, rk808, clk-twl6040
and apcs-msm8916. New driver is clk-bd718x7

This series has been discussed for a while now. For those who want to
see whole discussion:

The bd71837 driver was originally proposed here
https://lore.kernel.org/lkml/d99c8762b0fbbcb18ec4f4d104191364c0ea798c.1528117485.git.matti.vaittinen@fi.rohmeurope.com/

clk portion was separated from that series and devm variants were
proposed here
https://lore.kernel.org/linux-clk/cover.1535630942.git.matti.vaittinen@fi.rohmeurope.com/

Cleanup to other drivers was initiated in this series while waiting for
MFD portions of bd718x7 to be applied. And now when MFD dependencies are in-tree
his version (4) combines bd718x7 driver back to this series.

Changelog (for this series) v4
- Add support for ROHM bd718x7 PMIC clock gate. Included in this patch
  series because it depends on managed interfaces added in patch 1.

Changelog (for this series) v3
Address issues spotted by Krzysztof Kozlowski
- Drop patch 3 for clk-s3c2410-dclk as this device can never be removed
- Fix indentiation for clk-max77686
- Rest  of the patches are unchanged.

Changelog (for this series) v2
Issue spotted by 0-Day test suite
- Add a stub function 'devm_of_clk_add_parent_hw_provider' for no OF config.
- patches 2-8 are unchanged.

This patch series is based on clk-next

---

Matti Vaittinen (8):
  clk: clkdev/of_clk - add managed lookup and provider registrations
  clk: clk-max77686: Clean clkdev lookup leak and use devm
  clk: clk-st: avoid clkdev lookup leak at remove
  clk: clk-hi655x: Free of_provider at remove
  clk: rk808: use managed version of of_provider registration
  clk: clk-twl6040: Free of_provider at remove
  clk: apcs-msm8916: simplify probe cleanup by using devm
  clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

 Documentation/driver-model/devres.txt |   3 +
 drivers/clk/Kconfig                   |   7 ++
 drivers/clk/Makefile                  |   1 +
 drivers/clk/clk-bd718x7.c             | 131 ++++++++++++++++++++++++++++++++++
 drivers/clk/clk-hi655x.c              |   4 +-
 drivers/clk/clk-max77686.c            |  29 ++------
 drivers/clk/clk-rk808.c               |  15 +---
 drivers/clk/clk-twl6040.c             |   5 +-
 drivers/clk/clk.c                     |  28 ++++++--
 drivers/clk/clkdev.c                  | 122 ++++++++++++++++++++++++-------
 drivers/clk/qcom/apcs-msm8916.c       |   5 +-
 drivers/clk/x86/clk-st.c              |   3 +-
 include/linux/clk-provider.h          |  11 +++
 include/linux/clkdev.h                |   4 ++
 14 files changed, 292 insertions(+), 76 deletions(-)
 create mode 100644 drivers/clk/clk-bd718x7.c

-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Matti Vaittinen]

With MFD devices the clk properties may be contained in MFD (parent) DT
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 few 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          |  11 +++
 include/linux/clkdev.h                |   4 ++
 5 files changed, 136 insertions(+), 32 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 43681ca0837f..fac63760b01c 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 af011974d4ec..9bb921eb90f6 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3893,12 +3893,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),
@@ -3906,10 +3906,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);
@@ -3917,8 +3916,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 60c51871b04b..a6663f084cf1 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -916,6 +916,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,
@@ -953,6 +957,13 @@ static inline int devm_of_clk_add_hw_provider(struct device *dev,
 {
 	return 0;
 }
+static inline 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 0;
+}
 static inline void of_clk_del_provider(struct device_node *np) {}
 static inline void devm_of_clk_del_provider(struct device *dev) {}
 static inline struct clk *of_clk_src_simple_get(
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


-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Stephen Boyd]

Quoting Matti Vaittinen (2018-11-13 03:55:58)
> With MFD devices the clk properties may be contained in MFD (parent) DT
> 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 few 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.

Please split this into clkdev and non-clkdev devm functionality.

> 
> 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          |  11 +++
>  include/linux/clkdev.h                |   4 ++
>  5 files changed, 136 insertions(+), 32 deletions(-)
> 
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index 43681ca0837f..fac63760b01c 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()

The 'release' or non-common functions shouldn't be documented here.

>  
>  DMA
>    dmaenginem_async_device_register()
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index af011974d4ec..9bb921eb90f6 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3893,12 +3893,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),
> @@ -3906,10 +3906,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;

Why is this moved outside of the if condition? In fact, why isn't just
the first line in this hunk deleted and passed to this function as
struct device_node *np?

>                 devres_add(dev, ptr);
>         } else {
>                 devres_free(ptr);
> @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev,
>  
>         return ret;
>  }

Nitpick: Add a newline here.

> +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,

I'm wondering if we can somehow auto-detect this in
devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
If it isn't there, then we go to the parent node and look for a
#clock-cells property there in the DT node for that device. Does that
make sense? Then there isn't any new API and we can attach the lifetime
of the devm registration to the presence of the property indicating this
is a clk controller or not.

> +                                            data);
> +}
> +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider);

Can we get some kernel doc on these functions?

> +
>  /**
>   * 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
[...]
> +
> +/**
> + * 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);

Nitpick: Drop & on functions taken as pointers.

> +       WARN_ON(rval);
> +}
> +EXPORT_SYMBOL(devm_clk_release_clkdev);

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

[Matti Vaittinen]

Hello Stephen,

Thanks a bunch for taking the time and reviewing this!

On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > With MFD devices the clk properties may be contained in MFD (parent) DT
> > 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 few 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.
> 
> Please split this into clkdev and non-clkdev devm functionality.
Allright. I'll split this to two patches.

> > --- 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()
> 
> The 'release' or non-common functions shouldn't be documented here.
So I will drop the line mentioning devm_clk_release_clkdev()

> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index af011974d4ec..9bb921eb90f6 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3893,12 +3893,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),
> > @@ -3906,10 +3906,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;
> 
> Why is this moved outside of the if condition?
I completely removed the local variable np and just unconditionally set
the allocated devres to point at the node (if allocation succeeded). We
could of course only do this if the provider registration succeeded and
save one assignment - but I guess I intended to remove the curly braces
and thus decided to go for one liner after if. But apparently I didn't
remove the braces O_o. Well, I can put the assignment inside the
condition if you prefer that.

> In fact, why isn't just
> the first line in this hunk deleted and passed to this function as
> struct device_node *np?

I am sorry but I don't quite follow your suggestion here. Do you mean we
could just pass the struct device_node *np in devres_add()? I thought
the pointer passed to devress_add() should be allocated using
devres_alloc. Can you please elaborate what you mean?

> 
> >                 devres_add(dev, ptr);
> >         } else {
> >                 devres_free(ptr);
> > @@ -3917,8 +3916,25 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >  
> >         return ret;
> >  }
> 
> Nitpick: Add a newline here.

Will do.

> 
> > +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,
> 
> I'm wondering if we can somehow auto-detect this in
> devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
> If it isn't there, then we go to the parent node and look for a
> #clock-cells property there in the DT node for that device. Does that
> make sense? Then there isn't any new API and we can attach the lifetime
> of the devm registration to the presence of the property indicating this
> is a clk controller or not.

Huh. I don't know why but building this kind of logic in core is a bit
scary to me. I guess I can try implementing something like this - but I
am not really a fan of this. (Accidentally) omit the #clock-cells from
node and we go to parent node - I am a novice on this area but this
sounds like a potential hazard to me. I believe the driver should know
if it's properties should be in own or parent node - and if they are
not, then there should be no guessing but error. The lifetime is topic
where I would like to get information from you who know the kernel
better than I do =) But I guess the parent node is there at least as
long as the child device is alive. So for me the life time of
get-callback is more crucial - but as I said, I don't understand the
kernel in details so you probably know it better than me. But please let
me know your final take on this and I will follow the guidance =)

> 
> > +                                            data);
> > +}
> > +EXPORT_SYMBOL_GPL(devm_of_clk_add_parent_hw_provider);
> 
> Can we get some kernel doc on these functions?
Sure. I will add the doc. Reason why I didn't do it is that the current
devm_of_clk_add_hw_provider() did not have doc. But I'll add that in
next patch.

> > +       rval = devres_release(dev, devm_clkdev_release,
> > +                             &devm_clk_match_clkdev, cl);
> 
> Nitpick: Drop & on functions taken as pointers.
Ok. Will do.

-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Russell King - ARM Linux]

On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote:
> Hello Stephen,
> 
> Thanks a bunch for taking the time and reviewing this!
> 
> On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > > With MFD devices the clk properties may be contained in MFD (parent) DT
> > > 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 few 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.
> > 
> > Please split this into clkdev and non-clkdev devm functionality.
> Allright. I'll split this to two patches.

Definitely, please note that clkdev is separate from the common
clk stuff, and is actually maintained by a different person (me)
although I currently defer much of the review to the common clk
guys.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

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

[Matti Vaittinen]

On Fri, Nov 30, 2018 at 11:06:53AM +0000, Russell King - ARM Linux wrote:
> On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote:
> > Hello Stephen,
> > 
> > Thanks a bunch for taking the time and reviewing this!
> > 
> > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > > > With MFD devices the clk properties may be contained in MFD (parent) DT
> > > > 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 few 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.
> > > 
> > > Please split this into clkdev and non-clkdev devm functionality.
> > Allright. I'll split this to two patches.
> 
> Definitely, please note that clkdev is separate from the common
> clk stuff, and is actually maintained by a different person (me)
> although I currently defer much of the review to the common clk
> guys.

Right. This clarifies how clkdev is maintained. Thanks for letting me
know. Is it still Ok to do these changes through linux-clk tree or is
there some other tree where the devm portion should be submitted? If so,
how should the dependencies be handled?

Br,
	Matti Vaittinen

> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Matti Vaittinen]

Hello Stephen & All,

I created v5 where I fixed obvious issues. I'll send it in few minutes.
Please note following topics:

On Fri, Nov 30, 2018 at 12:50:22PM +0200, Matti Vaittinen wrote:
> 
> On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > >  
> > > -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),
> > > @@ -3906,10 +3906,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;
> > 
> > Why is this moved outside of the if condition?
> I completely removed the local variable np and just unconditionally set
> the allocated devres to point at the node (if allocation succeeded). We
> could of course only do this if the provider registration succeeded and
> save one assignment - but I guess I intended to remove the curly braces
> and thus decided to go for one liner after if. But apparently I didn't
> remove the braces O_o. Well, I can put the assignment inside the
> condition if you prefer that.
> 
> > In fact, why isn't just
> > the first line in this hunk deleted and passed to this function as
> > struct device_node *np?
> 
> I am sorry but I don't quite follow your suggestion here. Do you mean we
> could just pass the struct device_node *np in devres_add()? I thought
> the pointer passed to devress_add() should be allocated using
> devres_alloc. Can you please elaborate what you mean?

I could not really spot what to fix in patched code (see below).

static int __devm_of_clk_add_hw_provider(struct device *dev,
			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
					      void *data),
			struct device_node *of_node, void *data)
{
	struct device_node **ptr;
	int ret;

	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
			   GFP_KERNEL);
	if (!ptr)
		return -ENOMEM;

	*ptr = of_node;
	ret = of_clk_add_hw_provider(of_node, get, data);
	if (!ret)
		devres_add(dev, ptr);
	else
		devres_free(ptr);

	return ret;
}

As far as I understand we need to allocate the ptr using devres_alloc.
We also need to pass this ptr to of_clk_add_hw_provider - and we must
assign our node to the *ptr. (I removed the extra braces - this change
is laso included in v5 but I don't see how we should improve). Can you
please explain me if you still wish to me change this further?

> > > +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,
> > 
> > I'm wondering if we can somehow auto-detect this in
> > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
> > If it isn't there, then we go to the parent node and look for a
> > #clock-cells property there in the DT node for that device. Does that
> > make sense? Then there isn't any new API and we can attach the lifetime
> > of the devm registration to the presence of the property indicating this
> > is a clk controller or not.
> 
> Huh. I don't know why but building this kind of logic in core is a bit
> scary to me. I guess I can try implementing something like this - but I
> am not really a fan of this. (Accidentally) omit the #clock-cells from
> node and we go to parent node - I am a novice on this area but this
> sounds like a potential hazard to me. I believe the driver should know
> if it's properties should be in own or parent node - and if they are
> not, then there should be no guessing but error. The lifetime is topic
> where I would like to get information from you who know the kernel
> better than I do =) But I guess the parent node is there at least as
> long as the child device is alive. So for me the life time of
> get-callback is more crucial - but as I said, I don't understand the
> kernel in details so you probably know it better than me. But please let
> me know your final take on this and I will follow the guidance =)

I did not put the 'auto-detection' for provider node in the patch v5 as
it really gives me bad vibes :) Maybe it is just my pessimistic nature
but I do expect that problems will arise when we accidentally end up in
parent node when this is not the purpose. I would rather keep this
simple by adding one specific API function more - and keeping the
existing API specific as well. But I can do v5 if you insist on having
this auto-detection.

-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Stephen Boyd]

Quoting Matti Vaittinen (2018-11-30 02:50:22)
> Hello Stephen,
> 
> Thanks a bunch for taking the time and reviewing this!
> 
> On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > > With MFD devices the clk properties may be contained in MFD (parent) DT
> > > 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 few 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.
> > 
> > Please split this into clkdev and non-clkdev devm functionality.
> Allright. I'll split this to two patches.
> 
> > > --- 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()
> > 
> > The 'release' or non-common functions shouldn't be documented here.
> So I will drop the line mentioning devm_clk_release_clkdev()
> 
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index af011974d4ec..9bb921eb90f6 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -3893,12 +3893,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),
> > > @@ -3906,10 +3906,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;
> > 
> > Why is this moved outside of the if condition?
> I completely removed the local variable np and just unconditionally set
> the allocated devres to point at the node (if allocation succeeded). We
> could of course only do this if the provider registration succeeded and
> save one assignment - but I guess I intended to remove the curly braces
> and thus decided to go for one liner after if. But apparently I didn't
> remove the braces O_o. Well, I can put the assignment inside the
> condition if you prefer that.
> 
> > In fact, why isn't just
> > the first line in this hunk deleted and passed to this function as
> > struct device_node *np?
> 
> I am sorry but I don't quite follow your suggestion here. Do you mean we
> could just pass the struct device_node *np in devres_add()? I thought
> the pointer passed to devress_add() should be allocated using
> devres_alloc. Can you please elaborate what you mean?

I'm just trying to reduce the diff in the patch.

> 
> > 
> > >                 devres_add(dev, ptr);
> > >         } else {
> > >                 devres_free(ptr);
[..]
> > 
> > > +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,
> > 
> > I'm wondering if we can somehow auto-detect this in
> > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
> > If it isn't there, then we go to the parent node and look for a
> > #clock-cells property there in the DT node for that device. Does that
> > make sense? Then there isn't any new API and we can attach the lifetime
> > of the devm registration to the presence of the property indicating this
> > is a clk controller or not.
> 
> Huh. I don't know why but building this kind of logic in core is a bit
> scary to me. I guess I can try implementing something like this - but I
> am not really a fan of this. (Accidentally) omit the #clock-cells from
> node and we go to parent node - I am a novice on this area but this
> sounds like a potential hazard to me. I believe the driver should know
> if it's properties should be in own or parent node - and if they are
> not, then there should be no guessing but error. The lifetime is topic
> where I would like to get information from you who know the kernel
> better than I do =) But I guess the parent node is there at least as
> long as the child device is alive. So for me the life time of
> get-callback is more crucial - but as I said, I don't understand the
> kernel in details so you probably know it better than me. But please let
> me know your final take on this and I will follow the guidance =)

Please do the magic instead of adding another API. It makes things
simpler and will work for this case without having to change anything
besides of_clk_add_provider().

If the DT doesn't have the #clock-cells property in the node being
registered then calling clk_get() will fail for any consumer devices
that point to the node with a phandle and clock specifier. I don't
expect us to get very far into development if that's the case.

Of course, we don't fail in of_clk_add_provider() if there isn't a
#clock-cells property in the node, we just happily add the node to the
provider list and carry on. I doubt anyone is failing to specify the DT
property, but maybe they are, in which case we could keep not failing
and just add the node of whatever we're called with originally if
neither the parent or the passed node have the #clock-cells property. I
wouldn't try to go any higher than one node above the current node and
look for a #clock-cells though.

If this all still seems scary then don't worry about it, I'll implement
it myself.

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

[Matti Vaittinen]

Hello Again Stephen,

I did already send v5 prior to your reply but I will create v6 today
based on this discussion.

On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-11-30 02:50:22)
> > Hello Stephen,
> > 
> > Thanks a bunch for taking the time and reviewing this!
> > 
> > On Fri, Nov 30, 2018 at 12:54:10AM -0800, Stephen Boyd wrote:
> > > Quoting Matti Vaittinen (2018-11-13 03:55:58)
> > > > -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),
> > > > @@ -3906,10 +3906,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;
> > > 
> > > Why is this moved outside of the if condition?
> > I completely removed the local variable np and just unconditionally set
> > the allocated devres to point at the node (if allocation succeeded). We
> > could of course only do this if the provider registration succeeded and
> > save one assignment - but I guess I intended to remove the curly braces
> > and thus decided to go for one liner after if. But apparently I didn't
> > remove the braces O_o. Well, I can put the assignment inside the
> > condition if you prefer that.
> > 
> > > In fact, why isn't just
> > > the first line in this hunk deleted and passed to this function as
> > > struct device_node *np?
> > 
> > I am sorry but I don't quite follow your suggestion here. Do you mean we
> > could just pass the struct device_node *np in devres_add()? I thought
> > the pointer passed to devress_add() should be allocated using
> > devres_alloc. Can you please elaborate what you mean?
> 
> I'm just trying to reduce the diff in the patch.

Oh, right. I will see how renaming the argument to np would impact to
patch size. iActually, I never consider the patch size at all - I have
only been concentrating on how the resulting file looks like. It didn't
ever cross my mind that patch size matters. But I guess the size of
chanes is really meaningfull when the amount of changes is large.

> > > >                 devres_add(dev, ptr);
> > > >         } else {
> > > >                 devres_free(ptr);
> [..]
> > > 
> > > > +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,
> > > 
> > > I'm wondering if we can somehow auto-detect this in
> > > devm_of_clk_add_hw_provider() by looking for #clock-cells in the node.
> > > If it isn't there, then we go to the parent node and look for a
> > > #clock-cells property there in the DT node for that device. Does that
> > > make sense? Then there isn't any new API and we can attach the lifetime
> > > of the devm registration to the presence of the property indicating this
> > > is a clk controller or not.
> > 
> > Huh. I don't know why but building this kind of logic in core is a bit
> > scary to me. I guess I can try implementing something like this - but I
> > am not really a fan of this. (Accidentally) omit the #clock-cells from
> > node and we go to parent node - I am a novice on this area but this
> > sounds like a potential hazard to me. I believe the driver should know
> > if it's properties should be in own or parent node - and if they are
> > not, then there should be no guessing but error. The lifetime is topic
> > where I would like to get information from you who know the kernel
> > better than I do =) But I guess the parent node is there at least as
> > long as the child device is alive. So for me the life time of
> > get-callback is more crucial - but as I said, I don't understand the
> > kernel in details so you probably know it better than me. But please let
> > me know your final take on this and I will follow the guidance =)
> 
> Please do the magic instead of adding another API. It makes things
> simpler and will work for this case without having to change anything
> besides of_clk_add_provider().

All right. Let's go on this direction then.

> If the DT doesn't have the #clock-cells property in the node being
> registered then calling clk_get() will fail for any consumer devices
> that point to the node with a phandle and clock specifier. I don't
> expect us to get very far into development if that's the case.

Makes sense. So only potential thing to break is if someone out there
has broken DT/driver - where they currently see this failure. Eg. they
use node w/o #clock-cells as provider and where they try and fail
controlling this clock - but ignore the error (and system just "works"
with HW defaults). After this change they may actually succeed in
controlling - but do control wrong clock.

Not likely scenario (sure happens somewhere) - and it involves already
broken design. So I agree with you. Besides, you are the maintainer for
clk framework and thus get the most of the rain if **** hits the fan =D

> Of course, we don't fail in of_clk_add_provider() if there isn't a
> #clock-cells property in the node, we just happily add the node to the
> provider list and carry on. I doubt anyone is failing to specify the DT
> property, but maybe they are, in which case we could keep not failing
> and just add the node of whatever we're called with originally if
> neither the parent or the passed node have the #clock-cells property. I
> wouldn't try to go any higher than one node above the current node and
> look for a #clock-cells though.

I think we should use parent device's node, not the paren node in DT,
right? But I agree, we should only look "one level up in the chain".

> 
> If this all still seems scary then don't worry about it, I'll implement
> it myself.

It still is somewhat "scary" - but I really would like to use the devm
based provider registration in the bd718x7 driver so I will implement it
in this series. The engineer version of the "living on the edge", you
know =)

Br,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-03 23:13:15)
> On Mon, Dec 03, 2018 at 03:35:10PM -0800, Stephen Boyd wrote:
> 
> > If the DT doesn't have the #clock-cells property in the node being
> > registered then calling clk_get() will fail for any consumer devices
> > that point to the node with a phandle and clock specifier. I don't
> > expect us to get very far into development if that's the case.
> 
> Makes sense. So only potential thing to break is if someone out there
> has broken DT/driver - where they currently see this failure. Eg. they
> use node w/o #clock-cells as provider and where they try and fail
> controlling this clock - but ignore the error (and system just "works"
> with HW defaults). After this change they may actually succeed in
> controlling - but do control wrong clock.
> 
> Not likely scenario (sure happens somewhere) - and it involves already
> broken design. So I agree with you. Besides, you are the maintainer for
> clk framework and thus get the most of the rain if **** hits the fan =D

Yes, exactly.

> 
> > Of course, we don't fail in of_clk_add_provider() if there isn't a
> > #clock-cells property in the node, we just happily add the node to the
> > provider list and carry on. I doubt anyone is failing to specify the DT
> > property, but maybe they are, in which case we could keep not failing
> > and just add the node of whatever we're called with originally if
> > neither the parent or the passed node have the #clock-cells property. I
> > wouldn't try to go any higher than one node above the current node and
> > look for a #clock-cells though.
> 
> I think we should use parent device's node, not the paren node in DT,
> right? But I agree, we should only look "one level up in the chain".

Are these two things different? I'm suggesting looking at
device_node::parent and trying to find a #clock-cells property.

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

[Vaittinen, Matti]

Hello Stephen,
.
> > I think we should use parent device's node, not the paren node in DT,
> > right? But I agree, we should only look "one level up in the chain".

> Are these two things different? I'm suggesting looking at
> device_node::parent and trying to find a #clock-cells property.

I thought that MFD sub-devices may completely lack the DT node but I will verify this tomorrow. BTW, I already did the patch version 6 which checks the dev->parent->of_node if #clock-cells is not found from device's own node or if own node is NULL.

Br. Matti Vaittinen 

[PATCH v4 2/8] clk: clk-max77686: Clean clkdev lookup leak and use devm

[Matti Vaittinen]

clk-max77686 never clean clkdev lookup at remove. This can cause
oops if clk-max77686 is removed and inserted again. Fix leak by
using new devm clkdev lookup registration. Simplify also error
path by using new devm_of_clk_add_parent_hw_provider.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>
---
 drivers/clk/clk-max77686.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c
index 22c937644c93..e65925d5e412 100644
--- a/drivers/clk/clk-max77686.c
+++ b/drivers/clk/clk-max77686.c
@@ -235,8 +235,9 @@ static int max77686_clk_probe(struct platform_device *pdev)
 			return ret;
 		}
 
-		ret = clk_hw_register_clkdev(&max_clk_data->hw,
-					     max_clk_data->clk_idata.name, NULL);
+		ret = devm_clk_hw_register_clkdev(dev, &max_clk_data->hw,
+						  max_clk_data->clk_idata.name,
+						  NULL);
 		if (ret < 0) {
 			dev_err(dev, "Failed to clkdev register: %d\n", ret);
 			return ret;
@@ -244,8 +245,9 @@ static int max77686_clk_probe(struct platform_device *pdev)
 	}
 
 	if (parent->of_node) {
-		ret = of_clk_add_hw_provider(parent->of_node, of_clk_max77686_get,
-					     drv_data);
+		ret = devm_of_clk_add_parent_hw_provider(dev,
+							 of_clk_max77686_get,
+							 drv_data);
 
 		if (ret < 0) {
 			dev_err(dev, "Failed to register OF clock provider: %d\n",
@@ -261,27 +263,11 @@ static int max77686_clk_probe(struct platform_device *pdev)
 					 1 << MAX77802_CLOCK_LOW_JITTER_SHIFT);
 		if (ret < 0) {
 			dev_err(dev, "Failed to config low-jitter: %d\n", ret);
-			goto remove_of_clk_provider;
+			return ret;
 		}
 	}
 
 	return 0;
-
-remove_of_clk_provider:
-	if (parent->of_node)
-		of_clk_del_provider(parent->of_node);
-
-	return ret;
-}
-
-static int max77686_clk_remove(struct platform_device *pdev)
-{
-	struct device *parent = pdev->dev.parent;
-
-	if (parent->of_node)
-		of_clk_del_provider(parent->of_node);
-
-	return 0;
 }
 
 static const struct platform_device_id max77686_clk_id[] = {
@@ -297,7 +283,6 @@ static struct platform_driver max77686_clk_driver = {
 		.name  = "max77686-clk",
 	},
 	.probe = max77686_clk_probe,
-	.remove = max77686_clk_remove,
 	.id_table = max77686_clk_id,
 };
 
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

[PATCH v4 3/8] clk: clk-st: avoid clkdev lookup leak at remove

[Matti Vaittinen]

Use devm based clkdev lookup registration to avoid leaking lookup
structures.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/x86/clk-st.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/x86/clk-st.c b/drivers/clk/x86/clk-st.c
index 3a0996f2d556..25d4b97aff9b 100644
--- a/drivers/clk/x86/clk-st.c
+++ b/drivers/clk/x86/clk-st.c
@@ -52,7 +52,8 @@ static int st_clk_probe(struct platform_device *pdev)
 		0, st_data->base + MISCCLKCNTL1, OSCCLKENB,
 		CLK_GATE_SET_TO_DISABLE, NULL);
 
-	clk_hw_register_clkdev(hws[ST_CLK_GATE], "oscout1", NULL);
+	devm_clk_hw_register_clkdev(&pdev->dev, hws[ST_CLK_GATE], "oscout1",
+				    NULL);
 
 	return 0;
 }
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

[PATCH v4 4/8] clk: clk-hi655x: Free of_provider at remove

[Matti Vaittinen]

use devm variant for of_provider registration so provider is freed
at exit.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/clk-hi655x.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-hi655x.c b/drivers/clk/clk-hi655x.c
index 403a0188634a..394d0109104d 100644
--- a/drivers/clk/clk-hi655x.c
+++ b/drivers/clk/clk-hi655x.c
@@ -107,8 +107,8 @@ static int hi655x_clk_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
-				     &hi655x_clk->clk_hw);
+	return devm_of_clk_add_parent_hw_provider(&pdev->dev,
+				of_clk_hw_simple_get, &hi655x_clk->clk_hw);
 }
 
 static struct platform_driver hi655x_clk_driver = {
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

[PATCH v4 5/8] clk: rk808: use managed version of of_provider registration

[Matti Vaittinen]

Simplify clean-up for rk808 by using managed version of of_provider
registration.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/clk-rk808.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/clk-rk808.c b/drivers/clk/clk-rk808.c
index 6461f2820a5b..177340edaae5 100644
--- a/drivers/clk/clk-rk808.c
+++ b/drivers/clk/clk-rk808.c
@@ -138,23 +138,12 @@ static int rk808_clkout_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	return of_clk_add_hw_provider(node, of_clk_rk808_get, rk808_clkout);
-}
-
-static int rk808_clkout_remove(struct platform_device *pdev)
-{
-	struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
-	struct i2c_client *client = rk808->i2c;
-	struct device_node *node = client->dev.of_node;
-
-	of_clk_del_provider(node);
-
-	return 0;
+	return devm_of_clk_add_parent_hw_provider(&pdev->dev,
+					of_clk_rk808_get, rk808_clkout);
 }
 
 static struct platform_driver rk808_clkout_driver = {
 	.probe = rk808_clkout_probe,
-	.remove = rk808_clkout_remove,
 	.driver		= {
 		.name	= "rk808-clkout",
 	},
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

[PATCH v4 6/8] clk: clk-twl6040: Free of_provider at remove

[Matti Vaittinen]

use devm variant for of_provider registration so provider is freed
at exit.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 drivers/clk/clk-twl6040.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-twl6040.c b/drivers/clk/clk-twl6040.c
index 25dfe050ae9f..e9da09453eb2 100644
--- a/drivers/clk/clk-twl6040.c
+++ b/drivers/clk/clk-twl6040.c
@@ -108,9 +108,8 @@ static int twl6040_pdmclk_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, clkdata);
 
-	return of_clk_add_hw_provider(pdev->dev.parent->of_node,
-				      of_clk_hw_simple_get,
-				      &clkdata->pdmclk_hw);
+	return devm_of_clk_add_parent_hw_provider(&pdev->dev,
+			of_clk_hw_simple_get, &clkdata->pdmclk_hw);
 }
 
 static struct platform_driver twl6040_pdmclk_driver = {
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

[PATCH v4 7/8] clk: apcs-msm8916: simplify probe cleanup by using devm

[Matti Vaittinen]

use devm variant for of_provider registration.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/clk/qcom/apcs-msm8916.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c
index b1cc8dbcd327..f4e0c136ab1a 100644
--- a/drivers/clk/qcom/apcs-msm8916.c
+++ b/drivers/clk/qcom/apcs-msm8916.c
@@ -96,8 +96,8 @@ static int qcom_apcs_msm8916_clk_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	ret = of_clk_add_hw_provider(parent->of_node, of_clk_hw_simple_get,
-				     &a53cc->clkr.hw);
+	ret = devm_of_clk_add_parent_hw_provider(dev, of_clk_hw_simple_get,
+						 &a53cc->clkr.hw);
 	if (ret) {
 		dev_err(dev, "failed to add clock provider: %d\n", ret);
 		goto err;
@@ -118,7 +118,6 @@ static int qcom_apcs_msm8916_clk_remove(struct platform_device *pdev)
 	struct device *parent = pdev->dev.parent;
 
 	clk_notifier_unregister(a53cc->pclk, &a53cc->clk_nb);
-	of_clk_del_provider(parent->of_node);
 
 	return 0;
 }
-- 
2.14.3


-- 
Matti Vaittinen
ROHM Semiconductors

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

[PATCH v4 8/8] 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.

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

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index 81cdb4eaca07..2dc12bf75b1b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -283,6 +283,13 @@ config COMMON_CLK_STM32H7
 	---help---
 	  Support for stm32h7 SoC family clocks
 
+config COMMON_CLK_BD718XX
+	tristate "Clock driver for ROHM BD718x7 PMIC"
+	depends on MFD_ROHM_BD718XX
+	help
+	  This driver supports ROHM BD71837 and ROHM BD71847
+	  PMICs clock gates.
+
 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 72be7a38cff1..a47430b387db 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


-- 
Matti Vaittinen
ROHM Semiconductors

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