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

Message

Philipp Zabel Sept. 25, 2024, 4:40 p.m. UTC

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.

regards
Philipp

Signed-off-by: Philipp Zabel <[e-mail]>
---
Changes in v2:
- Clear RESET_CONTROL_FLAGS_BIT_OPTIONAL out of flags.
- Check flags in __of_reset_control_get().
- Fix devm_reset_control_array_get() documentation.
- Clear RESET_CONTROL_FLAGS_BIT_DEASSERTED out of flags.
- Link to v1: https://lore.kernel.org/r/[e-mail]

---
Philipp Zabel (3):
      reset: replace boolean parameters with flags parameter
      reset: Add devres helpers to request pre-deasserted reset controls
      reset: uniphier-glue: Use devm_reset_control_bulk_get_shared_deasserted()

 drivers/reset/core.c                | 119 +++++++++++-----
 drivers/reset/reset-uniphier-glue.c |  24 +---
 include/linux/reset.h               | 274 ++++++++++++++++++++++++++++--------
 3 files changed, 303 insertions(+), 114 deletions(-)
---
base-commit: 487b1b32e317b85c2948eb4013f3e089a0433d49
change-id: 20240621-reset-get-deasserted-5185a8e4a706

Best regards,

Comments

Uwe Kleine-König Sept. 26, 2024, 5:57 a.m. UTC | #1

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 <[e-mail]>

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.

Best regards
Uwe

Philipp Zabel Oct. 1, 2024, 3:50 p.m. UTC | #2

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 <[e-mail]>
>
> 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.

Applied to reset/next.

[1/3] reset: replace boolean parameters with flags parameter
      https://git.pengutronix.de/cgit/pza/linux/commit/?id=dad35f7d2fc1
[2/3] reset: Add devres helpers to request pre-deasserted reset controls
      https://git.pengutronix.de/cgit/pza/linux/commit/?id=dad35f7d2fc1
[3/3] reset: uniphier-glue: Use devm_reset_control_bulk_get_shared_deasserted()
      https://git.pengutronix.de/cgit/pza/linux/commit/?id=c0260e2b0ed8

regards
Philipp

Uwe Kleine-König Oct. 2, 2024, 8:42 a.m. UTC | #3

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 <[e-mail]>
> >
> > 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