[PATCH v6 00/10] clk: clkdev/of_clk - add managed lookup and provider registrations

[PATCH v6 00/10] clk: clkdev/of_clk - add managed 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 10 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
the patch version (4) combined bd718x7 driver back to this series.

Changelog (for this series) v6
- Drop 'devm_of_clk_add_parent_hw_provider'. Change
  'devm_of_clk_add_hw_provider' to look the parent device node for
  provider information if device's own node does not contain
  #clock-cells - property.
- Add kerneldoc in own patch.
- Remove NULL checks from devres match function for clkdev releasing
- Clean styling issues from clkdev.c

Changelog (for this series) v5
- Split v4 patch 1. Place clkdev stuff to patch 1 and clk provider to patch 2
- Remove devm_clk_release_clkdev from devres.txt
- Added kerneldoc for managed provider registrations.
- Cleaned styling issues.

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 (10):
  clkdev: add managed clkdev lookup registration
  clk: Add kerneldoc to managed of-provider interfaces
  clk: of-provider: look at parent if registered device has no provider
    info
  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 |   1 +
 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            |  28 ++------
 drivers/clk/clk-rk808.c               |  15 +---
 drivers/clk/clk-twl6040.c             |   5 +-
 drivers/clk/clk.c                     |  38 +++++++++-
 drivers/clk/clkdev.c                  | 111 ++++++++++++++++++++++------
 drivers/clk/qcom/apcs-msm8916.c       |   5 +-
 drivers/clk/x86/clk-st.c              |   3 +-
 include/linux/clkdev.h                |   4 ++
 13 files changed, 284 insertions(+), 69 deletions(-)
 create mode 100644 drivers/clk/clk-bd718x7.c

-- 
2.14.3

[PATCH v6 01/10] clkdev: add managed clkdev lookup registration

[Matti Vaittinen]

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 |   1 +
 drivers/clk/clkdev.c                  | 111 +++++++++++++++++++++++++++-------
 include/linux/clkdev.h                |   4 ++
 3 files changed, 93 insertions(+), 23 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 43681ca0837f..dbf14326243b 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -238,6 +238,7 @@ CLOCK
   devm_clk_put()
   devm_clk_hw_register()
   devm_of_clk_add_hw_provider()
+  devm_clk_hw_register_clkdev()
 
 DMA
   dmaenginem_async_device_register()
diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 9ab3db8b3988..4621f8a91fc0 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -401,6 +401,23 @@ 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
@@ -423,17 +440,8 @@ int clk_register_clkdev(struct clk *clk, const char *con_id,
 	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;
+	return do_clk_register_clkdev(__clk_get_hw(clk), &cl, con_id,
+					      dev_id);
 }
 EXPORT_SYMBOL(clk_register_clkdev);
 
@@ -456,18 +464,75 @@ 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;
+
+	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/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 v6 02/10] clk: Add kerneldoc to managed of-provider interfaces

[Matti Vaittinen]

Document the devm_of_clk_del_provider and the
devm_of_clk_add_hw_provider functions.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index af011974d4ec..78c90913f987 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3893,6 +3893,17 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
 	of_clk_del_provider(*(struct device_node **)res);
 }
 
+/**
+ * devm_of_clk_add_hw_provider() - Managed clk provider node registration
+ * @dev: Device acting as the clock provider. Used for DT node and lifetime.
+ * @get: callback for decoding clk_hw
+ * @data: context pointer for @get callback.
+ *
+ * Returns 0 on success or an errno on failure.
+ *
+ * Registers clock provider for given device's node. Provider is automatically
+ * released at device exit.
+ */
 int devm_of_clk_add_hw_provider(struct device *dev,
 			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
 					      void *data),
@@ -3950,6 +3961,10 @@ static int devm_clk_provider_match(struct device *dev, void *res, void *data)
 	return *np == data;
 }
 
