RE: [PATCH] ASoC: rsnd: adg: make rsnd_adg_clk_control() idempotent

[John Madieu]

Hi Morimoto-san,

Thank you for the review!

> -----Original Message-----
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Sent: jeudi 11 juin 2026 00:52
> To: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH] ASoC: rsnd: adg: make rsnd_adg_clk_control()
> idempotent
> 
> 
> Hi John
> 
> > rsnd_adg_clk_control() is asymmetric on the disable path: the clkin
> > clocks are guarded by clkin_rate[], but the "adg" clock is disabled
> > unconditionally. If an enable attempt fails (for example a clkin
> > failing to turn on during resume), the error path correctly rolls
> > everything back, but rsnd_resume() ignores the return value, so the
> > following system suspend calls rsnd_adg_clk_disable() again and
> > underflows the "adg" clock enable count:
> >
> >   adg_0_clks1 already disabled
> >   WARNING: drivers/clk/clk.c:1188 clk_core_disable+0xa4/0xac
> >   Call trace:
> >    clk_core_disable+0xa4/0xac (P)
> >    clk_disable+0x30/0x4c
> >    rsnd_adg_clk_control+0x9c/0x2cc
> >    rsnd_suspend+0x20/0x74
> >    device_suspend+0x140/0x3ec
> >    dpm_suspend+0x168/0x270
> >
> > Track the enable state explicitly and bail out of redundant
> > enable/disable calls, mirroring what is already done for the per-SSI
> > clock prepare state. A failed enable leaves the state as disabled, so
> > the next suspend becomes a no-op and the next resume retries cleanly.
> >
> > Fixes: 47899d53f86f ("ASoC: rsnd: adg: Add per-SSI ADG and SSIF supply
> > clock management")
> > Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> > ---
> >  sound/soc/renesas/rcar/adg.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/renesas/rcar/adg.c
> > b/sound/soc/renesas/rcar/adg.c index 5479cefb6dbe..53efd1be5139 100644
> > --- a/sound/soc/renesas/rcar/adg.c
> > +++ b/sound/soc/renesas/rcar/adg.c
> > @@ -45,6 +45,7 @@ struct rsnd_adg {
> >  	struct rsnd_mod mod;
> >  	int clkin_rate[CLKINMAX];
> >  	bool ssi_clk_prepared;
> > +	bool clk_enabled;
> 
> Can we use clk_is_enabled_when_prepared() instead ?

I don't think it can work here. clk_is_enabled_when_prepared() reports
a static property of the clock implementation (it returns true when
the clock has no .enable/.disable ops, i.e. clk_prepare() implicitly
enables it). It does not report the current enable state, and its
kernel-doc explicitly says:

 * Regardless of the value returned here, the caller must always invoke
 * clk_enable() or clk_prepare_enable()  and counterparts for usage counts
 * to be right.

which is precisely what gets violated in the bug this patch fixes: the
driver calls rsnd_adg_clk_disable() without a matching successful
enable, underflowing the "adg" clock usage count.

The CCF intentionally does not expose the enable state to clock
consumers (__clk_is_enabled() is provider-only), since enable_count is
shared between consumers and any snapshot of it would be racy. So a
driver-local flag, mirroring the existing ssi_clk_prepared, seems to
be the standard way to keep our own enable/disable calls balanced.

Best regards,
John