Re: [EXTERNAL] Re: [PATCH v3 5/5] ASoC: dt-bindings: Add tas2563 into ti,ta2781.yaml to enable DSP mode

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 27/12/2023 07:58, Ding, Shenghao wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Monday, December 25, 2023 9:18 PM
>> To: Ding, Shenghao <shenghao-ding@ti.com>; broonie@kernel.org;
>> conor+dt@kernel.org
>> Cc: robh+dt@kernel.org; andriy.shevchenko@linux.intel.com; Lu, Kevin
>> <kevin-lu@ti.com>; Xu, Baojun <baojun.xu@ti.com>;
>> devicetree@vger.kernel.org; lgirdwood@gmail.com; perex@perex.cz; pierre-
>> louis.bossart@linux.intel.com; 13916275206@139.com; linux-
>> sound@vger.kernel.org; linux-kernel@vger.kernel.org;
>> liam.r.girdwood@intel.com; soyer@irl.hu; tiwai@suse.de; Gupta, Peeyush
>> <peeyush@ti.com>; Navada Kanyana, Mukund <navada@ti.com>
>> Subject: [EXTERNAL] Re: [PATCH v3 5/5] ASoC: dt-bindings: Add tas2563 into
>> ti,ta2781.yaml to enable DSP mode
>>
>> On 25/12/2023 06:39, Shenghao Ding wrote:
>>> Move tas2563 from tas2562.yaml to tas2781.yaml, because tas2563 only
>>> work in bypass-DSP mode with tas2562 driver. In oder to enable DSP
>>> mode for tas2563, it has been moved to tas2781 driver. As to the
>>> hardware part, such as register setting and DSP firmware, all these
>>> are stored in the binary firmware. What tas2781 drivder dooes is to
>>> parse the firmware and download them to the tas2781 or tas2563, then
>> power on tas2781 or tas2563.
>>> So, tas2781 driver can be resued as tas2563 driver. Only attention
>>> will be paid to downloading corresponding firmware.
>>>
>>> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
>>>
>>> ---
>>> Change in v3:
>>>  - Add devicetree list and other list of necessary people and lists to
>>> CC
>>>  - Express Compatibility in the bindings
>>
>> Where?
>>
>>>  - Add more comments on why move tas2563 to tas2781 driver
>>>  - Provide rationale in terms of bindings and hardware, not in terms of
>> driver.
>>>    Or at least not only.
>>> ---
>>>  .../devicetree/bindings/sound/ti,tas2781.yaml | 66
>>> ++++++++++++++-----
>>>  1 file changed, 51 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
>>> b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
>>> index a69e6c223308..bbe8e5f2c013 100644
>>> --- a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
>>> @@ -5,36 +5,72 @@
>>>  $id: http://devicetree.org/schemas/sound/ti,tas2781.yaml#
>>>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>>>
>>> -title: Texas Instruments TAS2781 SmartAMP
>>> +title: Texas Instruments TAS2781/TAS2563 SmartAMP
>>>
>>>  maintainers:
>>>    - Shenghao Ding <shenghao-ding@ti.com>
>>>
>>>  description:
>>> -  The TAS2781 is a mono, digital input Class-D audio amplifier
>>> -  optimized for efficiently driving high peak power into small
>>> -  loudspeakers. An integrated on-chip DSP supports Texas Instruments
>>> -  Smart Amp speaker protection algorithm. The integrated speaker
>>> -  voltage and current sense provides for real time
>>> +  The TAS2781/TAS2563 is a mono, digital input Class-D audio
>>> + amplifier optimized for efficiently driving high peak power into
>>> + small loudspeakers. An integrated on-chip DSP supports Texas
>>> + Instruments Smart Amp speaker protection algorithm. The  integrated
>>> + speaker voltage and current sense provides for real time
>>>    monitoring of loudspeaker behavior.
>>>
>>>  allOf:
>>>    - $ref: dai-common.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - ti,tas2781
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          description:
>>> +            I2C address, in multiple AMP case, all the i2c address
>>> +            aggregate as one Audio Device to support multiple audio slots.
>>> +          maxItems: 8
>>> +          minItems: 1
>>> +          items:
>>> +            minimum: 0x38
>>> +            maximum: 0x3f
>>> +    else:
>>
>> How this else is possible? Please show me any DTS which triggers this else
>> case.
> Do you mean add two if and remove else branch. Like following

