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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3369 bytes --]

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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]