2024-01-26 04:02:25

by Shenghao Ding

[permalink] [raw]
Subject: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add initial DT binding

PCM6240 driver implements a flexible and configurable setting for register
and filter coefficients, to one, two or even multiple PCM6240 Family Audio
chips.

Signed-off-by: Shenghao Ding <[email protected]>

---
Change in v2:
- Rewrite the subject to match something similar to other commits.
- And something to be compatible with.
- minItems, then maxItems.
- Drop reset-gpios description
- Remove the repeated reg descriptions and reg constraints.
- Drop redundant spaces.
- Add missing line breaks between blocks and additionalProperties.
- All these chips have only a portion of the functionality of codec,
such as ADC or DAC, and so on, but their audio performance is far
superior to the codec's, and cost is lower than codec, and much easier
to program than codec. Simply one or two register settings can enable
them to work. Init for these chips are hardware reset or software reset.
As to some audio filter params for internal filters, it is up to the
special user cases, which can be saved into the bin file. The default
value also can work well.
---
.../devicetree/bindings/sound/ti,pcm6240.yaml | 276 ++++++++++++++++++
1 file changed, 276 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/ti,pcm6240.yaml

diff --git a/Documentation/devicetree/bindings/sound/ti,pcm6240.yaml b/Documentation/devicetree/bindings/sound/ti,pcm6240.yaml
new file mode 100644
index 000000000000..f06264d81b46
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ti,pcm6240.yaml
@@ -0,0 +1,276 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 - 2024 Texas Instruments Incorporated
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/ti,pcm6240.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments PCM6240 Family Audio ADC/DAC
+
+maintainers:
+ - Shenghao Ding <[email protected]>
+
+description: |
+ The PCM6240 Family is a big family of Audio ADC/DAC for
+ different Specifications, range from Personal Electric
+ to Automotive Electric, even some professional fields.
+
+ Specifications about the audio chip can be found at:
+ https://www.ti.com/lit/gpn/tlv320adc3120
+ https://www.ti.com/lit/gpn/tlv320adc5120
+ https://www.ti.com/lit/gpn/tlv320adc6120
+ https://www.ti.com/lit/gpn/dix4192
+ https://www.ti.com/lit/gpn/pcm1690
+ https://www.ti.com/lit/gpn/pcm3120-q1
+ https://www.ti.com/lit/gpn/pcm3140-q1
+ https://www.ti.com/lit/gpn/pcm5120-q1
+ https://www.ti.com/lit/gpn/pcm6120-q1
+ https://www.ti.com/lit/gpn/pcm6260-q1
+ https://www.ti.com/lit/gpn/pcm9211
+ https://www.ti.com/lit/gpn/pcmd3140
+ https://www.ti.com/lit/gpn/pcmd3180
+ https://www.ti.com/lit/gpn/taa5212
+ https://www.ti.com/lit/gpn/tad5212
+
+properties:
+ compatible:
+ description: |
+ ti,adc3120: Stereo-channel, 768-kHz, Burr-Brown™ audio analog-to-
+ digital converter (ADC) with 106-dB SNR.
+
+ ti,adc5120: 2-Channel, 768-kHz, Burr-BrownTM Audio ADC with 120-dB SNR.
+
+ ti,adc6120: Stereo-channel, 768-kHz, Burr-Brown™ audio analog-to-
+ digital converter (ADC) with 123-dB SNR.
+
+ ti,pcm1690: 113dB SNR, 24-Bit, 192-kHz Sampling, Enhanced Multi-Level
+ ?S, Eight-Channel Audio Digital-to-Analog Converter with Differential
+ Outputs.
+
+ ti,pcm3120: Automotive, stereo, 106-dB SNR, 768-kHz, low-power
+ software-controlled audio ADC.
+
+ ti,pcm3140: Automotive, Quad-Channel, 768-kHz, Burr-BrownTM Audio ADC
+ with 106-dB SNR.
+
+ ti,pcm5120: Automotive, stereo, 120-dB SNR, 768-kHz, low-power
+ software-controlled audio ADC.
+
+ ti,pcm5140: Automotive, Quad-Channel, 768-kHz, Burr-BrownTM Audio ADC
+ with 120-dB SNR.
+
+ ti,pcm6120: Automotive, stereo, 123-dB SNR, 768-kHz, low-power
+ software-controlled audio ADC.
+
+ ti,pcm6140: Automotive, Quad-Channel, 768-kHz, Burr-BrownTM Audio ADC
+ with 123-dB SNR.
+
+ ti,pcm6240: Automotive 4-ch audio ADC with integrated programmable mic
+ bias, boost and input diagnostics.
+
+ ti,pcm6260: Automotive 6-ch audio ADC with integrated programmable mic
+ bias, boost and input diagnostics.
+
+ ti,pcm9211: 216-kHz Digital Audio Interface Transceiver (DIX)
+ With Stereo ADC and Routing.
+
+ ti,pcmd3140: Four-channel PDM-input to TDM or I�S output converter.
+
+ ti,pcmd3180: Eight-channel pulse-density-modulation input to TDM or
+ I�S output converter.
+
+ ti,taa5212: Low-power high-performance stereo audio ADC with 118-dB
+ dynamic range.
+
+ ti,tad5212: Low-power stereo audio DAC with 120-dB dynamic range.
+ oneOf:
+ - items:
+ - enum:
+ - ti,adc3120
+ - ti,adc5120
+ - ti,pcm3120
+ - ti,pcm5120
+ - ti,pcm6120
+ - const: ti,adc6120
+ - items:
+ - enum:
+ - ti,pcm6260
+ - ti,pcm6140
+ - ti,pcm3140
+ - ti,pcm5140
+ - const: ti,pcm6240
+ - items:
+ - const: ti,dix4192
+ - const: ti,pcm6240
+ - const: ti,adc6120
+ - items:
+ - const: ti,pcm1690
+ - const: ti,pcm9211
+ - const: ti,pcmd512x
+ - items:
+ - enum:
+ - ti,pcmd3180
+ - const: ti,pcmd3140
+ - items:
+ - enum:
+ - taa5412
+ - const: ti,taa5212
+ - items:
+ - enum:
+ - tad5412
+ - const: ti,tad5212
+ - items:
+ - enum:
+ - ti,pcm6240
+ - ti,pcmd3140
+ - ti,taa5212
+ - ti,tad5212
+ - ti,pcmd3180
+ reg:
+ description:
+ I2C address, in multiple pcmdevices case, all the i2c address
+ aggregate as one Audio Device to support multiple audio slots.
+ minItems: 1
+ maxItems: 4
+
+ reset-gpios:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+ description:
+ Invalid only for ti,pcm1690 because of no INT pin.
+
+ '#sound-dai-cells':
+ const: 0
+
+required:
+ - compatible
+ - reg
+
+allOf:
+ - $ref: dai-common.yaml#
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,pcm1690
+ then:
+ properties:
+ reg:
+ items:
+ minimum: 0x4c
+ maximum: 0x4f
+ interrupts: false
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,adc3120
+ - ti,adc5120
+ - ti,adc6120
+ - ti,pcm3120
+ - ti,pcm5120
+ - ti,pcm6120
+ - ti,pcmd3140
+ then:
+ properties:
+ reg:
+ maxItems: 1
+ items:
+ maximum: 0x4e
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,dix4192
+ then:
+ properties:
+ reg:
+ items:
+ minimum: 0x70
+ maximum: 0x73
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,pcm6240
+ - ti,pcm6260
+ then:
+ properties:
+ reg:
+ minItems: 1
+ maxItems: 4
+ items:
+ minimum: 0x48
+ maximum: 0x4b
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,pcm9211
+ then:
+ properties:
+ reg:
+ items:
+ minimum: 0x40
+ maximum: 0x43
+
+ - if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - ti,taa5212
+ - ti,taa5412
+ - ti,tad5212
+ - ti,tad5412
+ then:
+ properties:
+ reg:
+ items:
+ minimum: 0x50
+ maximum: 0x53
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ /* example for two devices with interrupt support */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pcm6240: audio-codec@48 {
+ compatible = "ti,pcm6240";
+ reg = <0x48>, /* primary-device */
+ <0x4b>; /* secondary-device */
+ #sound-dai-cells = <0>;
+ reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>;
+ interrupt-parent = <&gpio1>;
+ interrupts = <15>;
+ };
+ };
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ i2c {
+ /* example for one device without interrupt support*/
+ #address-cells = <1>;
+ #size-cells = <0>;
+ pcmd3180: audio-codec@4c {
+ compatible = "ti,pcmd3180";
+ reg = <0x4c>;
+ #sound-dai-cells = <0>;
+ reset-gpios = <&gpio1 10 GPIO_ACTIVE_HIGH>;
+ };
+ };
+...
--
2.34.1