+/**
+ * devm_of_clk_del_provider() - Remove clock provider registered using devm
+ * @dev: Device to whose lifetime the clock provider was bound
+ */
 void devm_of_clk_del_provider(struct device *dev)
 {
 	int ret;
-- 
2.14.3

Re: [PATCH v6 02/10] clk: Add kerneldoc to managed of-provider interfaces

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:33:48)
> Document the devm_of_clk_del_provider and the
> devm_of_clk_add_hw_provider functions.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---

Applied to clk-next

[PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Matti Vaittinen]

It seems to be usual for MFD devices that the created 'clock sub-device'
do not have own DT node. The clock provider information is usually in the
main device node which is owned by the MFD device. Change the devm variant
of clk of-provider registration to check the parent device node if given
device has no own node or if the node does not contain the #clock-cells
property. In such case use the parent node if it contains the #clock-cells.

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

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 78c90913f987..66dc7c1483d7 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3893,6 +3893,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
 	of_clk_del_provider(*(struct device_node **)res);
 }
 
+static int of_is_clk_provider(struct device_node *np)
+{
+	return !!of_find_property(np, "#clock-cells", NULL);
+}
+
 /**
  * devm_of_clk_add_hw_provider() - Managed clk provider node registration
  * @dev: Device acting as the clock provider. Used for DT node and lifetime.
@@ -3901,8 +3906,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
  *
  * Returns 0 on success or an errno on failure.
  *
- * Registers clock provider for given device's node. Provider is automatically
- * released at device exit.
+ * Registers clock provider for given device's node. If the device has no DT
+ * node or if the device node lacks of clock provider information (#clock-cells)
+ * then the parent device's node is scanned for this information. If parent node
+ * has the #clock-cells then it is used in registration. Provider is
+ * automatically released at device exit.
  */
 int devm_of_clk_add_hw_provider(struct device *dev,
 			struct clk_hw *(*get)(struct of_phandle_args *clkspec,
@@ -3912,12 +3920,17 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 	struct device_node **ptr, *np;
 	int ret;
 
+	np = dev->of_node;
+
+	if (!of_is_clk_provider(dev->of_node))
+		if (of_is_clk_provider(dev->parent->of_node))
+			np = dev->parent->of_node;
+
 	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
 			   GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
 
-	np = dev->of_node;
 	ret = of_clk_add_hw_provider(np, get, data);
 	if (!ret) {
 		*ptr = np;
@@ -3968,9 +3981,15 @@ static int devm_clk_provider_match(struct device *dev, void *res, void *data)
 void devm_of_clk_del_provider(struct device *dev)
 {
 	int ret;
+	struct device_node *np;
+
+	np = dev->of_node;
 
+	if (!of_is_clk_provider(dev->of_node))
+		if (of_is_clk_provider(dev->parent->of_node))
+			np = dev->parent->of_node;
 	ret = devres_release(dev, devm_of_clk_release_provider,
-			     devm_clk_provider_match, dev->of_node);
+			     devm_clk_provider_match, np);
 
 	WARN_ON(ret);
 }
-- 
2.14.3

Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:34:53)
> It seems to be usual for MFD devices that the created 'clock sub-device'
> do not have own DT node. The clock provider information is usually in the
> main device node which is owned by the MFD device. Change the devm variant
> of clk of-provider registration to check the parent device node if given
> device has no own node or if the node does not contain the #clock-cells
> property. In such case use the parent node if it contains the #clock-cells.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
>  drivers/clk/clk.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 78c90913f987..66dc7c1483d7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3893,6 +3893,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
>         of_clk_del_provider(*(struct device_node **)res);
>  }
>  
> +static int of_is_clk_provider(struct device_node *np)
> +{
> +       return !!of_find_property(np, "#clock-cells", NULL);
> +}
> +
>  /**
>   * devm_of_clk_add_hw_provider() - Managed clk provider node registration
>   * @dev: Device acting as the clock provider. Used for DT node and lifetime.
> @@ -3901,8 +3906,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
>   *
>   * Returns 0 on success or an errno on failure.
>   *
> - * Registers clock provider for given device's node. Provider is automatically
> - * released at device exit.
> + * Registers clock provider for given device's node. If the device has no DT
> + * node or if the device node lacks of clock provider information (#clock-cells)
> + * then the parent device's node is scanned for this information. If parent node
> + * has the #clock-cells then it is used in registration. Provider is
> + * automatically released at device exit.
>   */
>  int devm_of_clk_add_hw_provider(struct device *dev,
>                         struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> @@ -3912,12 +3920,17 @@ int devm_of_clk_add_hw_provider(struct device *dev,
>         struct device_node **ptr, *np;
>         int ret;
>  
> +       np = dev->of_node;
> +
> +       if (!of_is_clk_provider(dev->of_node))
> +               if (of_is_clk_provider(dev->parent->of_node))
> +                       np = dev->parent->of_node;

As said on v5, let's just modify of_clk_add_provider() to do the parent
search.

Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Matti Vaittinen]

Hello Stephen,

I copied some parts of the v4 discussion here as well. Let's continue
them under this one email thread. (and yep, this is my bad we now have
multiple email threads - I did these new patches without waiting for
the final conclusion. I should try to be more patient in the future...)

> > > 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.

So yep. It appears that the DT node for MFD sub-device is left NULL.
This is quite logical as there really is no clk sub-node in MFD (pmic)
node. The option to "hack around" this would be setting the of_node to
parent node in driver code. But this feels wrong. Drivers should not
mess with the "dt node ownership" - and it also feels a bit odd when
many devices use same DT node. I think we may hit in problems when
obtaining resources or doing reference counting. Hence I think we should
keep the of_node NULL for sub-device if the sub-device does not have own
node inside the main devie node. And I think Rob was not a fan of having
own nodes for sub-devices inside the MFD node (AFAIR my first driver
draft for this device had it but Rob and you thought that was not correct).

On Tue, Dec 04, 2018 at 11:38:17AM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-12-04 03:34:53)
> > It seems to be usual for MFD devices that the created 'clock sub-device'
> > do not have own DT node. The clock provider information is usually in the
> > main device node which is owned by the MFD device. Change the devm variant
> > of clk of-provider registration to check the parent device node if given
> > device has no own node or if the node does not contain the #clock-cells
> > property. In such case use the parent node if it contains the #clock-cells.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> >  drivers/clk/clk.c | 27 +++++++++++++++++++++++----
> >  1 file changed, 23 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 78c90913f987..66dc7c1483d7 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3893,6 +3893,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >         of_clk_del_provider(*(struct device_node **)res);
> >  }
> >  
> > +static int of_is_clk_provider(struct device_node *np)
> > +{
> > +       return !!of_find_property(np, "#clock-cells", NULL);
> > +}
> > +
> >  /**
> >   * devm_of_clk_add_hw_provider() - Managed clk provider node registration
> >   * @dev: Device acting as the clock provider. Used for DT node and lifetime.
> > @@ -3901,8 +3906,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >   *
> >   * Returns 0 on success or an errno on failure.
> >   *
> > - * Registers clock provider for given device's node. Provider is automatically
> > - * released at device exit.
> > + * Registers clock provider for given device's node. If the device has no DT
> > + * node or if the device node lacks of clock provider information (#clock-cells)
> > + * then the parent device's node is scanned for this information. If parent node
> > + * has the #clock-cells then it is used in registration. Provider is
> > + * automatically released at device exit.
> >   */
> >  int devm_of_clk_add_hw_provider(struct device *dev,
> >                         struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> > @@ -3912,12 +3920,17 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >         struct device_node **ptr, *np;
> >         int ret;
> >  
> > +       np = dev->of_node;
> > +
> > +       if (!of_is_clk_provider(dev->of_node))
> > +               if (of_is_clk_provider(dev->parent->of_node))
> > +                       np = dev->parent->of_node;
> 
> As said on v5, let's just modify of_clk_add_provider() to do the parent
> search.

But that won't solve the issue if we don't do "dirty hacks" in driver.
The devm interface still only gets the device-pointer, not the DT node
as argument. And if DT node for device is NULL (like in MFD cases) -
then there is no parent node, only parent device with a node. For plain
of_clk_add_provider() the driver can just give the parent's node pointer
in cases where it knows it is the parent who has the provider data in
DT. But our original problem is in devm interfaces.

Br,
	Matti Vaittinen

-- 
Matti Vaittinen
ROHM Semiconductors

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

Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 23:00:46)
> Hello Stephen,
> 
> I copied some parts of the v4 discussion here as well. Let's continue
> them under this one email thread. (and yep, this is my bad we now have
> multiple email threads - I did these new patches without waiting for
> the final conclusion. I should try to be more patient in the future...)
> 
> > > > 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.
> 
> So yep. It appears that the DT node for MFD sub-device is left NULL.
> This is quite logical as there really is no clk sub-node in MFD (pmic)
> node. The option to "hack around" this would be setting the of_node to
> parent node in driver code. But this feels wrong. Drivers should not
> mess with the "dt node ownership" - and it also feels a bit odd when
> many devices use same DT node. I think we may hit in problems when
> obtaining resources or doing reference counting. Hence I think we should
> keep the of_node NULL for sub-device if the sub-device does not have own
> node inside the main devie node. And I think Rob was not a fan of having
> own nodes for sub-devices inside the MFD node (AFAIR my first driver
> draft for this device had it but Rob and you thought that was not correct).