Apologies, maybe my comment was not right, because the context tricked
me. The if:else usually goes with allOf: to the end of the file, after
required: block. I think I assumed there is no compatible change, since
you this is expected to be the last diff hunk in the patch.

>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - ti,tas2781
>     then:
>       properties:
>         reg:
>           description:
>             I2C address, in multiple-AMP case, all the i2c address
>             aggregate as one Audio Device to support multiple audio slots.
>           maxItems: 8
>           minItems: 1
>           items:
>             minimum: 0x38
>             maximum: 0x3f
>   - if:
>       properties:
>         compatible:
>           contains:
>             enum:
>               - ti,tas2563
>     then:
>       properties:
>         reg:
>           description:
>             I2C address, in multiple-AMP case, all the i2c address
>             aggregate as one Audio Device to support multiple audio slots.
>           maxItems: 4
>           minItems: 1
>           items:
>             minimum: 0x4c
>             maximum: 0x4f
>>
>>
>>> +      properties:
>>> +        reg:
>>> +          description:
>>> +            I2C address, in multiple AMP case, all the i2c address
>>> +            aggregate as one Audio Device to support multiple audio slots.
>>> +          maxItems: 4
>>> +          minItems: 1
>>> +          items:
>>> +            minimum: 0x4c
>>> +            maximum: 0x4f
>>>
>>>  properties:
>>>    compatible:
>>> +    description: |
>>> +      ti,tas2781: 24-V Class-D Amplifier with Real Time Integrated Speaker
>>> +      Protection and Audio Processing, 16/20/24/32bit stereo I2S or
>>> +      multichannel TDM.
>>> +
>>> +      ti,tas2563: 6.1-W Boosted Class-D Audio Amplifier With Integrated
>>> +      DSP and IV Sense, 16/20/24/32bit stereo I2S or multichannel TDM.
>>>      enum:
>>>        - ti,tas2781
>>> +      - ti,tas2563
>>
>> Still nothing improved. Where is the fallback?
> Do you mean to add following as fallback?
>     oneOf:
>       - items:
>           - enum:
>               - ti,tas2781
>       - items:
>           - enum:
>               - ti,tas2563

These are two separate list of items. What I meant is something like
example schema:
oneOf:
 - enum
 - items
     - const
     - const

>>
>>> +      # Tas781 driver can support both tas2563 and tas2781, because the
>>> +      # hardware part in the driver code, such as register setting and DSP
>>> +      # firmware, all these are stored in the binary firmware. What drivder
>>> +      # dooes is to parse the firmware and download it to the tas2781 or
>>> +      # tas2563, then control tas2781 or tas2563 to power on/off or switch
>>> +      # different dsp params. So, tas2781 driver can be resued as tas2563
>>> +      # driver. Only attention will be paid to downloading corresponding
>>> +      # firmware.
>>
>> Don't write useless driver description and implement proper list of two
>> compatibles using one as fallback for another. I already pointed you to
>> example-schema which gives you nice example for this.
>>
>> It is third try not doing what I asked you. Probably we misunderstand each
>> other, then please answer:
>>
>> 1. Please find example-schema.yaml and share whether this succeeded or not.
>> 2. Open the example-schema.yaml in your editor.
>> 3. Read the section about compatibles. You need oneOf and items, just like it
>> is there.
>>
>> Now, please confirm that you did all these steps before you send v4 with
>> more test.
>>
>>>
>>> -  reg:
>>> -    description:
>>> -      I2C address, in multiple tas2781s case, all the i2c address
>>> -      aggregate as one Audio Device to support multiple audio slots.
>>> -    maxItems: 8
>>> -    minItems: 1
>>> -    items:
>>> -      minimum: 0x38
>>> -      maximum: 0x3f
>>> +  reg: true
>>
>> OK, you clearly just keep ignoring my comments.
>>
>> This is a friendly reminder during the review process.

Just to remind this - my previous comments about not removing widest
constraints is still valid.

Best regards,
Krzysztof