Re: [PATCH v2 0/3] reset: Requesting pre-deasserted, auto-reasserting reset controls via devres

[Uwe Kleine-König]

On Tue, Oct 01, 2024 at 05:50:59PM +0200, Philipp Zabel wrote:
> Hi Uwe,
> 
> On Do, 2024-09-26 at 07:57 +0200, Uwe Kleine-König wrote:
> > Hello Philipp,
> > 
> > On Wed, Sep 25, 2024 at 06:40:08PM +0200, Philipp Zabel wrote:
> > > There is a recurring pattern of drivers requesting a reset control and
> > > deasserting the reset during probe, followed by registering a reset
> > > assertion via devm_add_action_or_reset().
> > > 
> > > We can simplify this by providing devm_reset_control_get_*_deasserted()
> > > helpers that return an already deasserted reset control, similarly to
> > > devm_clk_get_enabled().
> > > 
> > > This doesn't remove a lot of boilerplate at each instance, but there are
> > > quite a few of them now.
> > 
> > I really like it, thanks for respinning!
> > 
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> >
> > Two small notes: I think __devm_reset_control_get() could be a bit
> > simplified if it used devm_add_action_or_reset() instead of
> > devres_alloc() + devres_add(). I also would have prefered an if block
> > (or a function pointer) in the release function instead of a ?:
> > construct to select the right release function like e.g.
> > __devm_clk_get() does it. But that's both subjective I think and
> > orthogonal to this patch set.
> 
> Thank you. Not sure about using devm_add_action_or_reset(), but I'll
> look into using a single release function.

The switch to devm_add_action_or_reset() would look as follows (still
with two release functions):

diff --git a/drivers/reset/core.c b/drivers/reset/core.c
index 22f67fc77ae5..499dbcdedabd 100644
--- a/drivers/reset/core.c
+++ b/drivers/reset/core.c
@@ -1231,53 +1231,43 @@ void reset_control_bulk_put(int num_rstcs, struct reset_control_bulk_data *rstcs
 }
 EXPORT_SYMBOL_GPL(reset_control_bulk_put);
 
-static void devm_reset_control_release(struct device *dev, void *res)
+static void devm_reset_control_release(void *data)
 {
-	reset_control_put(*(struct reset_control **)res);
+	reset_control_put((struct reset_control *)data);
 }
 
-static void devm_reset_control_release_deasserted(struct device *dev, void *res)
+static void devm_reset_control_release_deasserted(void *data)
 {
-	struct reset_control *rstc = *(struct reset_control **)res;
-
-	reset_control_assert(rstc);
-	reset_control_put(rstc);
+	reset_control_assert((struct reset_control *)data);
 }
 
 struct reset_control *
 __devm_reset_control_get(struct device *dev, const char *id, int index,
 			 enum reset_control_flags flags)
 {
-	struct reset_control **ptr, *rstc;
+	struct reset_control *rstc;
 	bool deasserted = flags & RESET_CONTROL_FLAGS_BIT_DEASSERTED;
-
-	ptr = devres_alloc(deasserted ? devm_reset_control_release_deasserted :
-			   devm_reset_control_release, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	int ret;
 
 	flags &= ~RESET_CONTROL_FLAGS_BIT_DEASSERTED;
 
 	rstc = __reset_control_get(dev, id, index, flags);
-	if (IS_ERR_OR_NULL(rstc)) {
-		devres_free(ptr);
+	if (IS_ERR_OR_NULL(rstc))
 		return rstc;
-	}
+
+	ret = devm_add_action_or_reset(dev, devm_reset_control_release, rstc);
+	if (ret)
+		return ERR_PTR(ret);
 
 	if (deasserted) {
-		int ret;
-
 		ret = reset_control_deassert(rstc);
-		if (ret) {
-			reset_control_put(rstc);
-			devres_free(ptr);
+		if (ret)
 			return ERR_PTR(ret);
-		}
-	}
 
-	*ptr = rstc;
-	devres_add(dev, ptr);
+		ret = devm_add_action_or_reset(dev, devm_reset_control_release_deasserted, rstc);
+		if (ret)
+			return ERR_PTR(ret);
+	}
 
 	return rstc;
 }
@@ -1472,21 +1462,16 @@ EXPORT_SYMBOL_GPL(of_reset_control_array_get);
 struct reset_control *
 devm_reset_control_array_get(struct device *dev, enum reset_control_flags flags)
 {
-	struct reset_control **ptr, *rstc;
-
-	ptr = devres_alloc(devm_reset_control_release, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct reset_control *rstc;
+	int ret;
 
 	rstc = of_reset_control_array_get(dev->of_node, flags);
-	if (IS_ERR_OR_NULL(rstc)) {
-		devres_free(ptr);
+	if (IS_ERR_OR_NULL(rstc))
 		return rstc;
-	}
 
-	*ptr = rstc;
-	devres_add(dev, ptr);
+	ret = devm_add_action_or_reset(dev, devm_reset_control_release, rstc);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return rstc;
 }

Only compile tested! In my eyes that's an improvement, but up to you to
decide.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature