On Mon, Aug 19, 2013 at 09:35:21AM +0100, Nicolin Chen wrote:
> This patch implements a device-tree-only CPU DAI driver for Freescale
> S/PDIF controller that supports stereo playback and record feature.
>
> Signed-off-by: Nicolin Chen <b42378@xxxxxxxxxxxxx>
> ---
> .../devicetree/bindings/sound/fsl,spdif.txt | 56 +
> sound/soc/fsl/Kconfig | 3 +
> sound/soc/fsl/Makefile | 2 +
> sound/soc/fsl/fsl_spdif.c | 1277 ++++++++++++++++++++
> sound/soc/fsl/fsl_spdif.h | 224 ++++
> 5 files changed, 1562 insertions(+), 0 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/sound/fsl,spdif.txt
> create mode 100644 sound/soc/fsl/fsl_spdif.c
> create mode 100644 sound/soc/fsl/fsl_spdif.h
>
> diff --git a/Documentation/devicetree/bindings/sound/fsl,spdif.txt b/Documentation/devicetree/bindings/sound/fsl,spdif.txt
> new file mode 100644
> index 0000000..e9caf1c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/fsl,spdif.txt
> @@ -0,0 +1,56 @@
> +Freescale Sony/Philips Digital Interface Format (S/PDIF) Controller
> +
> +The Freescale S/PDIF audio block is a stereo transceiver that allows the
> +processor to receive and transmit digital audio via an coaxial cable or
> +a fibre cable.
> +
> +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.
> +
> + - reg : Offset and length of the register set for the device.
> +
> + - interrupts : Contains spdif interrupt.
Is that the only interrupt the device generates?
> +
> + - dmas : Generic dma devicetree binding as described in
> + Documentation/devicetree/bindings/dma/dma.txt.
> +
> + - dma-names : Two dmas have to be defined, "tx" and "rx".
> +
> + - clocks : Contains an entry for each entry in clock-names.
> +
> + - clock-names : Includes the following entries:
> + name description
I don't think you need this line, it's obvious enough without it.
> + "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.
> +
> +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.
> + reg = <0x02004000 0x4000>;
> + interrupts = <0 52 0x04>;
> + dmas = <&sdma 14 18 0>,
> + <&sdma 15 18 0>;
> + dma-names = "rx", "tx";
> +
> + clocks = <&clks 197>, <&clks 3>,
> + <&clks 197>, <&clks 107>,
> + <&clks 0>, <&clks 118>,
> + <&clks 62>, <&clks 139>,
> + <&clks 0>;
> + clock-names = "core", "rxtx0",
> + "rxtx1", "rxtx2",
> + "rxtx3", "rxtx4",
> + "rxtx5", "rxtx6",
> + "rxtx7";
> +
> + status = "okay";
> +};
[...]
> +static int spdif_clk_set_rate(struct clk *clk, unsigned long rate)
> +{
> + unsigned long rate_actual;
> +
> + rate_actual = clk_round_rate(clk, rate);
> + clk_set_rate(clk, rate_actual);
> +
> + return 0;
> +}
Can't clk_set_rate fail?
[...]
> + /* Select clock source for rx/tx clock */
> + spdif_priv->rxclk = devm_clk_get(&pdev->dev, "rxtx1");
> + if (IS_ERR(spdif_priv->rxclk)) {
> + dev_err(&pdev->dev, "no rxtx1 property in devicetree\n");
Saying "no rxtx1 clock in devicetree" would be clearer.
[...]
> +static const struct of_device_id fsl_spdif_dt_ids[] = {
> + { .compatible = "fsl,imx35-spdif", },
> + {}
> +};
So "fsl,imx35-spdif" *must* be in the compatible list. The binding
should mention this.
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
