[PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier.

[PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier.

[Shenghao Ding]

Create tas2781.yaml for tas2781 driver.

Signed-off-by: Shenghao Ding <13916275206@139.com>

---
 Changes to be committed:
	new file:   Documentation/devicetree/bindings/sound/ti,tas2781.yaml
---
 .../devicetree/bindings/sound/ti,tas2781.yaml | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/ti,tas2781.yaml

diff --git a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
new file mode 100644
index 000000000..9a440d29f
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 - 2023 Texas Instruments Incorporated
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/ti,tas2781.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments TAS2781 SmartAMP
+
+maintainers:
+  - Shenghao Ding <shenghao-ding@ti.com>
+  - Kevin Lu <kevin-lu@ti.com>
+
+description: |
+  The TAS2781 is a mono, digital input Class-D audio amplifier
+  optimized for efficiently driving high peak power into small
+  loudspeakers. Integrated an 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.
+
+properties:
+  compatible:
+    enum:
+      - ti,tas2781
+
+  reg:
+    maxItems: 1
+    description: |
+      I2C address of the device can be in range from 0x38 to 0x40.
+
+  reset-gpios:
+    maxItems: 1
+    description: |
+      A GPIO line handling reset of the chip. As the line is active high,
+      it should be marked GPIO_ACTIVE_HIGH (see ../gpio/gpio.txt)
+
+  interrupts:
+    maxItems: 1
+
+  ti,audio-slots:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 4
+    description: |
+      I2c address of the device for different audio slots,
+      useless in mono case.
+
+  ti,broadcast-addr:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Generic i2c address for all the tas2781 devices in
+      purpose of I2C broadcast during the multi-device
+      writes, useless in mono case.
+
+  '#sound-dai-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+   #include <dt-bindings/gpio/gpio.h>
+   i2c {
+     /* example with quad support, such as tablet or pad device */
+     #address-cells = <1>;
+     #size-cells = <0>;
+     quad: codec@38 {
+       compatible = "ti,tas2781";
+       reg = <0x38>;
+       #sound-dai-cells = <1>;
+       reset-gpios = < &gpio1 10 GPIO_ACTIVE_HIGH >;
+       interrupt-parent = <&gpio1>;
+       interrupts = <15>;
+       ti,audio-slots = < 0x38 /* topleft-channel */
+                          0x39 /* topright-channel */
+                          0x3a /* bottomleft-channel */
+                          0x3b /* bottomright-channel */
+                        >;
+       ti,broadcast-addr = <0x40>;
+     };
+   };
+...
-- 
2.34.1

Re: [PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier.

[Krzysztof Kozlowski]

On 31/01/2023 14:17, Shenghao Ding wrote:
> Create tas2781.yaml for tas2781 driver.

You treat my feedback selectively... some things improved, but not
everything. I am literally repeating the same and expect different
results from you... isn't that the definition of insanity?

Let me paste ignored parts:

1. Your patches are still not correctly threaded. There is no threading
at all.

I said it multiple times. What else should I say here?

2. Subject: ASoC: dt-bindings: Add tas2781 amplifier
(drop the full stop - why did you add it there?)

3. Missing changelog under ---.
I mentioned it already few times, didn't I?

> 
> Signed-off-by: Shenghao Ding <13916275206@139.com>
> 
> ---
>  Changes to be committed:
> 	new file:   Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> ---
>  .../devicetree/bindings/sound/ti,tas2781.yaml | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> new file mode 100644
> index 000000000..9a440d29f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 - 2023 Texas Instruments Incorporated
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/ti,tas2781.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments TAS2781 SmartAMP
> +
> +maintainers:
> +  - Shenghao Ding <shenghao-ding@ti.com>
> +  - Kevin Lu <kevin-lu@ti.com>
> +
> +description: |
> +  The TAS2781 is a mono, digital input Class-D audio amplifier
> +  optimized for efficiently driving high peak power into small
> +  loudspeakers. Integrated an 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.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tas2781
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      I2C address of the device can be in range from 0x38 to 0x40.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: |
> +      A GPIO line handling reset of the chip. As the line is active high,
> +      it should be marked GPIO_ACTIVE_HIGH (see ../gpio/gpio.txt)

Drop the reference to gpio.txt.

I give up. I spent too much time on it while being also ignored a bit
too much.


Best regards,
Krzysztof

[PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier

[Shenghao Ding]

Create tas2781.yaml for tas2781 driver.

Signed-off-by: Shenghao Ding <13916275206@139.com>

---
Changes in v5:
 - Drop the full stop in the Subject.
 - Drop the reset-gpios reference to gpio.txt.
 - Add Changelog.
 Changes to be committed:
	new file:   Documentation/devicetree/bindings/sound/ti,tas2781.yaml
---
 .../devicetree/bindings/sound/ti,tas2781.yaml | 87 +++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/ti,tas2781.yaml

diff --git a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
new file mode 100644
index 000000000000..8af44792a904
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
@@ -0,0 +1,87 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 - 2023 Texas Instruments Incorporated
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/ti,tas2781.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments TAS2781 SmartAMP
+
+maintainers:
+  - Shenghao Ding <shenghao-ding@ti.com>
+  - Kevin Lu <kevin-lu@ti.com>
+
+description: |
+  The TAS2781 is a mono, digital input Class-D audio amplifier
+  optimized for efficiently driving high peak power into small
+  loudspeakers. Integrated an 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.
+
+properties:
+  compatible:
+    enum:
+      - ti,tas2781
+
+  reg:
+    maxItems: 1
+    description: |
+      I2C address of the device can be in range from 0x38 to 0x40.
+
+  reset-gpios:
+    maxItems: 1
+    description: |
+      A GPIO line handling reset of the chip. As the line is active high,
+      it should be marked GPIO_ACTIVE_HIGH.
+
+  interrupts:
+    maxItems: 1
+
+  ti,audio-slots:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 4
+    description: |
+      I2c address of the device for different audio slots,
+      useless in mono case.
+
+  ti,broadcast-addr:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      Generic i2c address for all the tas2781 devices in
+      purpose of I2C broadcast during the multi-device
+      writes, useless in mono case.
+
+  '#sound-dai-cells':
+    const: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+   #include <dt-bindings/gpio/gpio.h>
+   i2c {
+     /* example with quad support, such as tablet or pad device */
+     #address-cells = <1>;
+     #size-cells = <0>;
+     quad: codec@38 {
+       compatible = "ti,tas2781";
+       reg = <0x38>;
+       #sound-dai-cells = <1>;
+       reset-gpios = < &gpio1 10 GPIO_ACTIVE_HIGH >;
+       interrupt-parent = <&gpio1>;
+       interrupts = <15>;
+       ti,audio-slots = < 0x38 /* topleft-channel */
+                          0x39 /* topright-channel */
+                          0x3a /* bottomleft-channel */
+                          0x3b /* bottomright-channel */
+                        >;
+       ti,broadcast-addr = <0x40>;
+     };
+   };
+...
-- 
2.34.1

Re: [PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier

[Krzysztof Kozlowski]

On 03/02/2023 05:01, Shenghao Ding wrote:
> Create tas2781.yaml for tas2781 driver.
> 
> Signed-off-by: Shenghao Ding <13916275206@139.com>

You got four times my comment that your threading is wrong. You never
replied to that comment that something is not clear. Threading is still
wrong - there is no user of this binding here.

Best regards,
Krzysztof

Re: [PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier

[Rob Herring]

On Fri, Feb 03, 2023 at 12:01:15PM +0800, Shenghao Ding wrote:
> Create tas2781.yaml for tas2781 driver.
> 
> Signed-off-by: Shenghao Ding <13916275206@139.com>
> 
> ---
> Changes in v5:
>  - Drop the full stop in the Subject.
>  - Drop the reset-gpios reference to gpio.txt.
>  - Add Changelog.
>  Changes to be committed:
> 	new file:   Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> ---
>  .../devicetree/bindings/sound/ti,tas2781.yaml | 87 +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> new file mode 100644
> index 000000000000..8af44792a904
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (C) 2022 - 2023 Texas Instruments Incorporated
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/ti,tas2781.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments TAS2781 SmartAMP
> +
> +maintainers:
> +  - Shenghao Ding <shenghao-ding@ti.com>
> +  - Kevin Lu <kevin-lu@ti.com>
> +
> +description: |

Don't need '|' unless you need line endings preserved. Wrap lines at 80 
chars.

> +  The TAS2781 is a mono, digital input Class-D audio amplifier
> +  optimized for efficiently driving high peak power into small
> +  loudspeakers. Integrated an 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.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tas2781
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      I2C address of the device can be in range from 0x38 to 0x40.

Express as constraints instead:

items:
  minimum: 0x38
  maximum: 0x40

Or was this the range of the slot addresses?

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: |
> +      A GPIO line handling reset of the chip. As the line is active high,
> +      it should be marked GPIO_ACTIVE_HIGH.

The description doesn't add anything. Drop.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  ti,audio-slots:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 4
> +    description: |
> +      I2c address of the device for different audio slots,

I2C

> +      useless in mono case.
> +
> +  ti,broadcast-addr:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Generic i2c address for all the tas2781 devices in

I2C

Be consistent.

> +      purpose of I2C broadcast during the multi-device
> +      writes, useless in mono case.
> +
> +  '#sound-dai-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +   #include <dt-bindings/gpio/gpio.h>
> +   i2c {
> +     /* example with quad support, such as tablet or pad device */
> +     #address-cells = <1>;
> +     #size-cells = <0>;
> +     quad: codec@38 {
> +       compatible = "ti,tas2781";
> +       reg = <0x38>;
> +       #sound-dai-cells = <1>;
> +       reset-gpios = < &gpio1 10 GPIO_ACTIVE_HIGH >;
> +       interrupt-parent = <&gpio1>;
> +       interrupts = <15>;
> +       ti,audio-slots = < 0x38 /* topleft-channel */
> +                          0x39 /* topright-channel */
> +                          0x3a /* bottomleft-channel */
> +                          0x3b /* bottomright-channel */

Do these vary? Or the slot addresses are fixed and which slots are used 
varies? 

> +                        >;
> +       ti,broadcast-addr = <0x40>;

I tend to think the I2C addresses should all be in 'reg'.

> +     };
> +   };
> +...
> -- 
> 2.34.1
> 
> 

RE: [EXTERNAL] Re: [PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier

[Ding, Shenghao]

Hi Rob 
Thanks for your review comments. My answers are inline
with prefix [DING]

-----Original Message-----
From: Rob Herring <robh@kernel.org> 
Sent: Saturday, February 4, 2023 5:57 AM
To: Shenghao Ding <13916275206@139.com>
Cc: krzysztof.kozlowski+dt@linaro.org; broonie@kernel.org; lgirdwood@gmail.com; Lu, Kevin <kevin-lu@ti.com>; Ding, Shenghao <shenghao-ding@ti.com>; alsa-devel@alsa-project.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [EXTERNAL] Re: [PATCH v5] ASoC: dt-bindings: Add tas2781 amplifier

On Fri, Feb 03, 2023 at 12:01:15PM +0800, Shenghao Ding wrote:
> Create tas2781.yaml for tas2781 driver.
> 
> Signed-off-by: Shenghao Ding <13916275206@139.com>
> 
> ---
> Changes in v5:
>  - Drop the full stop in the Subject.
>  - Drop the reset-gpios reference to gpio.txt.
>  - Add Changelog.
>  Changes to be committed:
> 	new file:   Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> ---
>  .../devicetree/bindings/sound/ti,tas2781.yaml | 87 
> +++++++++++++++++++
>  1 file changed, 87 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/ti,tas2781.yaml 
> b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> new file mode 100644
> index 000000000000..8af44792a904
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/ti,tas2781.yaml
> @@ -0,0 +1,87 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright 
> +(C) 2022 - 2023 Texas Instruments Incorporated %YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/ti,tas2781.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments TAS2781 SmartAMP
> +
> +maintainers:
> +  - Shenghao Ding <shenghao-ding@ti.com>
> +  - Kevin Lu <kevin-lu@ti.com>
> +
> +description: |

Don't need '|' unless you need line endings preserved. Wrap lines at 80 chars.
[DING] Accept this comment. I will remove all the "|" after "description:"
> +  The TAS2781 is a mono, digital input Class-D audio amplifier  
> + optimized for efficiently driving high peak power into small  
> + loudspeakers. Integrated an 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.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tas2781
> +
> +  reg:
> +    maxItems: 1
> +    description: |
> +      I2C address of the device can be in range from 0x38 to 0x40.

Express as constraints instead:

items:
  minimum: 0x38
  maximum: 0x40
[DING] Accept this comment.
Or was this the range of the slot addresses?
[DING] Yes.
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: |
> +      A GPIO line handling reset of the chip. As the line is active high,
> +      it should be marked GPIO_ACTIVE_HIGH.

The description doesn't add anything. Drop.

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  ti,audio-slots:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 4
> +    description: |
> +      I2c address of the device for different audio slots,

I2C

> +      useless in mono case.
> +
> +  ti,broadcast-addr:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Generic i2c address for all the tas2781 devices in

I2C

Be consistent.
[DING] Accept this comment.
> +      purpose of I2C broadcast during the multi-device
> +      writes, useless in mono case.
> +
> +  '#sound-dai-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +   #include <dt-bindings/gpio/gpio.h>
> +   i2c {
> +     /* example with quad support, such as tablet or pad device */
> +     #address-cells = <1>;
> +     #size-cells = <0>;
> +     quad: codec@38 {
> +       compatible = "ti,tas2781";
> +       reg = <0x38>;
> +       #sound-dai-cells = <1>;
> +       reset-gpios = < &gpio1 10 GPIO_ACTIVE_HIGH >;
> +       interrupt-parent = <&gpio1>;
> +       interrupts = <15>;
> +       ti,audio-slots = < 0x38 /* topleft-channel */
> +                          0x39 /* topright-channel */
> +                          0x3a /* bottomleft-channel */
> +                          0x3b /* bottomright-channel */

Do these vary? Or the slot addresses are fixed and which slots are used varies? 
[DING] Yes, these will vary with different projects. In some projects there're two addresses, 
in some there're six addresses, in others the I2C addresses may be out of order. 
> +                        >;
> +       ti,broadcast-addr = <0x40>;

I tend to think the I2C addresses should all be in 'reg'.
[DING] broadcast-addr item is used in multiple tas2781s case to enable the 
broadcast mode to download dsp fw  simultaneously. Without this item, 
broadcast mode will be disabled, and downloading dsp fw will be one by one.
As you know, not all project will enable the broadcast feature, so we have to 
add this item. As to reg item, it is only stored the primary tas2781 I2C addr.
> +     };
> +   };
> +...
> --
> 2.34.1
> 
>