Re: [PATCH 1/3] dt-bindings: sound: add bindings for pm4125 audio codec

["Alexey Klimov" <alexey.klimov@linaro.org>]

On Thu Jun 26, 2025 at 7:13 AM BST, Krzysztof Kozlowski wrote:
> On Thu, Jun 26, 2025 at 12:50:29AM +0100, Alexey Klimov wrote:
>> The audio codec IC is found on Qualcomm PM4125/PM2250 PMIC.
>> It has TX and RX soundwire slave devices hence two files
>> are added.
>> 
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  .../bindings/sound/qcom,pm4125-codec.yaml          | 147 +++++++++++++++++++++
>>  .../devicetree/bindings/sound/qcom,pm4125-sdw.yaml |  86 ++++++++++++
>>  2 files changed, 233 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml b/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..1b6ce8d4397b4c1c048899bd2cc4d02318cc46c9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml

[..]

>> +  '#sound-dai-cells':
>> +    const: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - vdd-io-supply
>> +  - vdd-cp-supply
>> +  - vdd-mic-bias-supply
>> +  - vdd-pa-vpos-supply
>> +  - qcom,tx-device
>> +  - qcom,rx-device
>> +  - qcom,micbias1-microvolt
>> +  - qcom,micbias2-microvolt
>> +  - qcom,micbias3-microvolt
>> +  - "#sound-dai-cells"
>
> Keep consistent quotes, either ' or "
>
>> +
>> +additionalProperties: false
>
> This has to unevaluatedProperties

Ok for both points, I'll change it.

>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/spmi/spmi.h>
>> +
>> +    spmi {
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +
>> +        pmic@0 {
>
> pmic {
>
>> +            compatible = "qcom,pm8916", "qcom,spmi-pmic";
>
> Drop, you have warnings here.
>
>> +            reg = <0x0 SPMI_USID>;
>
> Drop

Ok to points above, I'll remove it.

>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            audio-codec@f000 {
>> +                compatible = "qcom,pm4125-codec";
>> +                reg = <0xf000>;
>> +                vdd-io-supply = <&pm4125_l15>;
>> +                vdd-cp-supply = <&pm4125_s4>;
>> +                vdd-pa-vpos-supply = <&pm4125_s4>;
>> +                vdd-mic-bias-supply = <&pm4125_l22>;
>> +                qcom,micbias1-microvolt = <1800000>;
>> +                qcom,micbias2-microvolt = <1800000>;
>> +                qcom,micbias3-microvolt = <1800000>;
>> +                qcom,rx-device = <&pm4125_rx>;
>> +                qcom,tx-device = <&pm4125_tx>;
>> +                #sound-dai-cells = <1>;
>> +            };
>> +        };
>> +    };
>> +
>> +    /* ... */
>> +
>> +    soundwire@a610000 {
>
> Drop this and next one.

The audio-codec node supposed to have qcom,{rx,tx}-device properties.
If I'll drop it then the example doesn't compile well unless I am missing
something?

For example when I removed soundwire tx node completely and dropped
qcom,tx-device then:

Documentation/devicetree/bindings/sound/qcom,pm4125-codec.example.dtb: audio-codec@f000 (qcom,pm4125-codec): 'qcom,tx-device' is a required property
	from schema $id: http://devicetree.org/schemas/sound/qcom,pm4125-codec.yaml#

Or should it be qcom,tx-device = <&null_placeholder_something> ?
I can't find any examples.

I guess I can drop example from the other file.

>> +        reg = <0x0a610000 0x2000>;
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +        pm4125_rx: audio-codec@0,4 {
>> +            compatible = "sdw20217010c00";
>> +            reg = <0 4>;
>> +            qcom,rx-port-mapping = <1 3>;
>> +        };
>> +    };
>> +
>> +    soundwire@a740000 {
>> +        reg = <0x0a740000 0x2000>;
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +        pm4125_tx: audio-codec@0,3 {
>> +            compatible = "sdw20217010c00";
>> +            reg = <0 3>;
>> +            qcom,tx-port-mapping = <1 1>;
>> +        };
>> +    };
>> +...
>> diff --git a/Documentation/devicetree/bindings/sound/qcom,pm4125-sdw.yaml b/Documentation/devicetree/bindings/sound/qcom,pm4125-sdw.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..7241d2ab5dcf4a0d5f25a75acb33a335f93d3b5e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/qcom,pm4125-sdw.yaml
>> @@ -0,0 +1,86 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sound/qcom,pm4125-sdw.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SoundWire Slave devices on PM4125/PM2250 PMIC audio codec.
>> +
>> +maintainers:
>> +  - Alexey Klimov <alexey.klimov@linaro.org>
>> +
>> +description: |
>
> Drop |

Ack.

>> +  The audio codec IC found on Qualcomm PM4125/PM2250 PMICs.
>> +  It has RX and TX Soundwire slave devices. This bindings is for the
>> +  slave devices.
>
> Last sentence is redundant and makes no sense. Codec has only slave
> devices, so how this can be anything else than for slave devices?

This came from other similar files that describe bindings for child codec nodes
of soundwire nodes. For example from qcom,wcd937x-sdw.yaml.
Should this be rephrased to "This bindings is for the soundwire slave devices." ?

>> +
>> +properties:
>> +  compatible:
>> +    const: sdw20217010c00
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  qcom,tx-port-mapping:
>> +    description: |
>> +      Specifies static port mapping between device and host tx ports.
>> +      In the order of the device port index which are adc1_port, adc23_port,
>> +      dmic03_mbhc_port, dmic46_port.
>> +      Supports maximum 2 tx soundwire ports.
>> +
>> +      PM4125 TX Port 1 (ADC1,2 & DMIC0 & MBHC)    <=> SWR0 Port 1
>> +      PM4125 TX Port 2 (ADC1 & DMIC0,1,2 & MBHC)  <=> SWR0 Port 2
>> +
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 2
>> +    maxItems: 2
>> +    items:
>> +      enum: [1, 2, 3, 4]
>> +
>> +  qcom,rx-port-mapping:
>> +    description: |
>> +      Specifies static port mapping between device and host rx ports.
>> +      In the order of device port index which are hph_port, clsh_port,
>> +      comp_port, lo_port, dsd port.
>> +      Supports maximum 2 rx soundwire ports.
>> +
>> +      PM4125 RX Port 1 (HPH_L/R)       <==>    SWR1 Port 1 (HPH_L/R)
>> +      PM4125 RX Port 2 (COMP_L/R)      <==>    SWR1 Port 3 (COMP_L/R)
>> +
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 2
>> +    maxItems: 2
>> +    items:
>> +      enum: [1, 2, 3, 4, 5]
>> +
>> +required:
>> +  - compatible
>> +  - reg
>
> rx and tx are excluding, so this should be here encoded.

Ok, I think I found a way to change it.

>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    soundwire@a610000 {
>> +        reg = <0x0a610000 0x2000>;
>> +        #address-cells = <2>;
>> +        #size-cells = <0>;
>> +        pm4125_rx: codec@0,1 {
>> +            compatible = "sdw20217010c00";
>> +            reg = <0 1>;
>> +            qcom,rx-port-mapping = <1 3>;
>> +        };
>> +    };
>> +
>> +    soundwire@a740000 {
>> +        reg = <0x0a740000 0x2000>;
>
> One example is enough, they are the same.

Ok, I'll drop it (see my comment above as well).

Thanks,
Alexey