diff mbox

[V4,03/10] ASoC: qcom: Document LPASS CPU bindings

Message ID 1423169626-22166-4-git-send-email-kwestfie@codeaurora.org (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Kenneth Westfield Feb. 5, 2015, 8:53 p.m. UTC
From: Kenneth Westfield <kwestfie@codeaurora.org>

Add documentation to the sound directory of the
device-tree bindings for the IPQ806x LPASS CPU DAI
device.

Signed-off-by: Kenneth Westfield <kwestfie@codeaurora.org>
Acked-by: Banajit Goswami <bgoswami@codeaurora.org>
---
 .../devicetree/bindings/sound/qcom,lpass-cpu.txt   | 66 ++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt

Comments

Mark Brown Feb. 6, 2015, 10:18 p.m. UTC | #1
On Thu, Feb 05, 2015 at 12:53:39PM -0800, Kenneth Westfield wrote:

> +- qcom,system-clock-shift		: Add this bool property if the default
> +					  frequency of the system clock needs to
> +					  be reduced.
> +- qcom,system-clock-shift-compare	: A numerical value used to right-shift
> +					  the default system clock frequency for
> +					  comparison with the target bit clock
> +					  frequency.
> +- qcom,system-clock-shift-amount	: A numerical value used to right-shift
> +					  the default system clock frequency.
> +- qcom,alternate-sysclk			: Add this bool property if the default
> +					  frequency of the system clock cannot
> +					  divide down to the target bit clock
> +					  frequency.
> +- qcom,alternate-sysclk-bitwidth	: A numerical value representing the
> +					  sample bitwidth which requires use of
> +					  the alternate system clock frequency.
> +- qcom,alternate-sysclk-frequency	: A numerical value representing the new
> +					  system clock frequency to use.

None of these seem like they are appropriate for device tree properties,
they appear to be choosing a specific clocking configuration which is
something that would normally be done as part of the system integration
in the machine driver rather than in the DAI driver.  This binding won't
work in cases where the clocks are being changed at runtime and would
limit systems where that becomes possible in future.

Further, the interface seems too low level - it's specifying individual
dividers and so on which would normally be things that can trivially be
calculated or inferred given the input and target clock rates.
Kenneth Westfield Feb. 9, 2015, 6:38 a.m. UTC | #2
On Sat, Feb 07, 2015 at 06:18:23AM +0800, Mark Brown wrote:
> On Thu, Feb 05, 2015 at 12:53:39PM -0800, Kenneth Westfield wrote:
> 
> > +- qcom,system-clock-shift		: Add this bool property if the default
> > +					  frequency of the system clock needs to
> > +					  be reduced.
> > +- qcom,system-clock-shift-compare	: A numerical value used to right-shift
> > +					  the default system clock frequency for
> > +					  comparison with the target bit clock
> > +					  frequency.
> > +- qcom,system-clock-shift-amount	: A numerical value used to right-shift
> > +					  the default system clock frequency.
> > +- qcom,alternate-sysclk			: Add this bool property if the default
> > +					  frequency of the system clock cannot
> > +					  divide down to the target bit clock
> > +					  frequency.
> > +- qcom,alternate-sysclk-bitwidth	: A numerical value representing the
> > +					  sample bitwidth which requires use of
> > +					  the alternate system clock frequency.
> > +- qcom,alternate-sysclk-frequency	: A numerical value representing the new
> > +					  system clock frequency to use.
> 
> None of these seem like they are appropriate for device tree properties,
> they appear to be choosing a specific clocking configuration which is
> something that would normally be done as part of the system integration
> in the machine driver rather than in the DAI driver.  This binding won't
> work in cases where the clocks are being changed at runtime and would
> limit systems where that becomes possible in future.

So I add a machine driver that selects the clocking freq in hw_params
and calls set_sysclk in the DAIs.

The DT node for the machine driver would look something like:
        default_system_clock_frequency = < xxxxxx >;
        alternate_system_clock_frequency = < xxxxxx >;
        cpu_dai = < &cpu >;
        codec_dai = < &codec >;
        pinctrl... ?

Does this sound ok?  Also, would it make sense to move the pinctrl back
to the machine driver?
Mark Brown Feb. 9, 2015, 8:08 a.m. UTC | #3
On Sun, Feb 08, 2015 at 10:38:23PM -0800, Kenneth Westfield wrote:

> So I add a machine driver that selects the clocking freq in hw_params
> and calls set_sysclk in the DAIs.

> The DT node for the machine driver would look something like:
>         default_system_clock_frequency = < xxxxxx >;
>         alternate_system_clock_frequency = < xxxxxx >;
>         cpu_dai = < &cpu >;
>         codec_dai = < &codec >;
>         pinctrl... ?

Why are the system clock frequencies being specified in the DT at all -
can't we either figure out the constraints from something else or just
set the rates to something sensible that the driver knows (allowing for
improvements in the driver in the future).

> Does this sound ok?  Also, would it make sense to move the pinctrl back
> to the machine driver?

Why would we want to do that?
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt b/Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt
new file mode 100644
index 0000000000000000000000000000000000000000..7406ae52aec196f136883eb01afbc6c425bdc465
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/qcom,lpass-cpu.txt
@@ -0,0 +1,66 @@ 
+* Qualcomm Technologies LPASS CPU DAI
+
+This node models the Qualcomm Technologies LPASS DAI ports.
+
+Required properties:
+
+- compatible		: "qcom,lpass-cpu"
+- clocks		: Must contain an entry for each entry in clock-names.
+- clock-names		: A list which must include the following entries:
+				* "ahbix-clk"
+				* "mi2s-osr-clk"
+				* "mi2s-bit-clk"
+- interrupts		: Must contain an entry for each entry in
+			  interrupt-names.
+- interrupt-names	: A list which must include the following entries:
+				* "lpass-irq-lpaif"
+- pinctrl-N		: One property must exist for each entry in
+			  pinctrl-names.  See ../pinctrl/pinctrl-bindings.txt
+			  for details of the property values.
+- pinctrl-names		: Must contain a "default" entry.
+- reg			: Must contain an address for each entry in reg-names.
+- reg-names		: A list which must include the following entries:
+				* "lpass-lpaif"
+				* "lpass-lpm"
+
+Optional properties:
+
+- qcom,system-clock-shift		: Add this bool property if the default
+					  frequency of the system clock needs to
+					  be reduced.
+- qcom,system-clock-shift-compare	: A numerical value used to right-shift
+					  the default system clock frequency for
+					  comparison with the target bit clock
+					  frequency.
+- qcom,system-clock-shift-amount	: A numerical value used to right-shift
+					  the default system clock frequency.
+- qcom,alternate-sysclk			: Add this bool property if the default
+					  frequency of the system clock cannot
+					  divide down to the target bit clock
+					  frequency.
+- qcom,alternate-sysclk-bitwidth	: A numerical value representing the
+					  sample bitwidth which requires use of
+					  the alternate system clock frequency.
+- qcom,alternate-sysclk-frequency	: A numerical value representing the new
+					  system clock frequency to use.
+
+Example:
+
+lpass-cpu@28100000 {
+	compatible = "qcom,lpass-cpu";
+	clocks = <&lcc AHBIX_CLK>, <&lcc MI2S_OSR_CLK>, <&lcc MI2S_BIT_CLK>;
+	clock-names = "ahbix-clk", "mi2s-osr-clk", "mi2s-bit-clk";
+	interrupts = <0 85 1>;
+	interrupt-names = "lpass-irq-lpaif";
+	pinctrl-names = "default", "idle";
+	pinctrl-0 = <&mi2s_default>;
+	pinctrl-1 = <&mi2s_idle>;
+	reg = <0x28100000 0x10000>, <0x28400000 0x4000>;
+	reg-names = "lpass-lpaif", "lpass-lpm";
+	qcom,system-clock-shift;
+	qcom,system-clock-shift-compare = <4>;
+	qcom,system-clock-shift-amount = <3>;
+	qcom,alternate-sysclk;
+	qcom,alternate-systclk-bitwidth = <24>;
+	qcom,alternate-systclk-frequency = <4608000>;
+};