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

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@freescale.com

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