2024-01-26 09:16:43

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add initial DT binding

On 26/01/2024 04:58, Shenghao Ding wrote:
> PCM6240 driver implements a flexible and configurable setting for register
> and filter coefficients, to one, two or even multiple PCM6240 Family Audio
> chips.
>

Subject: you just ignored my comment...

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

Tools like b4 or scripts_getmaintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, use mainline), work on fork of kernel (don't, use
mainline) or you ignore some maintainers (really don't). Just use b4 and
all the problems go away.

..

> + ti,pcmd3140: Four-channel PDM-input to TDM or I�S output converter.

This does not look like proper encoding.

> +
> + ti,pcmd3180: Eight-channel pulse-density-modulation input to TDM or
> + I�S output converter.
> +
> + ti,taa5212: Low-power high-performance stereo audio ADC with 118-dB
> + dynamic range.
> +
> + ti,tad5212: Low-power stereo audio DAC with 120-dB dynamic range.
> + oneOf:
> + - items:
> + - enum:
> + - ti,adc3120
> + - ti,adc5120
> + - ti,pcm3120
> + - ti,pcm5120
> + - ti,pcm6120
> + - const: ti,adc6120
> + - items:
> + - enum:
> + - ti,pcm6260
> + - ti,pcm6140
> + - ti,pcm3140
> + - ti,pcm5140
> + - const: ti,pcm6240
> + - items:
> + - const: ti,dix4192
> + - const: ti,pcm6240
> + - const: ti,adc6120

It does not make sense. You said above adc6120 is not compatible with
pcm6240.

> + - items:
> + - const: ti,pcm1690
> + - const: ti,pcm9211
> + - const: ti,pcmd512x
> + - items:
> + - enum:
> + - ti,pcmd3180
> + - const: ti,pcmd3140
> + - items:
> + - enum:
> + - taa5412
> + - const: ti,taa5212
> + - items:
> + - enum:
> + - tad5412
> + - const: ti,tad5212
> + - items:

No need for items.

> + - enum:
> + - ti,pcm6240
> + - ti,pcmd3140
> + - ti,taa5212
> + - ti,tad5212
> + - ti,pcmd3180

Where is adc6120 and others?

Missing blank line.

> + reg:
> + description:
> + I2C address, in multiple pcmdevices case, all the i2c address
> + aggregate as one Audio Device to support multiple audio slots.
> + minItems: 1
> + maxItems: 4
> +
> + reset-gpios:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> + description:
> + Invalid only for ti,pcm1690 because of no INT pin.
> +
> + '#sound-dai-cells':
> + const: 0
> +
> +required:
> + - compatible
> + - reg
> +
> +allOf:
> + - $ref: dai-common.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,pcm1690
> + then:
> + properties:
> + reg:
> + items:
> + minimum: 0x4c
> + maximum: 0x4f

Nothing improved.

> + interrupts: false
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,adc3120
> + - ti,adc5120
> + - ti,adc6120
> + - ti,pcm3120
> + - ti,pcm5120
> + - ti,pcm6120
> + - ti,pcmd3140
> + then:
> + properties:
> + reg:
> + maxItems: 1
> + items:
> + maximum: 0x4e
> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,dix4192
> + then:
> + properties:
> + reg:
> + items:
> + minimum: 0x70
> + maximum: 0x73

Drop entire if

> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,pcm6240
> + - ti,pcm6260
> + then:
> + properties:
> + reg:
> + minItems: 1
> + maxItems: 4
> + items:
> + minimum: 0x48
> + maximum: 0x4b

Drop entire if

> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,pcm9211
> + then:
> + properties:
> + reg:
> + items:
> + minimum: 0x40
> + maximum: 0x43

Drop entire if


> +
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - ti,taa5212
> + - ti,taa5412
> + - ti,tad5212
> + - ti,tad5412
> + then:
> + properties:
> + reg:
> + items:
> + minimum: 0x50
> + maximum: 0x53

Drop entire if


> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> + i2c {
> + /* example for two devices with interrupt support */
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pcm6240: audio-codec@48 {
> + compatible = "ti,pcm6240";
> + reg = <0x48>, /* primary-device */
> + <0x4b>; /* secondary-device */

Looks misaligned.



Best regards,
Krzysztof


2024-01-26 13:50:06

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add initial DT binding

On Fri, Jan 26, 2024 at 09:27:47AM +0100, Krzysztof Kozlowski wrote:
> On 26/01/2024 04:58, Shenghao Ding wrote:

> > + - if:
> > + properties:
> > + compatible:
> > + contains:
> > + enum:
> > + - ti,pcm1690
> > + then:
> > + properties:
> > + reg:
> > + items:
> > + minimum: 0x4c
> > + maximum: 0x4f

> Nothing improved.

Shenghao explained what what this is doing - I'm not sure what the
actual problem is here? It's an actual restriction on the values that
are valid.


Attachments:
(No filename) (587.00 B)
signature.asc (499.00 B)
Download all attachments

2024-01-29 04:45:02

by Shenghao Ding

[permalink] [raw]
Subject: RE: [EXTERNAL] Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add initial DT binding



> -----Original Message-----
> From: Mark Brown <[email protected]>
> Sent: Friday, January 26, 2024 9:50 PM
> To: Krzysztof Kozlowski <[email protected]>
> Cc: Ding, Shenghao <[email protected]>; [email protected];
> [email protected]; [email protected]; Lu, Kevin <kevin-
> [email protected]>; Xu, Baojun <[email protected]>; [email protected]; P
> O, Vijeth <[email protected]>; [email protected]; [email protected]; pierre-
> [email protected]; [email protected]; Chawla, Mohit
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected]; Huang,
> Jonathan <[email protected]>; [email protected]; Djuandi, Peter
> <[email protected]>; McPherson, Jeff <[email protected]>; Navada
> Kanyana, Mukund <[email protected]>
> Subject: [EXTERNAL] Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add
> initial DT binding
>
> On Fri, Jan 26, 2024 at 09:27:47AM +0100, Krzysztof Kozlowski wrote:
> > On 26/01/2024 04:58, Shenghao Ding wrote:
>
> > > + - if:
> > > + properties:
> > > + compatible:
> > > + contains:
> > > + enum:
> > > + - ti,pcm1690
> > > + then:
> > > + properties:
> > > + reg:
> > > + items:
> > > + minimum: 0x4c
> > > + maximum: 0x4f
>
> > Nothing improved.
>
> Shenghao explained what what this is doing - I'm not sure what the actual
> problem is here? It's an actual restriction on the values that are valid.

Hi, Krzysztof. May I have the privilege to petition on behalf of my customers? They
want to keep these if branches and the i2c address in yaml file. As you know, most
of my customers used to make mistakes and confuse with the i2c address. Listing
them here can help them to get the information easily.

2024-01-30 16:18:32

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [EXTERNAL] Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add initial DT binding

On 29/01/2024 05:43, Ding, Shenghao wrote:
>>
>> On Fri, Jan 26, 2024 at 09:27:47AM +0100, Krzysztof Kozlowski wrote:
>>> On 26/01/2024 04:58, Shenghao Ding wrote:
>>
>>>> + - if:
>>>> + properties:
>>>> + compatible:
>>>> + contains:
>>>> + enum:
>>>> + - ti,pcm1690
>>>> + then:
>>>> + properties:
>>>> + reg:
>>>> + items:
>>>> + minimum: 0x4c
>>>> + maximum: 0x4f
>>
>>> Nothing improved.
>>
>> Shenghao explained what what this is doing - I'm not sure what the actual
>> problem is here? It's an actual restriction on the values that are valid.
>
> Hi, Krzysztof. May I have the privilege to petition on behalf of my customers? They
> want to keep these if branches and the i2c address in yaml file. As you know, most
> of my customers used to make mistakes and confuse with the i2c address. Listing
> them here can help them to get the information easily.

To which cases this exception will apply? Your patches, all TI?

Bindings are not supposed to be so detailed to create DTS out if it,
because it makes them more difficult to maintain. Such amount of
if:then: makes it trickier, for no real gain.

Best regards,
Krzysztof


2024-01-31 12:18:40

by Shenghao Ding

[permalink] [raw]
Subject: RE: [EXTERNAL] Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240: Add initial DT binding



> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Wednesday, January 31, 2024 12:18 AM
> To: Ding, Shenghao <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Lu, Kevin <[email protected]>; Xu, Baojun
> <[email protected]>; [email protected]; P O, Vijeth <v-
> [email protected]>; [email protected]; [email protected]; pierre-
> [email protected]; [email protected]; Chawla, Mohit
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; [email protected]; Huang,
> Jonathan <[email protected]>; [email protected]; Djuandi, Peter
> <[email protected]>; McPherson, Jeff <[email protected]>; Navada
> Kanyana, Mukund <[email protected]>; Mark Brown <[email protected]>
> Subject: Re: [EXTERNAL] Re: [PATCH v2 4/4] ASoc: dt-bindings: PCM6240:
> Add initial DT binding
>
> On 29/01/2024 05: 43, Ding, Shenghao wrote: >> >> On Fri, Jan 26, 2024 at
> 09: 27: 47AM +0100, Krzysztof Kozlowski wrote: >>> On 26/01/2024 04: 58,
> Shenghao Ding wrote: >> >>>> + - if: >>>> + properties:
> ZjQcmQRYFpfptBannerStart This message was sent from outside of Texas
> Instruments.
> Do not click links or open attachments unless you recognize the source of
> this email and know the content is safe.
>
> ZjQcmQRYFpfptBannerEnd
> On 29/01/2024 05:43, Ding, Shenghao wrote:
> >>
> >> On Fri, Jan 26, 2024 at 09:27:47AM +0100, Krzysztof Kozlowski wrote:
> >>> On 26/01/2024 04:58, Shenghao Ding wrote:
> >>
> >>>> + - if:
> >>>> + properties:
> >>>> + compatible:
> >>>> + contains:
> >>>> + enum:
> >>>> + - ti,pcm1690
> >>>> + then:
> >>>> + properties:
> >>>> + reg:
> >>>> + items:
> >>>> + minimum: 0x4c
> >>>> + maximum: 0x4f
> >>
> >>> Nothing improved.
> >>
> >> Shenghao explained what what this is doing - I'm not sure what the
> >> actual problem is here? It's an actual restriction on the values that are
> valid.
> >
> > Hi, Krzysztof. May I have the privilege to petition on behalf of my
> > customers? They want to keep these if branches and the i2c address in
> > yaml file. As you know, most of my customers used to make mistakes and
> > confuse with the i2c address. Listing them here can help them to get the
> information easily.
>
> To which cases this exception will apply? Your patches, all TI?
>
Some AI customers usually like to try some innovative projects, such as their multiple-slot TDM
devices with serval different ADCs or DAC on different i2c buses. Traditional driver code really
can't meet their requirements.
> Bindings are not supposed to be so detailed to create DTS out if it, because it
> makes them more difficult to maintain. Such amount of
> if:then: makes it trickier, for no real gain.
>
Accept

Best regards,
Shenghao Ding