Re: [alsa-devel] [RFC PATCH 5/6] dt-bindings: soundwire: add bindings for Qcom controller

From: Pierre-Louis Bossart
Date: Mon Jun 10 2019 - 10:03:11 EST

diff --git a/Documentation/devicetree/bindings/soundwire/qcom,swr.txt b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
new file mode 100644
index 000000000000..eb84d0f4f36f
--- /dev/null
+++ b/Documentation/devicetree/bindings/soundwire/qcom,swr.txt
@@ -0,0 +1,62 @@
+Qualcomm SoundWire Controller
+This binding describes the Qualcomm SoundWire Controller Bindings.
+Required properties:
+- compatible: Must be "qcom,soundwire-v<MAJOR>.<MINOR>.<STEP>",
+ example:
+ "qcom,soundwire-v1.3.0"
+ "qcom,soundwire-v1.5.0"
+ "qcom,soundwire-v1.6.0"
+- reg: SoundWire controller address space.
+- interrupts: SoundWire controller interrupt.
+- clock-names: Must contain "iface".
+- clocks: Interface clocks needed for controller.
+- #sound-dai-cells: Must be 1 for digital audio interfaces on the controllers.
+- #address-cells: Must be 1 for SoundWire devices;
+- #size-cells: Must be <0> as SoundWire addresses have no size component.
+- qcom,dout-ports: Must be count of data out ports
+- qcom,din-ports: Must be count of data in ports

On these I think we should have specified dpn properties as specified in
DisCo, but then looking at spec we do not define that for master, but
bus seems to have it.

Pierre do you why master does not have dpn properties in DisCo?

Because there are no DP0 or DPn registers defined for Masters in the SoundWire 1.x spec. DisCo is about specifying properties for standard registers, when they are not standard vendor extensions need to come into play.

+- qcom,ports-offset1: Must be frame offset1 of each data port.
+ Out followed by In. Used for Block size calculation.
+- qcom,ports-offset2: Must be frame offset2 of each data port.
+ Out followed by In. Used for Block size calculation.
+- qcom,ports-sinterval-low: Must be sample interval low of each data port.
+ Out followed by In. Used for Sample Interval calculation.

These are software so do not belong here

Not necessarily. They define the allocation expected on that link and I see no problem specifying those values here. It's the moral equivalent of specifying which TDM slots and the bit depth of one slot you'd use for DSP_A/B.
And if you push back, then what would be your alternate proposal on where those values might be stored? This is a very specific usage of the link and it makes sense to me to have the information in firmware - exposed with properties - rather than hard-coded in a pretend bus allocation routine in the kernel. Either the allocation is fully dynamic and it's handled by the kernel or it's static and it's better to store it in firmware to deal with platform-to-platform variations.