Re: [PATCH v3] dt-bindings: soundwire: Convert text bindings to DT Schema

On 20/10/2022 09:06, Srinivasa Rao Mandadapu wrote:

Convert soundwire text bindings to DT Schema format.

Update interrupt property items as per device tree, as it is not appropriately described in text file.

Signed-off-by: Srinivasa Rao Mandadapu quic_srivasam@quicinc.com Co-developed-by: Ratna Deepthi Kudaravalli quic_rkudarav@quicinc.com Signed-off-by: Ratna Deepthi Kudaravalli quic_rkudarav@quicinc.com

---

Thank you for your patch. There is something to discuss/improve.

-}; diff --git a/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml b/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml new file mode 100644 index 0000000..12cd3c1 --- /dev/null +++ b/Documentation/devicetree/bindings/soundwire/qcom,soundwire.yaml @@ -0,0 +1,184 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/soundwire/qcom,soundwire.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#

+title: Qualcomm SoundWire Controller

+maintainers:

• • Srinivas Kandagatla srinivas.kandagatla@linaro.org
• • Srinivasa Rao Mandadapu quic_srivasam@quicinc.com

+description:

• The Qualcomm SoundWire controller along with its board specific bus parameters.

+properties:

• compatible:
• enum:

•  - qcom,soundwire-v1.3.0
  

•  - qcom,soundwire-v1.5.0
  

•  - qcom,soundwire-v1.5.1
  

•  - qcom,soundwire-v1.6.0
  

• reg:
• maxItems: 1
• interrupts:
• items:

•  - description: specify the SoundWire controller core.
  

•  - description: specify the Soundwire controller wake IRQ.
  

The second property was optional. You need to test DTS before sending the bindings. Please run `make dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst for instructions).

IOW, you need here: minItems: 1

• interrupt-names:

minItems: 1

• items:

•  - const: wakeup
  

•  - const: core
  

That's definitely wrong order and should not pass any tests...

• clocks:
• items:

•  - description: iface clock
  

• clock-names:
• items:

•  - const: iface
  

• resets:
• items:

•  - description: SWR_AUDIO_CGCR RESET
  

• reset-names:
• items:

•  - const: swr_audio_cgcr
  

• '#sound-dai-cells':
• const: 1
• '#address-cells':
• const: 2
• '#size-cells':
• const: 0
• wakeup-source: true
• qcom,din-ports:
• $ref: /schemas/types.yaml#/definitions/uint32
• description: count of data in ports
• qcom,dout-ports:
• $ref: /schemas/types.yaml#/definitions/uint32
• description: count of data out ports
• qcom,ports-word-length:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: size of payload channel sample.
• maxItems: 5
• qcom,ports-sinterval-low:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: sample interval low of each data port.
• maxItems: 5
• qcom,ports-offset1:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: payload transport window offset1 of each data port.

You miss here some pieces of original description,

• maxItems: 5
• qcom,ports-offset2:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: payload transport window offset2 of each data port.

Ditto - and in other places.

• maxItems: 5
• qcom,ports-lane-control:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: identify which data lane the data port uses.
• maxItems: 5
• qcom,ports-block-pack-mode:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: indicate the block packing mode.
• maxItems: 5
• qcom,ports-hstart:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: identifying lowerst numbered coloum in SoundWire frame.
• maxItems: 5
• qcom,ports-hstop:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: identifying highest numbered coloum in SoundWire frame.
• maxItems: 5
• qcom,ports-block-group-count:
• $ref: /schemas/types.yaml#/definitions/uint8-array
• description: indicate how many sample intervals are combined into a payload.
• maxItems: 5
• codec@4:

It's too specific. What if different device uses different address?

You need patternProperties "^codec@[0-9a-f]+$"

• $ref: ../sound/qcom,wcd938x-sdw.yaml#

Full path, so /schemas/sound/qcom,wcd.....

Cannot we have other codecs attached? For example check SC7280 IDP, SM8250 MTP, RB5.

• description:

•  Qualcomm WCD9380/WCD9385 Codec is a standalone Hi-Fi audio codec IC.
  

•  It has RX and TX Soundwire slave devices. This bindings is for the
  

•  slave devices.
  

• type: object

+required:

• • compatible
• • reg
• • interrupts
• • clocks
• • clock-names
• • '#sound-dai-cells'
• • '#address-cells'
• • '#size-cells'
• • qcom,dout-ports
• • qcom,din-ports
• • qcom,ports-sinterval-low
• • qcom,ports-offset1
• • qcom,ports-offset2

+additionalProperties: false

+examples:

• • |
• #include <dt-bindings/interrupt-controller/arm-gic.h>
• #include <dt-bindings/interrupt-controller/irq.h>
• #include <dt-bindings/clock/qcom,lpassaudiocc-sc7280.h>
• soundwire@3210000 {

•    compatible = "qcom,soundwire-v1.6.0";
  

•    reg = <0x03210000 0x2000>;
  

•    interrupts = <GIC_SPI 155 IRQ_TYPE_LEVEL_HIGH>,
  

•                 <&pdc 130 IRQ_TYPE_LEVEL_HIGH>;
  

Missing interrupt names.

Best regards, Krzysztof