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

[Bui Duc Phuc]

Hi Morimoto-san,

Thank you for the review.

> 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 ?

I moved fsi_clk_init() below the two functions fsi_clk_set_rate_cpg
and fsi_clk_set_rate_external because, inside fsi_clk_init(),
I assign these functions to clock->set_rate. Moving the function was
necessary to avoid compilation errors.

+       if (is_cpg) {
+               xck = 0; ick = 1; div = 1;
+               clock->set_rate = fsi_clk_set_rate_cpg;
+       } else {
+               xck = 1; ick = 1; div = 0;
+               clock->set_rate = fsi_clk_set_rate_external;
+       }

Would you prefer that I use forward declarations instead of changing
the function order?

> 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.

Understood, I will fix the comment placement accordingly.

> > -     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() ?

At the probe stage, the Master/Slave status has not yet been determined
because it depends on a subsequent set_fmt() call. Therefore, I am not using
the fsi_is_clk_master() function inside the new fsi_clk_init() during
the probe process.

Instead, the new fsi_clk_init() function acquires all resources
(including the mandatory SPU clock) upfront using
devm_clk_get_optional().
The actual fsi_is_clk_master() check remains strictly enforced in
fsi_hw_startup() before enabling any functional clocks.

/* start master clock */
if (fsi_is_clk_master(fsi))
return fsi_clk_enable(dev, fsi);

> Why don't use fsi->clk_cpg ?

You're right, using fsi->clk_cpg is cleaner since it's already
initialized in fsi_port_info_init().
 I will use it in the next version.

> And why you need to call fsi_clk_init() twice ?
The FSI controller has two independent ports (Port A and Port B).
Each port requires its own clock resource initialization and configuration.

Best regards,
Phuc