| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2a84eb1157a4421ba19900eeb2bfc7db@ti.com>
Date: Wed, 27 Dec 2023 06:58:09 +0000
From: "Ding, Shenghao" <shenghao-ding@...com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
CC: "robh+dt@...nel.org" <robh+dt@...nel.org>,
"andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
"Lu,
Kevin" <kevin-lu@...com>, "Xu, Baojun" <baojun.xu@...com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"lgirdwood@...il.com" <lgirdwood@...il.com>,
"perex@...ex.cz"
<perex@...ex.cz>,
"pierre-louis.bossart@...ux.intel.com"
<pierre-louis.bossart@...ux.intel.com>,
"13916275206@....com"
<13916275206@....com>,
"linux-sound@...r.kernel.org"
<linux-sound@...r.kernel.org>,
"linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>,
"liam.r.girdwood@...el.com"
<liam.r.girdwood@...el.com>,
"soyer@....hu" <soyer@....hu>, "tiwai@...e.de"
<tiwai@...e.de>,
"Gupta, Peeyush" <peeyush@...com>,
"Navada Kanyana, Mukund"
<navada@...com>,
"broonie@...nel.org" <broonie@...nel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>
Subject: RE: [EXTERNAL] Re: [PATCH v3 5/5] ASoC: dt-bindings: Add tas2563 into
ti,ta2781.yaml to enable DSP mode
> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Sent: Monday, December 25, 2023 9:18 PM
> To: Ding, Shenghao <shenghao-ding@...com>; broonie@...nel.org;
> conor+dt@...nel.org
> Cc: robh+dt@...nel.org; andriy.shevchenko@...ux.intel.com; Lu, Kevin
> <kevin-lu@...com>; Xu, Baojun <baojun.xu@...com>;
> devicetree@...r.kernel.org; lgirdwood@...il.com; perex@...ex.cz; pierre-
> louis.bossart@...ux.intel.com; 13916275206@....com; linux-
> sound@...r.kernel.org; linux-kernel@...r.kernel.org;
> liam.r.girdwood@...el.com; soyer@....hu; tiwai@...e.de; Gupta, Peeyush
> <peeyush@...com>; Navada Kanyana, Mukund <navada@...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@...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@...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
- 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
Powered by blists - more mailing lists