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

On Mon, Aug 19, 2013 at 11:13:04AM +0100, Nicolin Chen wrote:

On Mon, Aug 19, 2013 at 10:54:33AM +0100, Mark Rutland wrote:

quoted

quoted

I see, so 'Compatible list, must contains "fsl,imx35-spdif"' would be okay?

While that needs to be mentioned, other values which might be present
(e.g. "fsl,imx6q-spdif") must be mentioned, or we can't rely on them
if we want to use them in future from drivers to provide additional
information (and thus they're useless).

Well, imx6q-spdif is identical to the imx35-spdif. So I think the
'must contains 35' would be enough for the current case. If any
future modification happens to the list, I can update this doc later.

quoted

quoted

quoted

quoted

+  - interrupts : Contains spdif interrupt.

Is that the only interrupt the device generates?

Yes, how could I improve this description?

It's probably not possible to make it much clearar to be honest,
"Contains the sole interrupt generated by the device" might be a little
overkill.

Then I keep it no-change :)

Ok :)

quoted

quoted

quoted

quoted

+       "rxtx<0-7>"     Clock source list for tx and rx clock.
+                       This clock list should be identical to
+                       the source list connecting to the spdif
+                       clock mux in "SPDIF Transceiver Clock
+                       Diagram" of SoC reference manual. It
+                       can also be referred to TxClk_Source
+                       bit of register SPDIF_STC.

quoted

quoted

The list is also identical to the TxClk_Source bit value list of
register SPDIF_STC.

What I don't understand is how the value of the SPDIF_STC register
within the spdif IP block can describe the necessary details of clocks
coming from an external clock provider.

What do the TxClk_Source bits represent? The configuration of the clock
inputs on the spdif IP block, or the outputs on some clock provider? Are
they writeable, or configured at integration time? If the clock provider
were replaced with another arbitrary clock provider, what would they
represent?

Actually there's a clock mux for TxClk and it's connecting with 8 clock 
sources. So the TxClk_Source bit show the connection between its value
with the correspond source on the clock mux:

TxClk_Source	000 XTAL clk input
		001 CCM spdif0_clk_root input
		010 asrc_clk input
		011 spdif_extclk input, from pads
		100 esai_hckt input
		101 frequency divided ipg_clk input
		110 mlb_clk input
		111 mlb phy clk input

Ah. So are these the actual input names on the mux, or the names of the
outputs that are wired up to the mux? If the former, these may be better
clock-names than rxtx<0-7>.

Is there a similar Rx mux?

Given the "rxtx<0-7>" names you've given these clocks, are there 8 clock
inputs that get duplicated within the spdif block and fed into both
muxes, or are there 16 external inputs that happen to be two groups of 8
identical sets of clocks in systems so far?

quoted

quoted

I guess it's better to drop the 'imx6q-spdif' here?

That depends:

* If the two IP blocks are identical, only the "imx35-spdif" name is
  necessary, and we can forget about "fsl,imx6q-spdif".

* If "fsl,imx6q-spdif" is a strict superset of "fsl,imx35-spdif", having
  both names documented and in a compatible list for a "fsl,imx6q-spdif"
  device makes sense.

* If "fsl,imx6q-spdif" is a variation of "fsl,imx35-spdif", and the
  "fsl,imx6q-spdif" cannot always be treated identically to a
  "fsl,imx35-spdif", then it makes sense to have separate compatible
  strings, with a device being listed as either "fsl,imx6q-spdif" or
  "fsl,imx35-spdif".

I don't know enough about the hardware to make that judgement call.

Thank you for explaining! I will choose A, because they are internally
identical except their external clock sources.

Ok, that sounds like the right course of action to me.

Thanks,
Mark.