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:
+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).
- 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.
"core" The core clock of spdif controller"rxtx<0-7>" Clock source list for tx and rx clock.This clock list should be identical tothe source list connecting to the spdifclock mux in "SPDIF Transceiver ClockDiagram" of SoC reference manual. Itcan also be referred to TxClk_Sourcebit 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?
+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.