Re: [PATCH v2 2/6] ASoC: renesas: fsi: Fix hang by enabling SPU clock

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

Hi

Hi

> Enable/disable the shared SPU clock in hw startup/shutdown. Without this,
> accessing FSI registers may hang the system.
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
(snip)
> @@ -1492,6 +1492,18 @@ static int fsi_hw_startup(struct fsi_priv *fsi,
>  			  struct device *dev)
>  {
>  	u32 data = 0;
> +	int ret = 0;
> +	/* enable spu clock */
> +	mutex_lock(&fsi->master->clk_lock);
> +	if (fsi->master->clk_spu && fsi->master->spu_count++ == 0) {
> +		ret = clk_prepare_enable(fsi->master->clk_spu);
> +		if (ret < 0) {
> +			fsi->master->spu_count--;
> +			mutex_unlock(&fsi->master->clk_lock);
> +			return ret;
> +		}
> +	}
> +	mutex_unlock(&fsi->master->clk_lock);

1st, please insert white line between "int ret = 0;" and "/* enable spu
clock */".

2nd, besically, FSI already has "lock", and using it for several protecting.
Please re-use it, and don't add random new-lock. It makes code confusable.
Then, please use guard().

3rd, I don't like above count inc/dec, and mutex_unlock() style, because
the code unnecessarily complicated. It can be...

	int ret = 0;

	if (master->clk_spu) {
		guard(spinlock_irqsave)(&master->lock);

		if (master->spu_count == 0)
			ret = clk_prepare_enable(master->clk_spu);

		master->spu_count++;
	}
	if (ret < 0)
		return ret;

I'm not 100% sure, but I guess you need to count up spu_count anyway
regardless of clk_prepare_enable() result ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto