[PATCH] clk: clkdev - add managed versions of lookup registrations

[PATCH] clk: clkdev - add managed versions of lookup registrations

[Matti Vaittinen]

Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and
devm_clk_release_clkdev as a first styep to clean up drivers which are
leaking clkdev lookups at driver remove.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---

While searching for example on how clk drivers release clkdev at exit
I found that many of those don't. Simple grep for clkdev under
drivers/clk suggest that bunch of drivers which call clk_register_clkdev
or clk_hw_register_clkdev never call clkdev_drop. In order to help
cleaning this up I decided to add devm versions of clk_register_clkdev
and clk_hw_register_clkdev. I hope this would allow simply converting
bunch of calls to clk_register_clkdev and clk_hw_register_clkdev into
managed versions without building further clean up logic.

Please review this carefully. I have only performed some simple tests
with my BD71837 driver. And as I have only limited understanding on
clkdev lookups I may have done mistakes. Thanks!

 drivers/clk/clkdev.c   | 165 +++++++++++++++++++++++++++++++++++++++++--------
 include/linux/clkdev.h |   8 +++
 2 files changed, 147 insertions(+), 26 deletions(-)

diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
index 7513411140b6..4752a0004a6c 100644
--- a/drivers/clk/clkdev.c
+++ b/drivers/clk/clkdev.c
@@ -390,7 +390,7 @@ void clkdev_drop(struct clk_lookup *cl)
 }
 EXPORT_SYMBOL(clkdev_drop);
 
-static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
+static struct clk_lookup *do_clk_register_clkdev(struct clk_hw *hw,
 						const char *con_id,
 						const char *dev_id, ...)
 {
@@ -404,6 +404,24 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
 	return cl;
 }
 
+static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
+	const char *con_id, const char *dev_id)
+{
+	struct clk_lookup *cl;
+
+	/*
+	 * 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 = do_clk_register_clkdev(hw, con_id, "%s",
+					   dev_id);
+	else
+		cl = do_clk_register_clkdev(hw, con_id, NULL);
+
+	return cl;
+}
+
 /**
  * clk_register_clkdev - register one clock lookup for a struct clk
  * @clk: struct clk to associate with all clk_lookups
@@ -421,22 +439,18 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
 int clk_register_clkdev(struct clk *clk, const char *con_id,
 	const char *dev_id)
 {
-	struct clk_lookup *cl;
-
-	if (IS_ERR(clk))
-		return PTR_ERR(clk);
+	int rval = 0;
 
-	/*
-	 * 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);
+	if (IS_ERR(clk)) {
+		rval = PTR_ERR(clk);
+	} else {
+		struct clk_lookup *cl;
 
-	return cl ? 0 : -ENOMEM;
+		cl = __clk_register_clkdev(__clk_get_hw(clk), con_id, dev_id);
+		if (!cl)
+			rval = -ENOMEM;
+	}
+	return rval;
 }
 EXPORT_SYMBOL(clk_register_clkdev);
 
@@ -456,21 +470,120 @@ EXPORT_SYMBOL(clk_register_clkdev);
  */
 int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id,
 	const char *dev_id)
+{
+	int rval = 0;
+
+	if (IS_ERR(hw)) {
+		rval = PTR_ERR(hw);
+	} else {
+		struct clk_lookup *cl;
+
+		cl = __clk_register_clkdev(hw, con_id, dev_id);
+		if (!cl)
+			rval = -ENOMEM;
+	}
+
+	return rval;
+}
+EXPORT_SYMBOL(clk_hw_register_clkdev);
+
+static void devm_clkdev_release(struct device *dev, void *res)
+{
+	clkdev_drop(*(struct clk_lookup **)res);
+}
+
+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;
+}
+
+/**
+ * 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 or
+ * with devm_clk_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)
+{
+	struct clk_lookup **cl = NULL;
 
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
+	cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL);
+	if (cl) {
+		*cl = __clk_register_clkdev(hw, con_id, dev_id);
+		if (*cl)
+			devres_add(dev, cl);
+		else
+			devres_free(cl);
+	}
+	return (cl && *cl) ? 0 : -ENOMEM;
+}
+EXPORT_SYMBOL(devm_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);
-
-	return cl ? 0 : -ENOMEM;
+/**
+ * devm_clk_register_clkdev - managed clk lookup registration for a struct clk
+ * @dev: device this lookup is bound
+ * @clk: struct clk to associate with all clk_lookups
+ * @con_id: connection ID string on device
+ * @dev_id: 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 clks
+ * from a previous clk_register() call, and return the error code for
+ * those.  This is to permit this function to be called immediately
+ * after clk_register().
+ */
+int devm_clk_register_clkdev(struct device *dev, struct clk *clk,
+			     const char *con_id, const char *dev_id)
+{
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+	return devm_clk_hw_register_clkdev(dev, __clk_get_hw(clk), con_id,
+					   dev_id);
 }
