Re: [PATCH v6 RESEND 1/6] clk: generalize devm_clk_get() a bit - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Cc: linux-rtc@vger.kernel.org,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-pwm@vger.kernel.org, Stephen Boyd <sboyd@kernel.org>,
	Michael Turquette <mturquette@baylibre.com>,
	linux-clk@vger.kernel.org, linux-spi@vger.kernel.org,
	Wolfram Sang <wsa@kernel.org>,
	Oleksij Rempel <o.rempel@pengutronix.de>,
	Ludovic Desroches <ludovic.desroches@microchip.com>,
	Mark Brown <broonie@kernel.org>,
	Thierry Reding <thierry.reding@gmail.com>,
	kernel@pengutronix.de, Andrew Morton <akpm@linux-foundation.org>,
	Lee Jones <lee.jones@linaro.org>,
	Claudiu Beznea <claudiu.beznea@microchip.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v6 RESEND 1/6] clk: generalize devm_clk_get() a bit
Date: Mon, 10 May 2021 19:27:06 +0200	[thread overview]
Message-ID: <20210510172706.lfafdkfevya7ycqq@pengutronix.de> (raw)
In-Reply-To: <20210510180205.000004dd@Huawei.com>


[-- Attachment #1.1: Type: text/plain, Size: 1755 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
