Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver

[Nicolin Chen <b42378@freescale.com>]

Hi Tomasz,
   
   Thank you for the comments. I'll revise them in v6.
   And below is my reply for you comments.

On Thu, Aug 15, 2013 at 02:18:22PM +0200, Tomasz Figa wrote:
> > +  - clock-names : Includes the following entries:
> > +	name		type		comments
> > +	"core"		Required	The core clock of spdif controller
> > +
> > +	"rx"		Optional	Rx clock source for spdif record.
> > +					If absent, will use core clock.
> > +
> > +	"tx"		Optional	Tx clock source for spdif playback.
> > +					If absent, will use core clock.
> > +
> > +	"tx-32000"	Optional	Tx clock source for 32000Hz sample rate
> > +					playback. If absent, will use tx clock.
> > +
> > +	"tx-44100"	Optional	Tx clock source for 44100Hz sample rate
> > +					playback. If absent, will use tx clock.
> > +
> > +	"tx-48000"	Optional	Tx clock source for 48000Hz sample rate
> > +					playback. If absent, will use tx clock.
> > +
> > +	"src<0-7>"	Optional	Clock source list for tx and rx clock
> > +					to look up their clock source indexes.
> > +					This clock list should be identical to
> > +					the list of TxClk_Source bit value of
> > +					register SPDIF_STC. If absent or failed
> > +					to look up, tx and rx clock would then
> > +					ignore the "rx", "tx" "tx-32000",
> > +					"tx-44100", "tx-48000" clock phandles
> > +					and select the core clock as default
> > +					tx and rx clock.
> 
> I suspect a little abuse of clocks property here. From the description of
> "core" and "src<0-7>" clocks I assume that the IP can have up to 9 clock
> inputs - core clock and up to 8 extra source clocks. Is it correct?
> 
> If yes, this makes the "tx", "rx" and "tx-*" clocks describe
> configuration, not hardware. IMHO it should be up to the driver which
> source clocks to use for tx and rx channels and for each sampling rate.

First, you are right that all the properties you just commented are
software configurations. And I got the point that device tree now
can't allow any software configuration even if the actual hardware
connection will depend on it.

If so, I would like to remove those abused clocks and also drop the 
unused clocks in src<0-7>, then just remain those needed clocks src.
I think that can be plausible because there'll be no more clock abuse
and the driver will be able to get the source index from the name 
'src<num>'.

And you are right about the 9 clock inputs, just there're not only 9
inputs but also an extra external clock from S/PDIF transmitter via
coaxial cable or optical fiber -- RxCLK. Please check the following
list:

0000 if (DPLL Locked) SPDIF_RxClk else extal
0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk
0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk
0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt
0101 extal_clk
0110 spdif_clk
0111 asrc_clk
1000 spdif_extclk
1001 esai_hckt
1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk
1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk
1100 mkb_clk
1101 mlb_phy_clk

When (DPLL Locked) condition matches, the rx clock can ignore the 8 input
clocks from clock mux then use the external one from a S/PDIF transmitter.

So for the below part:

> > +Optional properties:
> > +
> > +  - rx-clksrc-lock: This is a boolean property. If present, ClkSrc_Sel
> > bit +  of SPDIF_SRPC would be set a clock source that cares DPLL locked
> > condition. +
> 
> This again looks like software configuration, not hardware description.
> Could you elaborate a bit more on meaning of this property?

I think the rx-clksrc-lock property should be included in DT as well, since
it's exactly a available clock source for rx. But I guess I just need to 
figure out a better way or a more elaborated description.

Thank you,
Nicolin Chen