On Wed, Nov 19, 2014 at 10:52:42AM -0800, Kenneth Westfield wrote:
> +* Qualcomm Technologies IPQ806x SoundCard
> +
> +This node models the Qualcomm Technologies IPQ806x LPASS Audio SoundCard,
> +with a connection between the CPU MI2S DAI and the external DAC.
> +Required properties:
> +- compatible : "qcom,ipq806x-snd-card"
> +- qcom,model : The user-visible name of this sound card
> +- pinctrl-0 : The default state of the MI2S pins
> +- pinctrl-names : The name of the default state
Why is a sound card doing pin control? I would expect that the
component devices would do their own pin control. Also if you have
named pin control states the set of valid names should be specified.
> +- dac-gpios : GPIO specifier to the GPIO -> DAC SDMODE pin
Simiarly why is a sound card controlling the DAC GPIOs, is this not part
of the CODEC?
> +- clocks : A list of clock specifiers in the following order:
> + * AHBIX bus clock
> +- clock-names : A list of names in the following order:
> + * ahbix_clk
Again I'd really expect any devices on the AHB to be controlling the AHB
related clocks rather than a sound card doing it.
> +asoc-platform : This is phandle list containing the references to platform device
> + nodes that are used as part of the sound card dai-links.
> +asoc-platform-names : This property contains list of platform names. The order of
> + the platform names should match to that of the phandle order
> + given in "asoc-platform".
The device tree bindings should be OS neutral but ASoC is a Linux thing.
This needs to be written in terms of the hardware it's describing.
> +Required properties:
> +- compatible : "qcom,lpass-cpu-dai"
> +- clocks : A list of clock specifiers in the following order:
> + * MI2S OSR clock
> + * MI2S Bit clock
> +- clock-names : A list of names in the following order:
> + * mi2s_osr_clk
> + * mi2s_bit_clk
If there are names (which is good) why is the ordering important? The
whole point in having a mandatory list of names is to remove the
ordering and completeness requirements.
> +Required properties:
> +- compatible : "qcom,lpass-lpaif"
> +- reg : Address space for the LPASS subsystem registers
> +- reg-names : The name of the LPASS subsystem register address space
> +- interrupts : Phandle to the LPASS interrupt
> +- interrupt-names : The names of the LPASS interrupt
Again you need to document the valid names.
> index 0000000000000000000000000000000000000000..d2ff501d44f7b7aa790cdadc8ba75c6a8bf37ccd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/qcom,lpass-pcm-mi2s.txt
> @@ -0,0 +1,12 @@
> +* Qualcomm Technologies IPQ806x PCM audio interface
> +
> +This node models the Qualcomm Technologies IPQ806x PCM audio interface.
> +
> +Required properties:
> +- compatible: "qcom,lpass-pcm-mi2s"
> +
> +Example:
> +
> +lpass-pcm-mi2s {
> + compatible = "qcom,lpass-pcm-mi2s";
> +};
This doesn't appear to describe hardware - there are no register
addresses or anything. I'd guess this is most likely part of another
hardware block and should be handled by the driver for that device.
Attachment:
signature.asc
Description: Digital signature
