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

On Mon, Aug 19, 2013 at 10:34:24AM +0100, Nicolin Chen wrote:

Hi Mark,

   Thank you for the commenst. I'll Fix them in v8.

   Here are some remaining question:

On Mon, Aug 19, 2013 at 10:18:09AM +0100, Mark Rutland wrote:

quoted

quoted

+Required properties:
+
+  - compatible : Compatible list, contains "fsl,<chip>-spdif".

What are valid values for <chip>? The binding should mention this. There
are bindings that don't, but they need to be fixed. Undocumented ABIs
are a bad idea.

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

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.

quoted

quoted

+       "core"          The core clock of spdif controller
+       "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.

Could you elaborate on the last sentence? I'm not sure exactly what you
meant.

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?

quoted

quoted

+
+Example:
+
+spdif: spdif@02004000 {
+       compatible = "fsl,imx6q-spdif",
+               "fsl,imx35-spdif";

Is "fsl,imx35-spdif" necessary in the list, or is it not the case all
"fsl,<chip>-spdif" variants are compatible with it?

That should be mentioned along with the list of valid compatible
strings.

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.

Cheers,
Mark.