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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |