diff mbox series

[v6,RESEND,1/6] clk: generalize devm_clk_get() a bit

Message ID 20210510061724.940447-2-u.kleine-koenig@pengutronix.de (mailing list archive)
State Superseded, archived
Headers show
Series clk: provide new devm helpers for prepared and enabled clocks | expand

Commit Message

Uwe Kleine-König May 10, 2021, 6:17 a.m. UTC 
Allow to add an exit hook to devm managed clocks. Also use
clk_get_optional() in devm_clk_get_optional instead of open coding it.
The generalisation will be used in the next commit to add some more
devm_clk helpers.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/clk/clk-devres.c | 67 ++++++++++++++++++++++++++++++----------
 1 file changed, 50 insertions(+), 17 deletions(-)

Comments

Jonathan Cameron May 10, 2021, 5:02 p.m. UTC | #1 
On Mon, 10 May 2021 08:17:19 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Allow to add an exit hook to devm managed clocks. Also use
> clk_get_optional() in devm_clk_get_optional instead of open coding it.
> The generalisation will be used in the next commit to add some more
> devm_clk helpers.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

If feels marginally odd to register cleanup that we know won't do anything
for the optional case, but it works as far as I can tell and it would be
a little fiddly to special case it.

FWIW

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/clk/clk-devres.c | 67 ++++++++++++++++++++++++++++++----------
>  1 file changed, 50 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
> index be160764911b..91c995815b57 100644
> --- a/drivers/clk/clk-devres.c
> +++ b/drivers/clk/clk-devres.c
> @@ -4,39 +4,72 @@
>  #include <linux/export.h>
>  #include <linux/gfp.h>
>  
> +struct devm_clk_state {
> +	struct clk *clk;
> +	void (*exit)(struct clk *clk);
> +};
> +
>  static void devm_clk_release(struct device *dev, void *res)
>  {
> -	clk_put(*(struct clk **)res);
> +	struct devm_clk_state *state = *(struct devm_clk_state **)res;
> +
> +	if (state->exit)
> +		state->exit(state->clk);
> +
> +	clk_put(state->clk);
>  }
>  
> -struct clk *devm_clk_get(struct device *dev, const char *id)
> +static struct clk *__devm_clk_get(struct device *dev, const char *id,
> +				  struct clk *(*get)(struct device *dev, const char *id),
> +				  int (*init)(struct clk *clk),
> +				  void (*exit)(struct clk *clk))
>  {
> -	struct clk **ptr, *clk;
> +	struct devm_clk_state *state;
> +	struct clk *clk;
> +	int ret;
>  
> -	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
> -	if (!ptr)
> +	state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
> +	if (!state)
>  		return ERR_PTR(-ENOMEM);
>  
> -	clk = clk_get(dev, id);
> -	if (!IS_ERR(clk)) {
> -		*ptr = clk;
> -		devres_add(dev, ptr);
> -	} else {
> -		devres_free(ptr);
> +	clk = get(dev, id);
> +	if (IS_ERR(clk)) {
> +		ret = PTR_ERR(clk);
> +		goto err_clk_get;
>  	}
>  
> +	if (init) {
> +		ret = init(clk);
> +		if (ret)
> +			goto err_clk_init;
> +	}
> +
> +	state->clk = clk;
> +	state->exit = exit;
> +
> +	devres_add(dev, state);
> +
>  	return clk;
> +
> +err_clk_init:
> +
> +	clk_put(clk);
> +err_clk_get:
> +
> +	devres_free(state);
> +	return ERR_PTR(ret);
>  }
> -EXPORT_SYMBOL(devm_clk_get);
>  
> -struct clk *devm_clk_get_optional(struct device *dev, const char *id)
> +struct clk *devm_clk_get(struct device *dev, const char *id)
>  {
> -	struct clk *clk = devm_clk_get(dev, id);
> +	return __devm_clk_get(dev, id, clk_get, NULL, NULL);
>  
> -	if (clk == ERR_PTR(-ENOENT))
> -		return NULL;
> +}
> +EXPORT_SYMBOL(devm_clk_get);
>  
> -	return clk;
> +struct clk *devm_clk_get_optional(struct device *dev, const char *id)
> +{
> +	return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
>  }
>  EXPORT_SYMBOL(devm_clk_get_optional);
>
Uwe Kleine-König May 10, 2021, 5:27 p.m. UTC | #2 
On Mon, May 10, 2021 at 06:02:05PM +0100, Jonathan Cameron wrote:
> On Mon, 10 May 2021 08:17:19 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Allow to add an exit hook to devm managed clocks. Also use
> > clk_get_optional() in devm_clk_get_optional instead of open coding it.
> > The generalisation will be used in the next commit to add some more
> > devm_clk helpers.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> If feels marginally odd to register cleanup that we know won't do anything
> for the optional case, but it works as far as I can tell and it would be
> a little fiddly to special case it.

It took a moment for me to understand your concern. Yes, I register a
cleanup even if (*init) happens to be clk_get_optional() and it returned
NULL. It's not hard to optimize that:

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index b54f7f0f2a35..2420d6ae7b33 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -38,14 +38,17 @@ static struct clk *__devm_clk_get(struct device *dev, const char *id,
 		goto err_clk_get;
 	}
 
-	if (init) {
-		ret = init(clk);
-		if (ret)
-			goto err_clk_init;
+	/* short-cut the case where clk_get_optional returned NULL */
+	if (clk) {
+		if (init) {
+			ret = init(clk);
+			if (ret)
+				goto err_clk_init;
+		}
+		state->exit = exit;
 	}
 
 	state->clk = clk;
-	state->exit = exit;
 
 	devres_add(dev, state);

But I'd value simplicity over micro-optimizing and keep the code as is.
 
Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index be160764911b..91c995815b57 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -4,39 +4,72 @@ 
 #include <linux/export.h>
 #include <linux/gfp.h>
 
+struct devm_clk_state {
+	struct clk *clk;
+	void (*exit)(struct clk *clk);
+};
+
 static void devm_clk_release(struct device *dev, void *res)
 {
-	clk_put(*(struct clk **)res);
+	struct devm_clk_state *state = *(struct devm_clk_state **)res;
+
+	if (state->exit)
+		state->exit(state->clk);
+
+	clk_put(state->clk);
 }
 
-struct clk *devm_clk_get(struct device *dev, const char *id)
+static struct clk *__devm_clk_get(struct device *dev, const char *id,
+				  struct clk *(*get)(struct device *dev, const char *id),
+				  int (*init)(struct clk *clk),
+				  void (*exit)(struct clk *clk))
 {
-	struct clk **ptr, *clk;
+	struct devm_clk_state *state;
+	struct clk *clk;
+	int ret;
 
-	ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
-	if (!ptr)
+	state = devres_alloc(devm_clk_release, sizeof(*state), GFP_KERNEL);
+	if (!state)
 		return ERR_PTR(-ENOMEM);
 
-	clk = clk_get(dev, id);
-	if (!IS_ERR(clk)) {
-		*ptr = clk;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
+	clk = get(dev, id);
+	if (IS_ERR(clk)) {
+		ret = PTR_ERR(clk);
+		goto err_clk_get;
 	}
 
+	if (init) {
+		ret = init(clk);
+		if (ret)
+			goto err_clk_init;
+	}
+
+	state->clk = clk;
+	state->exit = exit;
+
+	devres_add(dev, state);
+
 	return clk;
+
+err_clk_init:
+
+	clk_put(clk);
+err_clk_get:
+
+	devres_free(state);
+	return ERR_PTR(ret);
 }
-EXPORT_SYMBOL(devm_clk_get);
 
-struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+struct clk *devm_clk_get(struct device *dev, const char *id)
 {
-	struct clk *clk = devm_clk_get(dev, id);
+	return __devm_clk_get(dev, id, clk_get, NULL, NULL);
 
-	if (clk == ERR_PTR(-ENOENT))
-		return NULL;
+}
+EXPORT_SYMBOL(devm_clk_get);
 
-	return clk;
+struct clk *devm_clk_get_optional(struct device *dev, const char *id)
+{
+	return __devm_clk_get(dev, id, clk_get_optional, NULL, NULL);
 }
 EXPORT_SYMBOL(devm_clk_get_optional);