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

[Ding, Shenghao]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Monday, December 25, 2023 9:18 PM
> To: Ding, Shenghao <shenghao-ding@xxxxxx>; broonie@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx
> Cc: robh+dt@xxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx; Lu, Kevin
> <kevin-lu@xxxxxx>; Xu, Baojun <baojun.xu@xxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx; perex@xxxxxxxx; pierre-
> louis.bossart@xxxxxxxxxxxxxxx; 13916275206@xxxxxxx; linux-
> sound@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> liam.r.girdwood@xxxxxxxxx; soyer@xxxxxx; tiwai@xxxxxxx; Gupta, Peeyush
> <peeyush@xxxxxx>; Navada Kanyana, Mukund <navada@xxxxxx>
> 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@xxxxxx>
> >
> > ---
> > 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@xxxxxx>
> >
> >  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
  - 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
> 
> > +      # 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.
> 
> It seems my or other reviewer's previous comments were not fully addressed.
> Maybe the feedback got lost between the quotes, maybe you just forgot to
> apply it. Please go back to the previous discussion and either implement all
> requested changes or keep discussing them.
> 
> Thank you.
> 
> 
> 
> Best regards,
> Krzysztof