On 27/12/2023 07:58, Ding, Shenghao wrote:
>
>
>> -----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
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
[Index of Archives]
[Pulseaudio]
[Linux Audio Users]
[ALSA Devel]
[Fedora Desktop]
[Fedora SELinux]
[Big List of Linux Books]
[Yosemite News]
[KDE Users]
