> -----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
