Re: [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:

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.