Yes let's not change this.

> > > @@ -3901,8 +3906,11 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> > >   *
> > >   * Returns 0 on success or an errno on failure.
> > >   *
> > > - * Registers clock provider for given device's node. Provider is automatically
> > > - * released at device exit.
> > > + * Registers clock provider for given device's node. If the device has no DT
> > > + * node or if the device node lacks of clock provider information (#clock-cells)
> > > + * then the parent device's node is scanned for this information. If parent node
> > > + * has the #clock-cells then it is used in registration. Provider is
> > > + * automatically released at device exit.
> > >   */
> > >  int devm_of_clk_add_hw_provider(struct device *dev,
> > >                         struct clk_hw *(*get)(struct of_phandle_args *clkspec,
> > > @@ -3912,12 +3920,17 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> > >         struct device_node **ptr, *np;
> > >         int ret;
> > >  
> > > +       np = dev->of_node;
> > > +
> > > +       if (!of_is_clk_provider(dev->of_node))
> > > +               if (of_is_clk_provider(dev->parent->of_node))
> > > +                       np = dev->parent->of_node;
> > 
> > As said on v5, let's just modify of_clk_add_provider() to do the parent
> > search.
> 
> But that won't solve the issue if we don't do "dirty hacks" in driver.
> The devm interface still only gets the device-pointer, not the DT node
> as argument. And if DT node for device is NULL (like in MFD cases) -
> then there is no parent node, only parent device with a node. For plain
> of_clk_add_provider() the driver can just give the parent's node pointer
> in cases where it knows it is the parent who has the provider data in
> DT. But our original problem is in devm interfaces.
> 

I was misunderstanding the MFD design. Should still work though, so I
squashed this into the patch to clean things up a bit. Does this work
for you?

------8<-----

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index bb689161f0f5..6ff852bda892 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -3893,9 +3893,23 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
 	of_clk_del_provider(*(struct device_node **)res);
 }
 
-static int of_is_clk_provider(struct device_node *np)
+/*
+ * We allow a child device to use its parent device as the clock provider node
+ * for cases like MFD sub-devices where the child device driver wants to use
+ * devm_*() APIs but not list the device in DT as a sub-node.
+ */
+static struct device_node *get_clk_provider_node(struct device *dev)
 {
-	return !!of_find_property(np, "#clock-cells", NULL);
+	struct device_node *np, *parent_np;
+
+	np = dev->of_node;
+	parent_np = dev->parent ? dev->parent->of_node : NULL;
+
+	if (!of_find_property(np, "#clock-cells", NULL))
+		if (of_find_property(parent_np, "#clock-cells", NULL))
+			np = parent_np;
+
+	return np;
 }
 
 /**
@@ -3920,17 +3934,12 @@ int devm_of_clk_add_hw_provider(struct device *dev,
 	struct device_node **ptr, *np;
 	int ret;
 
-	np = dev->of_node;
-
-	if (!of_is_clk_provider(dev->of_node))
-		if (of_is_clk_provider(dev->parent->of_node))
-			np = dev->parent->of_node;
-
 	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
 			   GFP_KERNEL);
 	if (!ptr)
 		return -ENOMEM;
 
+	np = get_clk_provider_node(dev);
 	ret = of_clk_add_hw_provider(np, get, data);
 	if (!ret) {
 		*ptr = np;
@@ -3981,13 +3990,8 @@ static int devm_clk_provider_match(struct device *dev, void *res, void *data)
 void devm_of_clk_del_provider(struct device *dev)
 {
 	int ret;
-	struct device_node *np;
-
-	np = dev->of_node;
+	struct device_node *np = get_clk_provider_node(dev);
 
-	if (!of_is_clk_provider(dev->of_node))
-		if (of_is_clk_provider(dev->parent->of_node))
-			np = dev->parent->of_node;
 	ret = devres_release(dev, devm_of_clk_release_provider,
 			     devm_clk_provider_match, np);
 

Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Matti Vaittinen]

On Wed, Dec 05, 2018 at 09:19:33AM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-12-04 23:00:46)
> > But that won't solve the issue if we don't do "dirty hacks" in driver.
> > The devm interface still only gets the device-pointer, not the DT node
> > as argument. And if DT node for device is NULL (like in MFD cases) -
> > then there is no parent node, only parent device with a node. For plain
> > of_clk_add_provider() the driver can just give the parent's node pointer
> > in cases where it knows it is the parent who has the provider data in
> > DT. But our original problem is in devm interfaces.
> > 
> 
> I was misunderstanding the MFD design. Should still work though, so I
> squashed this into the patch to clean things up a bit. Does this work
> for you?

This looks good to me. Especially changing the of_is_clk_provider to
get_clk_provider_node which allows to remove some repetition. If you
apply this then I will drop my patch from the series. Just please let me
know. I will cook version 7 of the series at Friday - tomorrow is the
independence day in Finland and I'll be offline =)

Thanks!

> 
> ------8<-----
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index bb689161f0f5..6ff852bda892 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -3893,9 +3893,23 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
>  	of_clk_del_provider(*(struct device_node **)res);
>  }
>  
> -static int of_is_clk_provider(struct device_node *np)
> +/*
> + * We allow a child device to use its parent device as the clock provider node
> + * for cases like MFD sub-devices where the child device driver wants to use
> + * devm_*() APIs but not list the device in DT as a sub-node.
> + */
> +static struct device_node *get_clk_provider_node(struct device *dev)
>  {
> -	return !!of_find_property(np, "#clock-cells", NULL);
> +	struct device_node *np, *parent_np;
> +
> +	np = dev->of_node;
> +	parent_np = dev->parent ? dev->parent->of_node : NULL;
> +
> +	if (!of_find_property(np, "#clock-cells", NULL))
> +		if (of_find_property(parent_np, "#clock-cells", NULL))
> +			np = parent_np;
> +
> +	return np;
>  }
>  
>  /**
> @@ -3920,17 +3934,12 @@ int devm_of_clk_add_hw_provider(struct device *dev,
>  	struct device_node **ptr, *np;
>  	int ret;
>  
> -	np = dev->of_node;
> -
> -	if (!of_is_clk_provider(dev->of_node))
> -		if (of_is_clk_provider(dev->parent->of_node))
> -			np = dev->parent->of_node;
> -
>  	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
>  			   GFP_KERNEL);
>  	if (!ptr)
>  		return -ENOMEM;
>  
> +	np = get_clk_provider_node(dev);
>  	ret = of_clk_add_hw_provider(np, get, data);
>  	if (!ret) {
>  		*ptr = np;
> @@ -3981,13 +3990,8 @@ static int devm_clk_provider_match(struct device *dev, void *res, void *data)
>  void devm_of_clk_del_provider(struct device *dev)
>  {
>  	int ret;
> -	struct device_node *np;
> -
> -	np = dev->of_node;
> +	struct device_node *np = get_clk_provider_node(dev);
>  
> -	if (!of_is_clk_provider(dev->of_node))
> -		if (of_is_clk_provider(dev->parent->of_node))
> -			np = dev->parent->of_node;
>  	ret = devres_release(dev, devm_of_clk_release_provider,
>  			     devm_clk_provider_match, np);
>  
> 

-- 
Matti Vaittinen
ROHM Semiconductors

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

Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Matti Vaittinen]

On Wed, Dec 05, 2018 at 08:20:58PM +0200, Matti Vaittinen wrote:
> On Wed, Dec 05, 2018 at 09:19:33AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-12-04 23:00:46)
> > > But that won't solve the issue if we don't do "dirty hacks" in driver.
> > > The devm interface still only gets the device-pointer, not the DT node
> > > as argument. And if DT node for device is NULL (like in MFD cases) -
> > > then there is no parent node, only parent device with a node. For plain
> > > of_clk_add_provider() the driver can just give the parent's node pointer
> > > in cases where it knows it is the parent who has the provider data in
> > > DT. But our original problem is in devm interfaces.
> > > 
> > 
> > I was misunderstanding the MFD design. Should still work though, so I
> > squashed this into the patch to clean things up a bit. Does this work
> > for you?
> 
> This looks good to me. Especially changing the of_is_clk_provider to
> get_clk_provider_node which allows to remove some repetition. If you
> apply this then I will drop my patch from the series. Just please let me
> know. I will cook version 7 of the series at Friday - tomorrow is the
> independence day in Finland and I'll be offline =)

I see you already applied the follow-up patches to clk-next. Please note
that most of the clean-ups did require this functionality as they used
the parent DT node.

> 
> Thanks!
> 
> > 
> > ------8<-----
> > 
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index bb689161f0f5..6ff852bda892 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -3893,9 +3893,23 @@ static void devm_of_clk_release_provider(struct device *dev, void *res)
> >  	of_clk_del_provider(*(struct device_node **)res);
> >  }
> >  
> > -static int of_is_clk_provider(struct device_node *np)
> > +/*
> > + * We allow a child device to use its parent device as the clock provider node
> > + * for cases like MFD sub-devices where the child device driver wants to use
> > + * devm_*() APIs but not list the device in DT as a sub-node.
> > + */
> > +static struct device_node *get_clk_provider_node(struct device *dev)
> >  {
> > -	return !!of_find_property(np, "#clock-cells", NULL);
> > +	struct device_node *np, *parent_np;
> > +
> > +	np = dev->of_node;
> > +	parent_np = dev->parent ? dev->parent->of_node : NULL;
> > +
> > +	if (!of_find_property(np, "#clock-cells", NULL))
> > +		if (of_find_property(parent_np, "#clock-cells", NULL))
> > +			np = parent_np;
> > +
> > +	return np;
> >  }
> >  
> >  /**
> > @@ -3920,17 +3934,12 @@ int devm_of_clk_add_hw_provider(struct device *dev,
> >  	struct device_node **ptr, *np;
> >  	int ret;
> >  
> > -	np = dev->of_node;
> > -
> > -	if (!of_is_clk_provider(dev->of_node))
> > -		if (of_is_clk_provider(dev->parent->of_node))
> > -			np = dev->parent->of_node;
> > -
> >  	ptr = devres_alloc(devm_of_clk_release_provider, sizeof(*ptr),
> >  			   GFP_KERNEL);
> >  	if (!ptr)
> >  		return -ENOMEM;
> >  
> > +	np = get_clk_provider_node(dev);
> >  	ret = of_clk_add_hw_provider(np, get, data);
> >  	if (!ret) {
> >  		*ptr = np;
> > @@ -3981,13 +3990,8 @@ static int devm_clk_provider_match(struct device *dev, void *res, void *data)
> >  void devm_of_clk_del_provider(struct device *dev)
> >  {
> >  	int ret;
> > -	struct device_node *np;
> > -
> > -	np = dev->of_node;
> > +	struct device_node *np = get_clk_provider_node(dev);
> >  
> > -	if (!of_is_clk_provider(dev->of_node))
> > -		if (of_is_clk_provider(dev->parent->of_node))
> > -			np = dev->parent->of_node;
> >  	ret = devres_release(dev, devm_of_clk_release_provider,
> >  			     devm_clk_provider_match, np);
> >  
> > 
> 
> -- 
> Matti Vaittinen
> ROHM Semiconductors
> 
> ~~~ "I don't think so," said Rene Descartes.  Just then, he vanished ~~~

-- 
Matti Vaittinen
ROHM Semiconductors

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

Re: [PATCH v6 03/10] clk: of-provider: look at parent if registered device has no provider info

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-05 10:20:58)
> On Wed, Dec 05, 2018 at 09:19:33AM -0800, Stephen Boyd wrote:
> > Quoting Matti Vaittinen (2018-12-04 23:00:46)
> > > But that won't solve the issue if we don't do "dirty hacks" in driver.
> > > The devm interface still only gets the device-pointer, not the DT node
> > > as argument. And if DT node for device is NULL (like in MFD cases) -
> > > then there is no parent node, only parent device with a node. For plain
> > > of_clk_add_provider() the driver can just give the parent's node pointer
> > > in cases where it knows it is the parent who has the provider data in
> > > DT. But our original problem is in devm interfaces.
> > > 
> > 
> > I was misunderstanding the MFD design. Should still work though, so I
> > squashed this into the patch to clean things up a bit. Does this work
> > for you?
> 
> This looks good to me. Especially changing the of_is_clk_provider to
> get_clk_provider_node which allows to remove some repetition. If you
> apply this then I will drop my patch from the series. Just please let me
> know. I will cook version 7 of the series at Friday - tomorrow is the
> independence day in Finland and I'll be offline =)
> 

Ok, I applied it this patch with the squash to clk-next.

[PATCH v6 04/10] 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_hw_provider.

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

diff --git a/drivers/clk/clk-max77686.c b/drivers/clk/clk-max77686.c
index 22c937644c93..3727d5472450 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,8 @@ 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_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 +262,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 +282,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

[PATCH v6 05/10] 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

[PATCH v6 06/10] 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..a0de3315df2e 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_hw_provider(&pdev->dev, of_clk_hw_simple_get,
+					   &hi655x_clk->clk_hw);
 }
 
 static struct platform_driver hi655x_clk_driver = {
-- 
2.14.3

Re: [PATCH v6 06/10] clk: clk-hi655x: Free of_provider at remove

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:37:29)
> use devm variant for of_provider registration so provider is freed
> at exit.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---

Applied to clk-next

[PATCH v6 07/10] 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..8d90bdf5b946 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_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

Re: [PATCH v6 07/10] clk: rk808: use managed version of of_provider registration

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:38:03)
> Simplify clean-up for rk808 by using managed version of of_provider
> registration.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---

Applied to clk-next

[PATCH v6 08/10] 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..ea846f77750b 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_hw_provider(&pdev->dev, of_clk_hw_simple_get,
+					   &clkdata->pdmclk_hw);
 }
 
 static struct platform_driver twl6040_pdmclk_driver = {
-- 
2.14.3

Re: [PATCH v6 08/10] clk: clk-twl6040: Free of_provider at remove

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:38:32)
> 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>
> ---

Applied to clk-next

[PATCH v6 09/10] 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..caa9b0d53d68 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_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

Re: [PATCH v6 09/10] clk: apcs-msm8916: simplify probe cleanup by using devm

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:39:06)
> use devm variant for of_provider registration.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---

Applied to clk-next

[PATCH v6 10/10] 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       |   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..d486859526ed
--- /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_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 v6 10/10] clk: bd718x7: Initial support for ROHM bd71837/bd71847 PMIC clock

[Stephen Boyd]

Quoting Matti Vaittinen (2018-12-04 03:39:38)
> diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
> new file mode 100644
> index 000000000000..d486859526ed
> --- /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

This isn't even the name of the file. Please remove. Also, only the SPDX
tag is supposed to have // on it and the other things should be normal
/* */ comment style.

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

Do you plan to use the clkdev lookup? This driver looks fairly DT
dependent, so I would prefer to remove the clkdev part and only add it
later if anyone needs it.

> +               if (rval)
> +                       dev_warn(&pdev->dev, "Failed to register clkdev\n");
> +               if (parent->of_node) {
> +                       rval = devm_of_clk_add_hw_provider(&pdev->dev,
> +                                            of_clk_hw_simple_get, &c->hw);
> +                       if (rval)
> +                               dev_err(&pdev->dev,
> +                                       "adding clk provider failed\n");
> +               }

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

[Matti Vaittinen]

Thanks again Stephen.

On Wed, Dec 05, 2018 at 09:28:21AM -0800, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-12-04 03:39:38)
> > diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
> > new file mode 100644
> > index 000000000000..d486859526ed
> > --- /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
> 
> This isn't even the name of the file. Please remove. Also, only the SPDX
> tag is supposed to have // on it and the other things should be normal
> /* */ comment style.

I'll drop this name.

> 
> > +
> > +#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>
> [....]
> > +
> > +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);
> 
> Do you plan to use the clkdev lookup? This driver looks fairly DT
> dependent, so I would prefer to remove the clkdev part and only add it
> later if anyone needs it.

