[PATCH] ASoC: ak4458: Check reset status after deassert

[PATCH] ASoC: ak4458: Check reset status after deassert

[shengjiu.wang]

From: Shengjiu Wang <shengjiu.wang@nxp.com>

The reset line of the ak4458 can be shared, for example when two ak4458
devices use the same reset GPIO. In that case the two codecs may call
reset_control_deassert() concurrently from their runtime resume paths.

Only the first caller performs the actual hardware deassert, which may
sleep (for example a GPIO-expander based reset controller). A second
caller merely increments the shared deassert count and returns
immediately, so it can continue while the first caller's .deassert() has
not finished releasing the line yet, leaving the device still in reset.

Poll reset_control_status() after deassert so a caller does not continue
until the reset is really released. reset_control_status() returns a
positive value while the line is still asserted, zero once it is
deasserted, and a negative errno when the reset controller does not
implement status reporting. Stop polling on status <= 0 so controllers
without a .status callback do not spin for the full timeout, and check
the reset_control_deassert() return value as well.

Fixes: 8a0de73cf9dc ("ASoC: ak4458: add optional reset control to instead of gpio")
Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
---
 sound/soc/codecs/ak4458.c | 32 +++++++++++++++++++++++++++++---
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/ak4458.c b/sound/soc/codecs/ak4458.c
index a74be99e877d..64984974ff88 100644
--- a/sound/soc/codecs/ak4458.c
+++ b/sound/soc/codecs/ak4458.c
@@ -8,6 +8,7 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pm_runtime.h>
@@ -635,11 +636,36 @@ static struct snd_soc_dai_driver ak4497_dai = {
 
 static void ak4458_reset(struct ak4458_priv *ak4458, bool active)
 {
+	int ret, status;
+
 	if (!IS_ERR_OR_NULL(ak4458->reset)) {
-		if (active)
+		if (active) {
 			reset_control_assert(ak4458->reset);
-		else
-			reset_control_deassert(ak4458->reset);
+		} else {
+			ret = reset_control_deassert(ak4458->reset);
+			if (ret) {
+				dev_warn(ak4458->dev,
+					 "failed to deassert reset: %d\n", ret);
+			} else {
+				/*
+				 * On a shared reset line another consumer may
+				 * still be in its hardware .deassert(), which can
+				 * sleep, so the line may not be released yet.
+				 * Poll the status until it reads deasserted (0).
+				 * A negative return means the controller does not
+				 * report status, so stop polling in that case.
+				 */
+				ret = read_poll_timeout(reset_control_status, status,
+							status <= 0, 100, 20000,
+							false, ak4458->reset);
+				if (ret)
+					dev_warn(ak4458->dev,
+						 "reset still asserted after deassert\n");
+				else if (status < 0)
+					dev_dbg(ak4458->dev,
+						"reset status not supported: %d\n", status);
+			}
+		}
 		usleep_range(1000, 2000);
 	}
 }
-- 
2.34.1

Re: [PATCH] ASoC: ak4458: Check reset status after deassert

[Alvin Šipraga]

On Sat, Oct 10, 2026 at 04:40:58PM +0800, shengjiu.wang@oss.nxp.com wrote:
> From: Shengjiu Wang <shengjiu.wang@nxp.com>
> 
> The reset line of the ak4458 can be shared, for example when two ak4458
> devices use the same reset GPIO. In that case the two codecs may call
> reset_control_deassert() concurrently from their runtime resume paths.
> 
> Only the first caller performs the actual hardware deassert, which may
> sleep (for example a GPIO-expander based reset controller). A second
> caller merely increments the shared deassert count and returns
> immediately, so it can continue while the first caller's .deassert() has
> not finished releasing the line yet, leaving the device still in reset.

This seems like something that should be fixed in the reset controller
framework. In fact the issue you describe seems to contradict the
documentation of reset_control_deassert():

/**
 * reset_control_deassert - deasserts the reset line
 * @rstc: reset controller
 *
 * After calling this function, the reset is guaranteed to be deasserted.

^ (1) seems not to be the case as you have found

 * Consumers must not use reset_control_reset on shared reset lines when
 * reset_control_(de)assert has been used.

^ (2) isn't this rule also being broken in the suspend/resume path of
      this driver?

 *
 * If rstc is NULL it is an optional reset and the function will just
 * return 0.
 */
int reset_control_deassert(struct reset_control *rstc)
...


(1) being fixed in the framework would solve your problem.

But (2) seems a bit more intractable. Shared reset GPIOs have always
been a pain... I wonder if there is a better approach?

Kind regards,
Alvin

Re: [PATCH] ASoC: ak4458: Check reset status after deassert

[Shengjiu Wang]

Hi Alvin, Philipp Zabel

On Sun, Oct 11, 2026 at 3:05 AM Alvin Šipraga <alvin.sipraga@analog.com> wrote:
>
> On Sat, Oct 10, 2026 at 04:40:58PM +0800, shengjiu.wang@oss.nxp.com wrote:
> > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> >
> > The reset line of the ak4458 can be shared, for example when two ak4458
> > devices use the same reset GPIO. In that case the two codecs may call
> > reset_control_deassert() concurrently from their runtime resume paths.
> >
> > Only the first caller performs the actual hardware deassert, which may
> > sleep (for example a GPIO-expander based reset controller). A second
> > caller merely increments the shared deassert count and returns
> > immediately, so it can continue while the first caller's .deassert() has
> > not finished releasing the line yet, leaving the device still in reset.
>
> This seems like something that should be fixed in the reset controller
> framework. In fact the issue you describe seems to contradict the
> documentation of reset_control_deassert():
>
> /**
>  * reset_control_deassert - deasserts the reset line
>  * @rstc: reset controller
>  *
>  * After calling this function, the reset is guaranteed to be deasserted.
>
> ^ (1) seems not to be the case as you have found

Agreed. Looking at the documentation again, the guarantee

* After calling this function, the reset is guaranteed to be deasserted.

does not hold for a shared reset control today, so the issue is really in
the reset core. For a shared reset only the first caller runs the
hardware operation; later callers just bump the refcount and return
immediately, e.g. in reset_control_deassert():

if (atomic_inc_return(&rstc->deassert_count) != 1)
return 0;

So when two consumers of the same shared reset (here two ak4458 sharing
one reset GPIO) call concurrently, the second caller can return before
the first caller's .deassert() - which may sleep, e.g. a GPIO-expander
based controller - has actually released the line. assert() and reset()
have the same pattern.

Rather than work around it in the codec, I'd like to propose a reset-core
fix for your review. The idea is to take the existing per-control
rstc->lock across the shared refcount update and the hardware
.assert()/.deassert()/.reset(), so a second concurrent caller waits for
the first caller's hardware operation to complete. The lock is taken
before the SRCU section, matching reset_control_acquire(), and these
paths do not take rcdev->lock, so no new lock ordering is introduced:

if (reset_control_is_array(rstc))
return reset_control_array_deassert(rstc_to_array(rstc));

guard(mutex)(&rstc->lock);
guard(srcu)(&rstc->srcu);
...

Does this approach look reasonable to you? If so I will send it as a
proper patch and drop the ASoC: ak4458 change.

>
>  * Consumers must not use reset_control_reset on shared reset lines when
>  * reset_control_(de)assert has been used.
>
> ^ (2) isn't this rule also being broken in the suspend/resume path of
>       this driver?

I don't think so: the ak4458 suspend/resume path only uses
reset_control_assert()/reset_control_deassert(); it never calls
reset_control_reset(), so this rule isn't violated here. Please let me
know if you had a different concern in mind.

Best regards
Shengjiu Wang
>
>  *
>  * If rstc is NULL it is an optional reset and the function will just
>  * return 0.
>  */
> int reset_control_deassert(struct reset_control *rstc)
> ...
>
>
> (1) being fixed in the framework would solve your problem.
>
> But (2) seems a bit more intractable. Shared reset GPIOs have always
> been a pain... I wonder if there is a better approach?
>
> Kind regards,
> Alvin
>

Re: [PATCH] ASoC: ak4458: Check reset status after deassert

[Alvin Šipraga]

Hi Shengjiu,

On Mon, Oct 12, 2026 at 02:49:03PM +0800, Shengjiu Wang wrote:
> Hi Alvin, Philipp Zabel
> 
> On Sun, Oct 11, 2026 at 3:05 AM Alvin Šipraga <alvin.sipraga@analog.com> wrote:
> >
> > On Sat, Oct 10, 2026 at 04:40:58PM +0800, shengjiu.wang@oss.nxp.com wrote:
> > > From: Shengjiu Wang <shengjiu.wang@nxp.com>
> > >
> > > The reset line of the ak4458 can be shared, for example when two ak4458
> > > devices use the same reset GPIO. In that case the two codecs may call
> > > reset_control_deassert() concurrently from their runtime resume paths.
> > >
> > > Only the first caller performs the actual hardware deassert, which may
> > > sleep (for example a GPIO-expander based reset controller). A second
> > > caller merely increments the shared deassert count and returns
> > > immediately, so it can continue while the first caller's .deassert() has
> > > not finished releasing the line yet, leaving the device still in reset.
> >
> > This seems like something that should be fixed in the reset controller
> > framework. In fact the issue you describe seems to contradict the
> > documentation of reset_control_deassert():
> >
> > /**
> >  * reset_control_deassert - deasserts the reset line
> >  * @rstc: reset controller
> >  *
> >  * After calling this function, the reset is guaranteed to be deasserted.
> >
> > ^ (1) seems not to be the case as you have found
> 
> Agreed. Looking at the documentation again, the guarantee
> 
> * After calling this function, the reset is guaranteed to be deasserted.
> 
> does not hold for a shared reset control today, so the issue is really in
> the reset core. For a shared reset only the first caller runs the
> hardware operation; later callers just bump the refcount and return
> immediately, e.g. in reset_control_deassert():
> 
> if (atomic_inc_return(&rstc->deassert_count) != 1)
> return 0;
> 
> So when two consumers of the same shared reset (here two ak4458 sharing
> one reset GPIO) call concurrently, the second caller can return before
> the first caller's .deassert() - which may sleep, e.g. a GPIO-expander
> based controller - has actually released the line. assert() and reset()
> have the same pattern.
> 
> Rather than work around it in the codec, I'd like to propose a reset-core
> fix for your review. The idea is to take the existing per-control
> rstc->lock across the shared refcount update and the hardware
> .assert()/.deassert()/.reset(), so a second concurrent caller waits for
> the first caller's hardware operation to complete. The lock is taken
> before the SRCU section, matching reset_control_acquire(), and these
> paths do not take rcdev->lock, so no new lock ordering is introduced:
> 
> if (reset_control_is_array(rstc))
> return reset_control_array_deassert(rstc_to_array(rstc));
> 
> guard(mutex)(&rstc->lock);
> guard(srcu)(&rstc->srcu);
> ...
> 
> Does this approach look reasonable to you? If so I will send it as a
> proper patch and drop the ASoC: ak4458 change.

Yeah, I think that is the correct fix!

> 
> >
> >  * Consumers must not use reset_control_reset on shared reset lines when
> >  * reset_control_(de)assert has been used.
> >
> > ^ (2) isn't this rule also being broken in the suspend/resume path of
> >       this driver?
> 
> I don't think so: the ak4458 suspend/resume path only uses
> reset_control_assert()/reset_control_deassert(); it never calls
> reset_control_reset(), so this rule isn't violated here. Please let me
> know if you had a different concern in mind.

Ahh, sorry! I conflated reset_control_reset() with
reset_control_deassert()... Please ignore this part :-)

Kind regards,
Alvin