Re: [PATCH v3 03/12] ASoC: dt-bindings: qcom,wsa881x: extend description to analog mode

[Krzysztof Kozlowski <krzk@kernel.org>]

On 27/05/2025 22:34, Alexey Klimov wrote:
> On Thu May 22, 2025 at 6:45 PM BST, Krzysztof Kozlowski wrote:
>> On 22/05/2025 19:40, Alexey Klimov wrote:
>>> WSA881X also supports analog mode when device is configured via i2c
>>> only. Document it, add properties, new compatibles and example.
>>>
>>> Cc: Srinivas Kandagatla <srini@kernel.org>
>>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>>> ---
>>>  .../devicetree/bindings/sound/qcom,wsa881x.yaml    | 66 +++++++++++++++++++---
>>>  1 file changed, 58 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml b/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml
>>> index ac03672ebf6de1df862ce282f955ac91bdd9167d..a33e2754ec6159dbcaf5b6fcacf89eb2a6056899 100644
>>> --- a/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/qcom,wsa881x.yaml
>>> @@ -12,15 +12,17 @@ maintainers:
>>>  description: |
>>>    WSA8810 is a class-D smart speaker amplifier and WSA8815
>>>    is a high-output power class-D smart speaker amplifier.
>>> -  Their primary operating mode uses a SoundWire digital audio
>>> -  interface. This binding is for SoundWire interface.
>>> -
>>> -allOf:
>>> -  - $ref: dai-common.yaml#
>>> +  This family of amplifiers support two operating modes:
>>> +  SoundWire digital audio interface which is a primary mode
>>> +  and analog mode when device is configured via i2c only.
>>> +  This binding describes both modes.
>>>  
>>>  properties:
>>>    compatible:
>>> -    const: sdw10217201000
>>> +    enum:
>>> +      - qcom,wsa8810
>>> +      - qcom,wsa8815
>>> +      - sdw10217201000
>>
>> You never responded to my comments, never implemented them. Same problem
>> as before.
> 
> You don't respond to emails sometimes and, while I want to move this forward,
> I am not taking any chances replying to few months old thread, so if it okay
> I'll respond here. Sorry for doing this.
> 
> Previous comment:
> 
>> You implement only one compatible, so does it mean they are compatible?
>> If so, make them compatible.
> 
> There are two compatibles in wsa881x-i2c.c.
> By looking at downstream sources and current code I think there is no diff
> between wsa8810 and wsa8815 and it is handled by reading hw registers if
> needed. So I am thinking that maybe it makes sense to reduce it to
> "qcom,wsa881x".

No, you need specific compatibles. That's the standard DT rule.
Compatibility is expressed with list and fallback (see example-schema or
any other qcom binding, really 95% of them have fallbacks).

WSA usually have version registers so even if there are differences,
they are fully detectable, thus one more argument for compatibility.

> 
> Previous comment:
>> Do not repeat property name as description. Say something useful. "GPIO
>> spec for" is redundant, it cannot be anything else, so basically your
>> description saod "mclk" which is the same as in property name.
> 
>> Usually clocks are not GPIOs, so description could explain that.
> 
> Should the "GPIO spec for control signal to the clock gating circuit" be
> changed to "control signal to the clock gating circuit"?

I don't see previous code, cannot even reference it via reply-to
link/header. That way of communication is not effective.


Best regards,
Krzysztof