Right. We depend heavily on DT so I guess I can drop the clkdev portion.

> > +               if (rval)
> > +                       dev_warn(&pdev->dev, "Failed to register clkdev\n");
> > +               if (parent->of_node) {
> > +                       rval = devm_of_clk_add_hw_provider(&pdev->dev,
> > +                                            of_clk_hw_simple_get, &c->hw);
> > +                       if (rval)
> > +                               dev_err(&pdev->dev,
> > +                                       "adding clk provider failed\n");
> > +               }

-- 
Matti Vaittinen
ROHM Semiconductors

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

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

[Pavel Machek]

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

On Wed 2018-12-05 09:28:21, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-12-04 03:39:38)
> > diff --git a/drivers/clk/clk-bd718x7.c b/drivers/clk/clk-bd718x7.c
> > new file mode 100644
> > index 000000000000..d486859526ed
> > --- /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
> 
> This isn't even the name of the file. Please remove. Also, only the SPDX
> tag is supposed to have // on it and the other things should be normal
> /* */ comment style.

Actually whole block commented by // is now considered ok.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v6 00/10] clk: clkdev/of_clk - add managed lookup and provider registrations

[Matti Vaittinen]

On Tue, Dec 04, 2018 at 01:31:43PM +0200, Matti Vaittinen wrote:
> Series add bd71837/bd71837 PMIC clock support + managed interfaces

> Changed drivers are:
> clk-max77686, clk-st, clk-hi655x, rk808, clk-twl6040
> and apcs-msm8916. New driver is clk-bd718x7
> 
> Changelog (for this series) v6
> - Drop 'devm_of_clk_add_parent_hw_provider'. Change
>   'devm_of_clk_add_hw_provider' to look the parent device node for
>   provider information if device's own node does not contain
>   #clock-cells - property.
 
Forgot to mention the obvious - I did also convert the call to
devm_of_clk_add_parent_hw_provider to call to devm_of_clk_add_hw_provider
from all drivers where I had this new call added.

I kept Reviewed-by and Acked-by tags from Peter Ujfalusi and Krzysztof
Kozlowski as the devm_of_clk_add_hw_provider should now do same thing
as devm_of_clk_add_parent_hw_provider did. Please let me know if this
is not Ok.

Br,
	Matti Vaittinen