Re: [PATCH v1 1/2] dt-bindings: sound: add qcom,wsa885x-i2c

[Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>]

On 6/11/2026 3:04 PM, Krzysztof Kozlowski wrote:
> On Wed, Jun 10, 2026 at 09:27:07PM +0530, Prasad Kumpatla wrote:
>> Document the Qualcomm WSA885X I2C smart amplifier binding.
>>
>> Describe the required supplies, powerdown and interrupt GPIOs, the
>> optional battery configuration, and the optional init-table property
>> used to program the device during codec initialization.
>>
>> This matches the driver programming model and documents the DT data
> Binding matches hardware, not driver. Please describe the hardware.

Hi Krzysztof,

Thanks for reviewing the patch and for the feedback.

Ack, Will add more HW details in next version.

>
>> needed to use the codec on platforms with Audio IF playback.
>>
>> Signed-off-by: Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>
>> ---
>>   .../bindings/sound/qcom,wsa885x-i2c.yaml      | 89 +++++++++++++++++++
>>   1 file changed, 89 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/sound/qcom,wsa885x-i2c.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/sound/qcom,wsa885x-i2c.yaml b/Documentation/devicetree/bindings/sound/qcom,wsa885x-i2c.yaml
>> new file mode 100644
>> index 000000000..1069f470d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/qcom,wsa885x-i2c.yaml
> There is no I2C in device name.

Ack, Will remove.

>
>> @@ -0,0 +1,89 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sound/qcom,wsa885x-i2c.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm WSA885x I2C smart speaker amplifier
>> +
>> +maintainers:
>> +  - Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> +  - Prasad Kumpatla <prasad.kumpatla@oss.qualcomm.com>
>> +
>> +description: |
> Do not need '|' unless you need to preserve formatting.
Ack, Will Update.
>
>> +  WSA885x is a Qualcomm Aqstic smart speaker amplifier with an I2C control
>> +  interface and a digital audio interface exposed through ASoC DAI callbacks.
>> +
>> +allOf:
>> +  - $ref: dai-common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,wsa885x-i2c
> Same here
>
> Also, incorrect usage of wildcard. Look at other bindings how this is
> written, so you will not repeat the same comments:
> https://lore.kernel.org/all/20250522-rb2_audio_v3-v3-3-9eeb08cab9dc@linaro.org/
>
> Read writing bindings before posting next version.
>
> I also cannot find traces of internal review of this. Did it happen? Did
> you receive toolset comments?

Ack, Thanks for the reference link, will cross check and update the 
bindings.

No, there is o internal review done for this patch due to timelines.

>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#sound-dai-cells':
>> +    const: 0
>> +
>> +  powerdown-gpios:
>> +    description: GPIO controlling the SD_N powerdown pin.
>> +    maxItems: 1
>> +
>> +  interrupt-gpios:
> No, interrupts are never written as GPIOs.
>
> Where is this binding coming from?


Agree, Will remove this and come up standard interrupt bindings in next 
version.

>
>> +    description: GPIO used for the codec interrupt output.
>> +    maxItems: 1
>> +
>> +  vdd-1p8-supply: true
>> +
>> +  vdd-io-supply: true
>> +
>> +  qcom,battery-config:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description: Speaker battery configuration, 1 for 1S and 2 for 2S.
> Use string
Ack.
>
>> +    default: 1
>> +    enum: [1, 2]
>> +
>> +  qcom,wsa885x-init-table:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>> +    minItems: 2
>> +    maxItems: 256
>> +    description: |
>> +      Sequence of register/value pairs applied during codec hardware
> No, we don't store register values usually.


Ack,I'll move them into the driver as a register table,

making them easier to maintain and avoiding opaque DT data.

>> +      initialization. Entries are encoded as alternating register address and
>> +      register value cells. The number of entries must be even (register/value
>> +      pairs); maxItems is 256 (128 pairs).
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#sound-dai-cells'
>> +  - powerdown-gpios
>> +  - interrupt-gpios
>> +  - vdd-1p8-supply
>> +  - vdd-io-supply
>> +
>> +additionalProperties: false
> unevaluated instead. Again, OPEN other existing bindings. Why doing
> something completely different? Is there any WSA88xx binding with
> additionalProperties? No.

Thanks for pointing this out. I'll align the schema with the existing 
WSA88xx

bindings and replace additionalProperties: false with 
unevaluatedProperties: false

in the next revision.

Thanks,

Prasad

>
> Best regards,
> Krzysztof
>