Re: [PATCH v2 4/6] ASoC: renesas: fsi: refactor clock initialization

[Kuninori Morimoto]

Hi

> Move fsi_clk_init() out of set_fmt() and handle clock master logic
> internally. This simplifies the flow and aligns with probe-time
> initialization.
> 
> Suggested-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
> Signed-off-by: bui duc phuc <phucduc.bui@xxxxxxxxx>
> ---
(snip)
> -/*
> - *		clock function
> - */
> -static int fsi_clk_init(struct device *dev,
> -			struct fsi_priv *fsi,
> -			int xck,
> -			int ick,
> -			int div,
> -			int (*set_rate)(struct device *dev,
> -					struct fsi_priv *fsi))

I have mentioned in previous mail to just move fsi_clk_init(), but why do
you need to move it ? It works without any issue without moving function,
I guess ?
This patch 1) moves fsi_clk_init() 2) and update it. So there are too many
diffs. Difficult to review.

Note is that the comment /* clock function */ is not only for fsi_clk_init()
but for all fsi_clk_xxx() functions. Here is that position.


> @@ -1684,15 +1696,6 @@ static int fsi_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
>  		break;
>  	}
>  
> -	if (fsi_is_clk_master(fsi)) {
> -		if (fsi->clk_cpg)
> -			fsi_clk_init(dai->dev, fsi, 0, 1, 1,
> -				     fsi_clk_set_rate_cpg);
> -		else
> -			fsi_clk_init(dai->dev, fsi, 1, 1, 0,
> -				     fsi_clk_set_rate_external);
> -	}

You removes fsi_is_clk_master() check in new fsi_clk_init() ?

> @@ -1992,6 +1995,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->master	= master;
>  	fsi_port_info_init(fsi, &info.port_a);
>  	fsi_handler_init(fsi, &info.port_a);
> +	fsi_clk_init(&pdev->dev, fsi, !!(info.port_a.flags & SH_FSI_CLK_CPG));
>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIA stream probe failed\n");
> @@ -2005,6 +2009,7 @@ static int fsi_probe(struct platform_device *pdev)
>  	fsi->master	= master;
>  	fsi_port_info_init(fsi, &info.port_b);
>  	fsi_handler_init(fsi, &info.port_b);
> +	fsi_clk_init(&pdev->dev, fsi, !!(info.port_b.flags & SH_FSI_CLK_CPG));
>  	ret = fsi_stream_probe(fsi, &pdev->dev);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "FSIB stream probe failed\n");

Why don't use fsi->clk_cpg ? And why you need to call fsi_clk_init() twice ?

Thank you for your help !!

Best regards
---
Kuninori Morimoto