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

[Nicolin Chen <b42378@freescale.com>]

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?

> > +  - interrupts : Contains spdif interrupt.
> 
> Is that the only interrupt the device generates?

Yes, how could I improve this description?


> > +       "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.

> 
> > +
> > +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?


Thank you,
Nicolin