Re: [PATCH v5 11/14] ASoC: rsnd: adg: Add per-SSI ADG and SSIF supply clock management

[Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>]

Hi John

Thank you for the patch

> RZ/G3E's ADG module requires explicit clock management for SSI audio
> interfaces that differs from R-Car Gen2/Gen3/Gen4:
> 
>  - Per-SSI ADG clocks (adg.ssi.N) for each SSI module
>  - A shared SSIF supply clock for the SSI subsystem
> 
> These clocks are acquired using optional APIs, making them transparent
> to platforms that do not require them.
> 
> Clock prepare/unprepare is handled in rsnd_adg_clk_control(), which
> is called from probe, remove, suspend and resume (all sleepable
> contexts). The trigger path (atomic context) only calls
> clk_enable/clk_disable, which is atomic-safe and requires no
> additional splitting.
> 
> Signed-off-by: John Madieu <john.madieu.xa@bp.renesas.com>
> ---
(snip)
> @@ -424,9 +455,35 @@ int rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
>  	if (ret < 0)
>  		rsnd_adg_clk_disable(priv);
>  
> +	/* RZ/G3E: per-SSI ADG and SSIF supply clocks */
> +	if (enable) {
> +		for (i = 0; i < ADG_SSI_MAX; i++) {
> +			ret = clk_prepare(adg->clk_adg_ssi[i]);
> +			if (ret < 0) {
> +				while (--i >= 0)
> +					clk_unprepare(adg->clk_adg_ssi[i]);
> +				rsnd_adg_clk_disable(priv);
> +				return ret;
> +			}
> +		}
> +		ret = clk_prepare(adg->clk_ssif_supply);
> +		if (ret < 0) {
> +			for (i = 0; i < ADG_SSI_MAX; i++)
> +				clk_unprepare(adg->clk_adg_ssi[i]);
> +			rsnd_adg_clk_disable(priv);
> +			return ret;
> +		}
> +	}
> +
>  	/* disable adg */
> -	if (!enable)
> +	if (!enable) {
> +		/* RZ/G3E: unprepare per-SSI and supply clocks */
> +		clk_unprepare(adg->clk_ssif_supply);
> +		for (i = 0; i < ADG_SSI_MAX; i++)
> +			clk_unprepare(adg->clk_adg_ssi[i]);
> +
>  		clk_disable_unprepare(adg->adg);
> +	}
>  
>  	return ret;
>  }

As mentioned in comment, to avoid duplicate code (like above), it will
call rsnd_adg_clk_disable() in case of rollback, too.

	/*
	 * rsnd_adg_clk_enable() might return error (_disable() will not).
	 * We need to rollback in such case
	 */
	if (ret < 0)
		rsnd_adg_clk_disable(priv);

Because of this style, I guess "enable" need to call clk_prepare() for *all*
clk without loop break, even though it returuns error. And call clk_unprepare()
for *all* clk when "rollback" and/or "disable".

Enable
	for (i = 0; i < ADG_SSI_MAX; i++)
		ret |= clk_prepare(...);
		    ^^
	ret |= clk_prepare(...);
	    ^^
	...
	if (ret < 0)
		rsnd_adg_clk_disable(priv);

Disable
	clk_unprepare(...);
	for (i = 0; i < ADG_SSI_MAX; i++)
		clk_unprepare(...);

Thank you for your help !!

Best regards
---
Kuninori Morimoto