-EXPORT_SYMBOL(clk_hw_register_clkdev);
+EXPORT_SYMBOL(devm_clk_register_clkdev);
diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
index 4890ff033220..27ebeae8ae26 100644
--- a/include/linux/clkdev.h
+++ b/include/linux/clkdev.h
@@ -52,4 +52,12 @@ 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_register_clkdev(struct device *dev, struct clk *clk,
+			     const char *con_id, const char *dev_id);
+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

Re: [PATCH] clk: clkdev - add managed versions of lookup registrations

[Stephen Boyd]

Quoting Matti Vaittinen (2018-06-28 00:54:53)
> Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and
> devm_clk_release_clkdev as a first styep to clean up drivers which are

s/styep/step/

> leaking clkdev lookups at driver remove.
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> 
>  drivers/clk/clkdev.c   | 165 +++++++++++++++++++++++++++++++++++++++++--------
>  include/linux/clkdev.h |   8 +++

Also need to update the Documentation file at
Documentation/driver-model/devres.txt

>  2 files changed, 147 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> index 7513411140b6..4752a0004a6c 100644
> --- a/drivers/clk/clkdev.c
> +++ b/drivers/clk/clkdev.c
> @@ -390,7 +390,7 @@ void clkdev_drop(struct clk_lookup *cl)
>  }
>  EXPORT_SYMBOL(clkdev_drop);
>  
> -static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
> +static struct clk_lookup *do_clk_register_clkdev(struct clk_hw *hw,

Don't rename this.

>                                                 const char *con_id,
>                                                 const char *dev_id, ...)
>  {
> @@ -404,6 +404,24 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
>         return cl;
>  }
>  
> +static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
> +       const char *con_id, const char *dev_id)
> +{
> +       struct clk_lookup *cl;
> +
> +       /*
> +        * 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 = do_clk_register_clkdev(hw, con_id, "%s",
> +                                          dev_id);
> +       else
> +               cl = do_clk_register_clkdev(hw, con_id, NULL);
> +
> +       return cl;

I think this is the same code as before? Try to minimize the diff so we
can focus on what's really changing.

> +}
> +
>  /**
>   * clk_register_clkdev - register one clock lookup for a struct clk
>   * @clk: struct clk to associate with all clk_lookups
[...]
> +
> +/**
> + * 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)
> +{
> +       struct clk_lookup **cl = NULL;

Don't assign to NULL to just overwrite it later.

>  
>         if (IS_ERR(hw))
>                 return PTR_ERR(hw);

Put another newline here.

> +       cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL);
> +       if (cl) {
> +               *cl = __clk_register_clkdev(hw, con_id, dev_id);

Why can't we just call clk_hw_register_clkdev()? Sure the IS_ERR()
chain is duplicated, but that can be left out of the devm version and
rely on the clk_hw_register_clkdev() to take care of it otherwise.


> +               if (*cl)
> +                       devres_add(dev, cl);
> +               else
> +                       devres_free(cl);
> +       }
> +       return (cl && *cl) ? 0 : -ENOMEM;
> +}
> +EXPORT_SYMBOL(devm_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);
> -
> -       return cl ? 0 : -ENOMEM;
> +/**
> + * devm_clk_register_clkdev - managed clk lookup registration for a struct clk
> + * @dev: device this lookup is bound
> + * @clk: struct clk to associate with all clk_lookups
> + * @con_id: connection ID string on device
> + * @dev_id: 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 clks
> + * from a previous clk_register() call, and return the error code for
> + * those.  This is to permit this function to be called immediately
> + * after clk_register().
> + */
> +int devm_clk_register_clkdev(struct device *dev, struct clk *clk,
> +                            const char *con_id, const char *dev_id)

I wouldn't even add this function, to encourage driver writers to
migrate to clk_hw based registration functions and to avoid removing it
later on.

> +{
> +       if (IS_ERR(clk))
> +               return PTR_ERR(clk);
> +       return devm_clk_hw_register_clkdev(dev, __clk_get_hw(clk), con_id,
> +                                          dev_id);
>  }
> -EXPORT_SYMBOL(clk_hw_register_clkdev);
> +EXPORT_SYMBOL(devm_clk_register_clkdev);
> diff --git a/include/linux/clkdev.h b/include/linux/clkdev.h
> index 4890ff033220..27ebeae8ae26 100644
> --- a/include/linux/clkdev.h
> +++ b/include/linux/clkdev.h
> @@ -52,4 +52,12 @@ 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_register_clkdev(struct device *dev, struct clk *clk,
> +                            const char *con_id, const char *dev_id);
> +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

Re: [PATCH] clk: clkdev - add managed versions of lookup registrations

[Matti Vaittinen]

Hello all,

Sorry for html email. I'm not used to send mails using gmail app.
Trying to resend as plain text now...

On 7/17/18, Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> ma 9. heinäk. 2018 klo 9.33 Stephen Boyd <sboyd@kernel.org> kirjoitti:
>
>> Quoting Matti Vaittinen (2018-06-28 00:54:53)
>> > Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and
>> > devm_clk_release_clkdev as a first styep to clean up drivers which are
>>
>> s/styep/step/
>>
>
> Snip...
>
> I'll respond to these comments when I am back from my summer vacation trip
> after a week or two.
>
> Best regards
> Matti Vaittinen
>
>
>>
>


-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Matti "Maz" Vaittinen

When you feel blue, no one sees your tears...
When your down, no one understands your struggle...
When you feel happy, no one notices your smile...
But fart just once...
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Re: [PATCH] clk: clkdev - add managed versions of lookup registrations

[Matti Vaittinen]

Hello All,

Sorry for longish delay but the exceptionally great summer in Finland
has kept me away from computer... Now when I am back from my travels
it's time to focus on patches again =)

On Sun, Jul 08, 2018 at 11:33:44PM -0700, Stephen Boyd wrote:
> Quoting Matti Vaittinen (2018-06-28 00:54:53)
> > Add devm_clk_hw_register_clkdev, devm_clk_register_clkdev and
> > devm_clk_release_clkdev as a first styep to clean up drivers which are
> 
> s/styep/step/

Thanks.

> > leaking clkdev lookups at driver remove.
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> > ---
> > 
> >  drivers/clk/clkdev.c   | 165 +++++++++++++++++++++++++++++++++++++++++--------
> >  include/linux/clkdev.h |   8 +++
> 
> Also need to update the Documentation file at
> Documentation/driver-model/devres.txt

Right. I'd better check that file then. Thanks for pointing it out.

> 
> >  2 files changed, 147 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/clk/clkdev.c b/drivers/clk/clkdev.c
> > index 7513411140b6..4752a0004a6c 100644
> > --- a/drivers/clk/clkdev.c
> > +++ b/drivers/clk/clkdev.c
> > @@ -390,7 +390,7 @@ void clkdev_drop(struct clk_lookup *cl)
> >  }
> >  EXPORT_SYMBOL(clkdev_drop);
> >  
> > -static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
> > +static struct clk_lookup *do_clk_register_clkdev(struct clk_hw *hw,
> 
> Don't rename this.
> 

I did rename this as I introduced new internal __clk_register_clkdev
(see below) - which is utilized by the clk_register_clkdev,
clk_hw_register_clkdev and devm_clk_hw_register_clkdev. This allowed
me to cut off some duplicated code from clk_register_clkdev and
clk_hw_register_clkdev.

(Mainly the:

	/*
	 * 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) 
	... con_id, "%s", dev_id);
else
	... con_id, NULL);

parameter selection for old __clk_register_clkdev (which I renamed to
do_clk_register_clkdev).

So I tried to reduce code by deciding this only in the new wrapper
function __clk_register_clkdev. For me it was more obvioust that
__clk_register_clkdev would be next internal layer for clk_register_clkdev.
The old __clk_register_clkdev - which is now named as do_clk_register_clkdev
is the final layer doing lookup and registration.

> >                                                 const char *con_id,
> >                                                 const char *dev_id, ...)
> >  {
> > @@ -404,6 +404,24 @@ static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
> >         return cl;
> >  }
> >  
> > +static struct clk_lookup *__clk_register_clkdev(struct clk_hw *hw,
> > +       const char *con_id, const char *dev_id)
> > +{
> > +       struct clk_lookup *cl;
> > +
> > +       /*
> > +        * 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 = do_clk_register_clkdev(hw, con_id, "%s",
> > +                                          dev_id);
> > +       else
> > +               cl = do_clk_register_clkdev(hw, con_id, NULL);
> > +
> > +       return cl;
> 
> I think this is the same code as before? Try to minimize the diff so we
> can focus on what's really changing.
>

This is code that earlier was duiplicated in both the
clk_register_clkdev and clk_hw_register_clkdev. I cleaned the code
duplication by adding this new __clk_register_clkdev function.
 
> > +}
> > +
> >  /**
> >   * clk_register_clkdev - register one clock lookup for a struct clk
> >   * @clk: struct clk to associate with all clk_lookups
> [...]
> > +
> > +/**
> > + * 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)
> > +{
> > +       struct clk_lookup **cl = NULL;
> 
> Don't assign to NULL to just overwrite it later.

Right.

> >  
> >         if (IS_ERR(hw))
> >                 return PTR_ERR(hw);
> 
> Put another newline here.
> 
Ok.

> > +       cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL);
> > +       if (cl) {
> > +               *cl = __clk_register_clkdev(hw, con_id, dev_id);
> 
> Why can't we just call clk_hw_register_clkdev()? Sure the IS_ERR()
> chain is duplicated, but that can be left out of the devm version and
> rely on the clk_hw_register_clkdev() to take care of it otherwise.
>
We could. But as I anyways introduced the new __clk_register_clkdev - in
order to slighly simplify clk_register_clkdev and clk_hw_register_clkdev
- it was convenient to not dublicate the IS_ERR chain and use the interal
__clk_register_clkdev -variant. And actually, I was not sure if it is
required to have some fast handling for the IS_ERR cases here and hence
I thought it should be checked before devres_alloc. But if there is no
need for priorizing this check - then I would remove IS_ERR checks from
devm_clk_hw_register_clkdev and clk_hw_register_clkdev and do it only in the
__clk_register_clkdev. Unfortunately we need to keep it in
clk_register_clkdev because this must be checked before we do
__clk_get_hw(clk). Anyways, that would further simplify this to something
like (untested, not even compiled code below which is only meant to explain
what I mean):

static int __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);

	if (dev_id)
		*cl = do_clk_register_clkdev(hw, con_id, "%s",
                                           dev_id);
        else
                *cl = do_clk_register_clkdev(hw, con_id, NULL);

        return (*cl) ? 0 : -ENOMEM;


int clk_register_clkdev(struct clk *clk, const char *con_id,
        const char *dev_id)
{
	int rval;
	struct clk_lookup *cl;

	if (!IS_ERR(clk))
		return __clk_register_clkdev(__clk_get_hw(clk), &cl, con_id, dev_id);

	return PTR_ERR(clk);	
}

int clk_hw_register_clkdev(struct clk_hw *hw, const char *con_id,
        const char *dev_id)
{
	int rval;
	struct clk_lookup *cl;
	return __clk_register_clkdev(hw, con_id, &cl, dev_id);
}

int devm_clk_hw_register_clkdev(struct device *dev, struct clk_hw *hw,
                                const char *con_id, const char *dev_id)
{
        struct clk_lookup **cl;
	int rval = -ENOMEM;

        if (IS_ERR(hw))
                return PTR_ERR(hw);

        cl = devres_alloc(devm_clkdev_release, sizeof(*cl), GFP_KERNEL);
        if (cl) {
                rval = __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);

or do you prefer that I do not touch the existing clk_register_clkdev
and clk_hw_register_clkdev at all and only add
devm_clk_hw_register_clkdev? If that's what you prefer we can go with it
too. I just think doing the 

if (dev_id) 
	... con_id, "%s", dev_id);
else
	... con_id, NULL);

selection only in one function makes this cleaner.

> > +/**
> > + * devm_clk_register_clkdev - managed clk lookup registration for a struct clk
> > + * @dev: device this lookup is bound
> > + * @clk: struct clk to associate with all clk_lookups
> > + * @con_id: connection ID string on device
> > + * @dev_id: 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 clks
> > + * from a previous clk_register() call, and return the error code for
> > + * those.  This is to permit this function to be called immediately
> > + * after clk_register().
> > + */
> > +int devm_clk_register_clkdev(struct device *dev, struct clk *clk,
> > +                            const char *con_id, const char *dev_id)
> 
> I wouldn't even add this function, to encourage driver writers to
> migrate to clk_hw based registration functions and to avoid removing it
> later on.

I can remove this.

Best regards
	Matti Vaittinen