Re: [PATCH 2/9] ASoC: qcom: Add device tree binding docs

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.

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.