[RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

Hi,

I put together a machine-level ASoC driver for recent Apple Macs (the
ones with ARM64 SoCs) and want to gauge opinions.

Commit 1 is the binding. It is some subset of simple-audio-card with
the extra distinction of allowing multiple CPU/CODEC DAIs per a DAI
link. I want to draw special attention to the issue of describing
speaker topologies. The way it now works is that the driver expects
the speakers to be declared in a fixed order in the sound-dai= list.
This populates a topology the driver expects on a particular machine
model. Mark (in CC) has made the suggestion of keeping the topology
descriptions with the codec nodes themselves in some generic manner,
akin to how sound-name-prefix= already helps identify codecs to the
user.

Commit 2 adds a new ASoC card method (filter_controls) to let the card
prevent some codec kcontrols from being visible to userspace. For example
the TAS2770 speaker amp driver would be happy to expose TDM slot selection
and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to
make the patch acceptable to upstream.

Commit 3 makes ASoC tolerate N-to-M DAI links, not sure what the right
(simple) approach should be there. Commit 4 adds some utility function
and commit 5 is the driver itself.

Let me know what you think.

Martin

Martin Povišer (5):
  dt-bindings: sound: Add Apple Macs sound system
  HACK: ASoC: Add card->filter_controls hook
  HACK: ASoC: Tolerate N-cpus-to-M-codecs links
  ASoC: Introduce snd_soc_of_get_dai_link_cpus
  ASoC: Add macaudio machine driver

 .../bindings/sound/apple,macaudio.yaml        | 103 +++
 include/sound/soc.h                           |   7 +
 sound/soc/apple/Kconfig                       |  10 +
 sound/soc/apple/Makefile                      |   3 +
 sound/soc/apple/macaudio.c                    | 597 ++++++++++++++++++
 sound/soc/soc-core.c                          | 125 +++-
 sound/soc/soc-dapm.c                          |  34 +-
 sound/soc/soc-pcm.c                           |   3 +
 8 files changed, 860 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml
 create mode 100644 sound/soc/apple/Kconfig
 create mode 100644 sound/soc/apple/Makefile
 create mode 100644 sound/soc/apple/macaudio.c

-- 
2.33.0

[RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

[Martin Povišer]

Add binding for Apple Silicon Macs' machine-level sound system.

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 .../bindings/sound/apple,macaudio.yaml        | 103 ++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml

diff --git a/Documentation/devicetree/bindings/sound/apple,macaudio.yaml b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
new file mode 100644
index 000000000000..a6380e4bdd1a
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
@@ -0,0 +1,103 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/apple,macaudio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Silicon Macs integrated sound system
+
+maintainers:
+  - Martin Povišer <povik+lin@cutebit.org>
+
+definitions:
+  dai:
+    type: object
+    properties:
+      sound-dai: true
+    required:
+      - sound-dai
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - apple,j274-macaudio
+          - apple,j293-macaudio
+          - apple,j314-macaudio
+      - const: apple,macaudio
+  "#address-cells":
+    const: 1
+  "#size-cells":
+    const: 0
+  model:
+    description: |
+      Model name to use when the sound system is presented to users as a sound card.
+    $ref: /schemas/types.yaml#/definitions/string
+
+patternProperties:
+  "^dai-link(@[0-9a-f]+)?$":
+    description: |
+      A DAI link comprising of CPU and CODEC DAI specifiers and supplemental properties.
+    type: object
+    properties:
+      reg:
+        maxItems: 1
+      mclk-fs:
+        description: |
+          Forced MCLK/samplerate factor (optional).
+        $ref: /schemas/types.yaml#/definitions/uint32
+      link-name:
+        description: Name for the DAI link to present to users.
+        $ref: /schemas/types.yaml#/definitions/string
+      cpu:
+        $ref: "#/definitions/dai"
+      codec:
+        $ref: "#/definitions/dai"
+    required:
+      - reg
+      - cpu
+      - codec
+    additionalProperties: false
+
+required:
+  - compatible
+  - model
+
+additionalProperties: false
+
+examples:
+  - |
+    sound {
+      compatible = "apple,j293-macaudio", "apple,macaudio";
+      model = "MacBook Pro J293 integrated audio";
+
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      dai-link@0 {
+        reg = <0>;
+        link-name = "Speakers";
+        mclk-fs = <64>;
+
+        cpu {
+          sound-dai = <&mca 0>, <&mca 1>;
+        };
+        codec {
+          sound-dai = <&speaker_left_front>, <&speaker_right_front>,
+            <&speaker_left_rear>, <&speaker_right_rear>;
+        };
+      };
+
+      dai-link@1 {
+        reg = <1>;
+        link-name = "Headphones Jack";
+        mclk-fs = <64>;
+
+        cpu {
+          sound-dai = <&mca 2>;
+        };
+        codec {
+          sound-dai = <&jack_codec>;
+        };
+      };
+    };
-- 
2.33.0

Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

[Krzysztof Kozlowski]

On 31/03/2022 02:04, Martin Povišer wrote:
> Add binding for Apple Silicon Macs' machine-level sound system.
> 
> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
> ---
>  .../bindings/sound/apple,macaudio.yaml        | 103 ++++++++++++++++++
>  1 file changed, 103 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml
> 

Commit title does not match subsystem.

> diff --git a/Documentation/devicetree/bindings/sound/apple,macaudio.yaml b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
> new file mode 100644
> index 000000000000..a6380e4bdd1a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
> @@ -0,0 +1,103 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/apple,macaudio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Silicon Macs integrated sound system
> +
> +maintainers:
> +  - Martin Povišer <povik+lin@cutebit.org>
> +

Add description.

> +definitions:

This does not make code more readable.

> +  dai:
> +    type: object
> +    properties:
> +      sound-dai: true
> +    required:
> +      - sound-dai
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,j274-macaudio
> +          - apple,j293-macaudio
> +          - apple,j314-macaudio
> +      - const: apple,macaudio

Open example-schema.yaml and look at formatting plus general coding
style. You miss line breaks making it unreadable.

> +  "#address-cells":
> +    const: 1
> +  "#size-cells":
> +    const: 0
> +  model:
> +    description: |
> +      Model name to use when the sound system is presented to users as a sound card.
> +    $ref: /schemas/types.yaml#/definitions/string
> +
> +patternProperties:
> +  "^dai-link(@[0-9a-f]+)?$":
> +    description: |
> +      A DAI link comprising of CPU and CODEC DAI specifiers and supplemental properties.
> +    type: object
> +    properties:
> +      reg:
> +        maxItems: 1
> +      mclk-fs:
> +        description: |
> +          Forced MCLK/samplerate factor (optional).

Optional is obvious from !required.

Description is different than existing field in simple card. Is this the
same field or not?

> +        $ref: /schemas/types.yaml#/definitions/uint32
> +      link-name:
> +        description: Name for the DAI link to present to users.
> +        $ref: /schemas/types.yaml#/definitions/string
> +      cpu:
> +        $ref: "#/definitions/dai"
> +      codec:
> +        $ref: "#/definitions/dai"

missing maxItems for DAI phandles.

> +    required:
> +      - reg
> +      - cpu
> +      - codec
> +    additionalProperties: false

This entire block is unreadable.

> +
> +required:
> +  - compatible
> +  - model
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    sound {
> +      compatible = "apple,j293-macaudio", "apple,macaudio";
> +      model = "MacBook Pro J293 integrated audio";
> +
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      dai-link@0 {
> +        reg = <0>;
> +        link-name = "Speakers";
> +        mclk-fs = <64>;
> +
> +        cpu {
> +          sound-dai = <&mca 0>, <&mca 1>;
> +        };
> +        codec {
> +          sound-dai = <&speaker_left_front>, <&speaker_right_front>,
> +            <&speaker_left_rear>, <&speaker_right_rear>;

Align the line.

> +        };
> +      };
> +
> +      dai-link@1 {
> +        reg = <1>;
> +        link-name = "Headphones Jack";
> +        mclk-fs = <64>;
> +
> +        cpu {
> +          sound-dai = <&mca 2>;
> +        };
> +        codec {
> +          sound-dai = <&jack_codec>;
> +        };
> +      };
> +    };


Best regards,
Krzysztof

Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

[Martin Povišer]

> On 31. 3. 2022, at 8:43, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> On 31/03/2022 02:04, Martin Povišer wrote:
>> Add binding for Apple Silicon Macs' machine-level sound system.
>> 
>> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>> ---
>> .../bindings/sound/apple,macaudio.yaml        | 103 ++++++++++++++++++
>> 1 file changed, 103 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>> 
> 
> Commit title does not match subsystem.

Tell more please. I don’t see it.

> 
>> diff --git a/Documentation/devicetree/bindings/sound/apple,macaudio.yaml b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>> new file mode 100644
>> index 000000000000..a6380e4bdd1a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>> @@ -0,0 +1,103 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/sound/apple,macaudio.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Apple Silicon Macs integrated sound system
>> +
>> +maintainers:
>> +  - Martin Povišer <povik+lin@cutebit.org>
>> +
> 
> Add description.
> 
>> +definitions:
> 
> This does not make code more readable.

Are you sure? It prevents duplication later on for ‘codec' and ‘cpu’.

> 
>> +  dai:
>> +    type: object
>> +    properties:
>> +      sound-dai: true
>> +    required:
>> +      - sound-dai
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - apple,j274-macaudio
>> +          - apple,j293-macaudio
>> +          - apple,j314-macaudio
>> +      - const: apple,macaudio
> 
> Open example-schema.yaml and look at formatting plus general coding
> style. You miss line breaks making it unreadable.
> 
>> +  "#address-cells":
>> +    const: 1
>> +  "#size-cells":
>> +    const: 0
>> +  model:
>> +    description: |
>> +      Model name to use when the sound system is presented to users as a sound card.
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +
>> +patternProperties:
>> +  "^dai-link(@[0-9a-f]+)?$":
>> +    description: |
>> +      A DAI link comprising of CPU and CODEC DAI specifiers and supplemental properties.
>> +    type: object
>> +    properties:
>> +      reg:
>> +        maxItems: 1
>> +      mclk-fs:
>> +        description: |
>> +          Forced MCLK/samplerate factor (optional).
> 
> Optional is obvious from !required.
> 
> Description is different than existing field in simple card. Is this the
> same field or not?

It is the same. I didn’t want to copy the simple card text because this is optionally BSD,
simple card wasn’t.

> 
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +      link-name:
>> +        description: Name for the DAI link to present to users.
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +      cpu:
>> +        $ref: "#/definitions/dai"
>> +      codec:
>> +        $ref: "#/definitions/dai"
> 
> missing maxItems for DAI phandles.

Well there’s not a maximum.

> 
> Best regards,
> Krzysztof

Martin

Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

[Krzysztof Kozlowski]

On 31/03/2022 08:57, Martin Povišer wrote:
> 
>> On 31. 3. 2022, at 8:43, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 31/03/2022 02:04, Martin Povišer wrote:
>>> Add binding for Apple Silicon Macs' machine-level sound system.
>>>
>>> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>>> ---
>>> .../bindings/sound/apple,macaudio.yaml        | 103 ++++++++++++++++++
>>> 1 file changed, 103 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>>
>>
>> Commit title does not match subsystem.
> 
> Tell more please. I don’t see it.

git log --oneline  -- Documentation/devicetree/bindings/sound/


Mark expects "ASoC: dt-bindings:"

> 
>>
>>> diff --git a/Documentation/devicetree/bindings/sound/apple,macaudio.yaml b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>> new file mode 100644
>>> index 000000000000..a6380e4bdd1a
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>> @@ -0,0 +1,103 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/sound/apple,macaudio.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Apple Silicon Macs integrated sound system
>>> +
>>> +maintainers:
>>> +  - Martin Povišer <povik+lin@cutebit.org>
>>> +
>>
>> Add description.
>>
>>> +definitions:
>>
>> This does not make code more readable.
> 
> Are you sure? It prevents duplication later on for ‘codec' and ‘cpu’.

That's true, but duplication is small, unless you think this will be
extended. I guess it is a trade-off, but so far for few lines and just
two users of such definition, I would prefer to duplicate. I don't have
strong opinion, though.

> 
>>
>>> +  dai:
>>> +    type: object
>>> +    properties:
>>> +      sound-dai: true
>>> +    required:
>>> +      - sound-dai
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - apple,j274-macaudio
>>> +          - apple,j293-macaudio
>>> +          - apple,j314-macaudio
>>> +      - const: apple,macaudio
>>
>> Open example-schema.yaml and look at formatting plus general coding
>> style. You miss line breaks making it unreadable.
>>
>>> +  "#address-cells":
>>> +    const: 1
>>> +  "#size-cells":
>>> +    const: 0
>>> +  model:
>>> +    description: |
>>> +      Model name to use when the sound system is presented to users as a sound card.
>>> +    $ref: /schemas/types.yaml#/definitions/string
>>> +
>>> +patternProperties:
>>> +  "^dai-link(@[0-9a-f]+)?$":
>>> +    description: |
>>> +      A DAI link comprising of CPU and CODEC DAI specifiers and supplemental properties.
>>> +    type: object
>>> +    properties:
>>> +      reg:
>>> +        maxItems: 1
>>> +      mclk-fs:
>>> +        description: |
>>> +          Forced MCLK/samplerate factor (optional).
>>
>> Optional is obvious from !required.
>>
>> Description is different than existing field in simple card. Is this the
>> same field or not?
> 
> It is the same. I didn’t want to copy the simple card text because this is optionally BSD,
> simple card wasn’t.

OK

> 
>>
>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>> +      link-name:
>>> +        description: Name for the DAI link to present to users.
>>> +        $ref: /schemas/types.yaml#/definitions/string
>>> +      cpu:
>>> +        $ref: "#/definitions/dai"
>>> +      codec:
>>> +        $ref: "#/definitions/dai"
>>
>> missing maxItems for DAI phandles.
> 
> Well there’s not a maximum.

There should be some maximum of supported codecs. Hardware might have
such constraints. If really unsure, choose some reasonable (small)
amount. It could be later raised, if needed.


Best regards,
Krzysztof

Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

[Martin Povišer]

> On 31. 3. 2022, at 10:17, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
> On 31/03/2022 08:57, Martin Povišer wrote:
>> 
>>> On 31. 3. 2022, at 8:43, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>>> 
>>> On 31/03/2022 02:04, Martin Povišer wrote:
>>>> Add binding for Apple Silicon Macs' machine-level sound system.
>>>> 
>>>> Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
>>>> ---
>>>> .../bindings/sound/apple,macaudio.yaml        | 103 ++++++++++++++++++
>>>> 1 file changed, 103 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>>> 
>>> 
>>> Commit title does not match subsystem.
>> 
>> Tell more please. I don’t see it.
> 
> git log --oneline  -- Documentation/devicetree/bindings/sound/
> 
> 
> Mark expects "ASoC: dt-bindings:"

Aha! Thanks.

>>>> diff --git a/Documentation/devicetree/bindings/sound/apple,macaudio.yaml b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>>> new file mode 100644
>>>> index 000000000000..a6380e4bdd1a
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/sound/apple,macaudio.yaml
>>>> @@ -0,0 +1,103 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/sound/apple,macaudio.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Apple Silicon Macs integrated sound system
>>>> +
>>>> +maintainers:
>>>> +  - Martin Povišer <povik+lin@cutebit.org>
>>>> +
>>> 
>>> Add description.
>>> 
>>>> +definitions:
>>> 
>>> This does not make code more readable.
>> 
>> Are you sure? It prevents duplication later on for ‘codec' and ‘cpu’.
> 
> That's true, but duplication is small, unless you think this will be
> extended. I guess it is a trade-off, but so far for few lines and just
> two users of such definition, I would prefer to duplicate. I don't have
> strong opinion, though.

OK

>> 
>>> 
>>>> +        $ref: /schemas/types.yaml#/definitions/uint32
>>>> +      link-name:
>>>> +        description: Name for the DAI link to present to users.
>>>> +        $ref: /schemas/types.yaml#/definitions/string
>>>> +      cpu:
>>>> +        $ref: "#/definitions/dai"
>>>> +      codec:
>>>> +        $ref: "#/definitions/dai"
>>> 
>>> missing maxItems for DAI phandles.
>> 
>> Well there’s not a maximum.
> 
> There should be some maximum of supported codecs. Hardware might have
> such constraints. If really unsure, choose some reasonable (small)
> amount. It could be later raised, if needed.

There are some constraints but technically not in the driver that binds
on this binding. I thought no limit is better than an arbitrary one, but
if the preference is to have one, I will add it, no problem.

> Best regards,
> Krzysztof

Best,
Martin

Re: [RFC PATCH 1/5] dt-bindings: sound: Add Apple Macs sound system

[Krzysztof Kozlowski]

On 31/03/2022 10:23, Martin Povišer wrote:
> 
>> There should be some maximum of supported codecs. Hardware might have
>> such constraints. If really unsure, choose some reasonable (small)
>> amount. It could be later raised, if needed.
> 
> There are some constraints but technically not in the driver that binds
> on this binding. I thought no limit is better than an arbitrary one, but
> if the preference is to have one, I will add it, no problem.

Just to clarify this - bindings are not about the driver, but about the
hardware. We model here the hardware and its programming model, not the
driver implementation (although of course it's always somehow related).
Hardware has some limitations for sure. The question is whether we know
them. :)

I prefer even arbitrary limit, because then schema will check DTSes for
simple mistakes. You can also explain this in commit msg, that maxItems
are arbitrary, so whoever in the future wants to change it, will know
the background.

Best regards,
Krzysztof

[RFC PATCH 2/5] HACK: ASoC: Add card->filter_controls hook

[Martin Povišer]

Add a new ASoC card callback for filtering the kcontrols of the card's
constituent components. This lets the card take over some of the
controls, deciding their value instead of leaving it up to userspace.

Also, and here's the HACK: part, move dapm_new_widgets call in front
of the card's late_probe call. This way all kcontrols should have been
created (and are safe to use) by the time late_probe is called.

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 include/sound/soc.h  |  3 +++
 sound/soc/soc-core.c | 45 +++++++++++++++++++++++++++-----------------
 sound/soc/soc-dapm.c | 34 ++++++++++++++++++++++++++++-----
 3 files changed, 60 insertions(+), 22 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 7a1650b303f1..0ab664500b8f 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -897,6 +897,9 @@ struct snd_soc_card {
 	int (*late_probe)(struct snd_soc_card *card);
 	int (*remove)(struct snd_soc_card *card);
 
+	int (*filter_controls)(struct snd_soc_card *card,
+			struct snd_kcontrol *kcontrol);
+
 	/* the pre and post PM functions are used to do any PM work before and
 	 * after the codec and DAI's do any PM work. */
 	int (*suspend_pre)(struct snd_soc_card *card);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a088bc9f7dd7..0bf05d98ec0d 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2069,12 +2069,12 @@ static int snd_soc_bind_card(struct snd_soc_card *card)
 		}
 	}
 
+	snd_soc_dapm_new_widgets(card);
+
 	ret = snd_soc_card_late_probe(card);
 	if (ret < 0)
 		goto probe_end;
 
-	snd_soc_dapm_new_widgets(card);
-
 	ret = snd_card_register(card->snd_card);
 	if (ret < 0) {
 		dev_err(card->dev, "ASoC: failed to register soundcard %d\n",
@@ -2209,19 +2209,34 @@ struct snd_kcontrol *snd_soc_cnew(const struct snd_kcontrol_new *_template,
 }
 EXPORT_SYMBOL_GPL(snd_soc_cnew);
 
-static int snd_soc_add_controls(struct snd_card *card, struct device *dev,
+static int snd_soc_add_controls(struct snd_soc_card *card, struct device *dev,
 	const struct snd_kcontrol_new *controls, int num_controls,
 	const char *prefix, void *data)
 {
-	int i;
+	int i, err;
 
 	for (i = 0; i < num_controls; i++) {
-		const struct snd_kcontrol_new *control = &controls[i];
-		int err = snd_ctl_add(card, snd_soc_cnew(control, data,
-							 control->name, prefix));
+		const struct snd_kcontrol_new *control_new = &controls[i];
+		struct snd_kcontrol *control;
+
+		control = snd_soc_cnew(control_new, data,
+					control_new->name, prefix);
+
+		if (card->filter_controls) {
+			err = card->filter_controls(card, control);
+			if (err < 0) {
+				snd_ctl_free_one(control);
+				return err;
+			} else if (err) {
+				continue;
+			}
+		}
+
+		err = snd_ctl_add(card->snd_card, control);
+
 		if (err < 0) {
 			dev_err(dev, "ASoC: Failed to add %s: %d\n",
-				control->name, err);
+				control_new->name, err);
 			return err;
 		}
 	}
@@ -2241,9 +2256,7 @@ static int snd_soc_add_controls(struct snd_card *card, struct device *dev,
 int snd_soc_add_component_controls(struct snd_soc_component *component,
 	const struct snd_kcontrol_new *controls, unsigned int num_controls)
 {
-	struct snd_card *card = component->card->snd_card;
-
-	return snd_soc_add_controls(card, component->dev, controls,
+	return snd_soc_add_controls(component->card, component->dev, controls,
 			num_controls, component->name_prefix, component);
 }
 EXPORT_SYMBOL_GPL(snd_soc_add_component_controls);
@@ -2258,13 +2271,11 @@ EXPORT_SYMBOL_GPL(snd_soc_add_component_controls);
  *
  * Return 0 for success, else error.
  */
-int snd_soc_add_card_controls(struct snd_soc_card *soc_card,
+int snd_soc_add_card_controls(struct snd_soc_card *card,
 	const struct snd_kcontrol_new *controls, int num_controls)
 {
-	struct snd_card *card = soc_card->snd_card;
-
-	return snd_soc_add_controls(card, soc_card->dev, controls, num_controls,
-			NULL, soc_card);
+	return snd_soc_add_controls(card, card->dev, controls, num_controls,
+			NULL, card);
 }
 EXPORT_SYMBOL_GPL(snd_soc_add_card_controls);
 
@@ -2281,7 +2292,7 @@ EXPORT_SYMBOL_GPL(snd_soc_add_card_controls);
 int snd_soc_add_dai_controls(struct snd_soc_dai *dai,
 	const struct snd_kcontrol_new *controls, int num_controls)
 {
-	struct snd_card *card = dai->component->card->snd_card;
+	struct snd_soc_card *card = dai->component->card;
 
 	return snd_soc_add_controls(card, dai->dev, controls, num_controls,
 			NULL, dai);
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index b06c5682445c..56ecbabe5453 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -873,7 +873,7 @@ static int dapm_create_or_share_kcontrol(struct snd_soc_dapm_widget *w,
 	int kci)
 {
 	struct snd_soc_dapm_context *dapm = w->dapm;
-	struct snd_card *card = dapm->card->snd_card;
+	struct snd_soc_card *card = dapm->card;
 	const char *prefix;
 	size_t prefix_len;
 	int shared;
@@ -957,7 +957,19 @@ static int dapm_create_or_share_kcontrol(struct snd_soc_dapm_widget *w,
 			goto exit_free;
 		}
 
-		ret = snd_ctl_add(card, kcontrol);
+		if (card->filter_controls) {
+			ret = card->filter_controls(card, kcontrol);
+			if (ret < 0) {
+				snd_ctl_free_one(kcontrol);
+				goto exit_free;
+			}
+
+			if (!ret)
+				ret = snd_ctl_add(card->snd_card, kcontrol);
+		} else {
+			ret = snd_ctl_add(card->snd_card, kcontrol);
+		}
+
 		if (ret < 0) {
 			dev_err(dapm->dev,
 				"ASoC: failed to add widget %s dapm kcontrol %s: %d\n",
@@ -1074,7 +1086,7 @@ static int dapm_new_pga(struct snd_soc_dapm_widget *w)
 /* create new dapm dai link control */
 static int dapm_new_dai_link(struct snd_soc_dapm_widget *w)
 {
-	int i;
+	int i, ret;
 	struct snd_soc_pcm_runtime *rtd = w->priv;
 
 	/* create control for links with > 1 config */
@@ -1084,10 +1096,22 @@ static int dapm_new_dai_link(struct snd_soc_dapm_widget *w)
 	/* add kcontrol */
 	for (i = 0; i < w->num_kcontrols; i++) {
 		struct snd_soc_dapm_context *dapm = w->dapm;
-		struct snd_card *card = dapm->card->snd_card;
+		struct snd_soc_card *card = dapm->card;
 		struct snd_kcontrol *kcontrol = snd_soc_cnew(&w->kcontrol_news[i],
 							     w, w->name, NULL);
-		int ret = snd_ctl_add(card, kcontrol);
+
+		if (card->filter_controls) {
+			ret = card->filter_controls(card, kcontrol);
+			if (ret < 0) {
+				snd_ctl_free_one(kcontrol);
+				return ret;
+			}
+
+			if (!ret)
+				ret = snd_ctl_add(card->snd_card, kcontrol);
+		} else {
+			ret = snd_ctl_add(card->snd_card, kcontrol);
+		}
 
 		if (ret < 0) {
 			dev_err(dapm->dev,
-- 
2.33.0

Re: [RFC PATCH 2/5] HACK: ASoC: Add card->filter_controls hook

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 890 bytes --]

On Thu, Mar 31, 2022 at 02:04:46AM +0200, Martin Povišer wrote:

> Add a new ASoC card callback for filtering the kcontrols of the card's
> constituent components. This lets the card take over some of the
> controls, deciding their value instead of leaving it up to userspace.

Define "filter".  What is this trying to accomplish?  As a matter of
policy we don't put use case configuration in the kernel, the goal is to
avoid having to update the kernel when people decide to do new things
with their userspace.

> Also, and here's the HACK: part, move dapm_new_widgets call in front
> of the card's late_probe call. This way all kcontrols should have been
> created (and are safe to use) by the time late_probe is called.

This will break any card that adds new controls, you could add a second
call earlier but deleting the existing call is going to break other
users.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Martin Povišer]

Tolerate N-to-M DAI links while using the first CPU DAI to decide
playback/capture abilities.

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/soc-pcm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 9a954680d492..770cf367a147 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2781,9 +2781,12 @@ static int soc_get_playback_capture(struct snd_soc_pcm_runtime *rtd,
 			} else if (rtd->num_cpus == rtd->num_codecs) {
 				cpu_dai = asoc_rtd_to_cpu(rtd, i);
 			} else {
+#if 0
 				dev_err(rtd->card->dev,
 					"N cpus to M codecs link is not supported yet\n");
 				return -EINVAL;
+#endif
+				cpu_dai = asoc_rtd_to_cpu(rtd, 0);
 			}
 
 			if (snd_soc_dai_stream_valid(codec_dai, SNDRV_PCM_STREAM_PLAYBACK) &&
-- 
2.33.0

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 720 bytes --]

On Thu, Mar 31, 2022 at 02:04:47AM +0200, Martin Povišer wrote:

> +#if 0
>  				dev_err(rtd->card->dev,
>  					"N cpus to M codecs link is not supported yet\n");
>  				return -EINVAL;
> +#endif
> +				cpu_dai = asoc_rtd_to_cpu(rtd, 0);

We need to figure out an interface for describing which CODEC/CPU
combinations are connected to each other.  I'm not seeing a great way to
do that right now, probably some side data table is going to be needed,
or perhaps the CPU DAI drivers can be persuaded to only have one DAI
actually register and claim to support more channels?  I'm not sure how
a configuraiton like this is going to work at userspace level if the
multiple CPU DAIs end up being visible...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Martin Povišer]

> On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 02:04:47AM +0200, Martin Povišer wrote:
> 
>> +#if 0
>> 				dev_err(rtd->card->dev,
>> 					"N cpus to M codecs link is not supported yet\n");
>> 				return -EINVAL;
>> +#endif
>> +				cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> 
> We need to figure out an interface for describing which CODEC/CPU
> combinations are connected to each other.  I'm not seeing a great way to
> do that right now, probably some side data table is going to be needed,
> or perhaps the CPU DAI drivers can be persuaded to only have one DAI
> actually register and claim to support more channels?  I'm not sure how
> a configuraiton like this is going to work at userspace level if the
> multiple CPU DAIs end up being visible...

To understand the issue better: How could the multiple CPU DAIs be
visible from userspace?

What about this interim solution: In case of N-to-M links we put in
the most restrictive condition for checking capture/playback stream
validity: we check all of the CPU DAIs. Whatever ends up being the
proper solution later can only be less restrictive than this.

As a reminder what happens on the Macs: the platform driver drives
all the CPU-side I2S ports that belong to the link with the same data,
so the particular CPU/CODEC wiring doesn’t matter.

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On Fri, Apr 22, 2022 at 04:06:06PM +0200, Martin Povišer wrote:
> > On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote:

> > We need to figure out an interface for describing which CODEC/CPU
> > combinations are connected to each other.  I'm not seeing a great way to
> > do that right now, probably some side data table is going to be needed,
> > or perhaps the CPU DAI drivers can be persuaded to only have one DAI
> > actually register and claim to support more channels?  I'm not sure how
> > a configuraiton like this is going to work at userspace level if the
> > multiple CPU DAIs end up being visible...

> To understand the issue better: How could the multiple CPU DAIs be
> visible from userspace?

If you register two separate DAIs (well, links) with the API without
doing anything else the API will just expose them to userspace as two
separate things with no indication that they're related.

> What about this interim solution: In case of N-to-M links we put in
> the most restrictive condition for checking capture/playback stream
> validity: we check all of the CPU DAIs. Whatever ends up being the
> proper solution later can only be less restrictive than this.

That's not the issue here?  

> As a reminder what happens on the Macs: the platform driver drives
> all the CPU-side I2S ports that belong to the link with the same data,
> so the particular CPU/CODEC wiring doesn’t matter.

Oh, that's not something I was aware of.  In that case this is the wrong
API - you should be using DPCM to map one front end onto multiple back
ends (Kirkwood does something similar IIRC, there will be other examples
but that's probably the simplest).  The back ends probably don't really
need to know that they're on the same physical bus (if indeed they are).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Martin Povišer]

> On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 04:06:06PM +0200, Martin Povišer wrote:
>>> On 4. 4. 2022, at 14:28, Mark Brown <broonie@kernel.org> wrote:
> 
>>> We need to figure out an interface for describing which CODEC/CPU
>>> combinations are connected to each other.  I'm not seeing a great way to
>>> do that right now, probably some side data table is going to be needed,
>>> or perhaps the CPU DAI drivers can be persuaded to only have one DAI
>>> actually register and claim to support more channels?  I'm not sure how
>>> a configuraiton like this is going to work at userspace level if the
>>> multiple CPU DAIs end up being visible...
> 
>> To understand the issue better: How could the multiple CPU DAIs be
>> visible from userspace?
> 
> If you register two separate DAIs (well, links) with the API without
> doing anything else the API will just expose them to userspace as two
> separate things with no indication that they're related.

Sure, but what I am addressing here is a single DAI link with multiple
CPU DAIs, invoked in DT like this:

	dai-link@0 {
		link-name = "Speakers";
		mclk-fs = <256>;

		cpu {
			sound-dai = <&mca 0>, <&mca 1>;
		};
		codec {
			sound-dai = <&speaker_left_woof1>,
				<&speaker_right_woof1>,
				<&speaker_left_tweet>,
				<&speaker_right_tweet>,
				<&speaker_left_woof2>,
				<&speaker_right_woof2>;
		};
	};

>> What about this interim solution: In case of N-to-M links we put in
>> the most restrictive condition for checking capture/playback stream
>> validity: we check all of the CPU DAIs. Whatever ends up being the
>> proper solution later can only be less restrictive than this.
> 
> That's not the issue here?

Well to me it looks like it is. Because if I invoke the DAI link like
I quoted above, and the platform driver supports it, the playback/capture
stream validity check is the only place it breaks down. Notwithstanding
this may be the wrong API as you wrote.

>> As a reminder what happens on the Macs: the platform driver drives
>> all the CPU-side I2S ports that belong to the link with the same data,
>> so the particular CPU/CODEC wiring doesn’t matter.
> 
> Oh, that's not something I was aware of.  In that case this is the wrong
> API - you should be using DPCM to map one front end onto multiple back
> ends (Kirkwood does something similar IIRC, there will be other examples
> but that's probably the simplest).  The back ends probably don't really
> need to know that they're on the same physical bus (if indeed they are).

I guess I need to look into that.

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On Mon, Apr 25, 2022 at 02:34:33PM +0200, Martin Povišer wrote:
> > On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote:

> > If you register two separate DAIs (well, links) with the API without
> > doing anything else the API will just expose them to userspace as two
> > separate things with no indication that they're related.

> Sure, but what I am addressing here is a single DAI link with multiple
> CPU DAIs, invoked in DT like this:

> 	dai-link@0 {
> 		link-name = "Speakers";
> 		mclk-fs = <256>;
> 
> 		cpu {
> 			sound-dai = <&mca 0>, <&mca 1>;
> 		};
> 		codec {
> 			sound-dai = <&speaker_left_woof1>,
> 				<&speaker_right_woof1>,
> 				<&speaker_left_tweet>,
> 				<&speaker_right_tweet>,
> 				<&speaker_left_woof2>,
> 				<&speaker_right_woof2>;
> 		};
> 	};

You could parse this into two separate links for the benefit of the
framewokr if you're using a custom machine driver (which I suspect you
probably have to).

> >> What about this interim solution: In case of N-to-M links we put in
> >> the most restrictive condition for checking capture/playback stream
> >> validity: we check all of the CPU DAIs. Whatever ends up being the
> >> proper solution later can only be less restrictive than this.

> > That's not the issue here?

> Well to me it looks like it is. Because if I invoke the DAI link like
> I quoted above, and the platform driver supports it, the playback/capture
> stream validity check is the only place it breaks down. Notwithstanding
> this may be the wrong API as you wrote.

I am surprised that doesn't otherwise explode TBH - at the very least
I'd expect it to show two PCMs to userspace which if I'm understanding
your description correctly isn't really what's going on.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Martin Povišer]

> On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Apr 25, 2022 at 02:34:33PM +0200, Martin Povišer wrote:
>>> On 25. 4. 2022, at 14:25, Mark Brown <broonie@kernel.org> wrote:
> 
>>> If you register two separate DAIs (well, links) with the API without
>>> doing anything else the API will just expose them to userspace as two
>>> separate things with no indication that they're related.
> 
>> Sure, but what I am addressing here is a single DAI link with multiple
>> CPU DAIs, invoked in DT like this:
> 
>> 	dai-link@0 {
>> 		link-name = "Speakers";
>> 		mclk-fs = <256>;
>> 
>> 		cpu {
>> 			sound-dai = <&mca 0>, <&mca 1>;
>> 		};
>> 		codec {
>> 			sound-dai = <&speaker_left_woof1>,
>> 				<&speaker_right_woof1>,
>> 				<&speaker_left_tweet>,
>> 				<&speaker_right_tweet>,
>> 				<&speaker_left_woof2>,
>> 				<&speaker_right_woof2>;
>> 		};
>> 	};
> 
> You could parse this into two separate links for the benefit of the
> framewokr if you're using a custom machine driver (which I suspect you
> probably have to).

Yeah, this is parsed by the ‘macaudio’ machine driver from the series.

>>>> What about this interim solution: In case of N-to-M links we put in
>>>> the most restrictive condition for checking capture/playback stream
>>>> validity: we check all of the CPU DAIs. Whatever ends up being the
>>>> proper solution later can only be less restrictive than this.
> 
>>> That's not the issue here?
> 
>> Well to me it looks like it is. Because if I invoke the DAI link like
>> I quoted above, and the platform driver supports it, the playback/capture
>> stream validity check is the only place it breaks down. Notwithstanding
>> this may be the wrong API as you wrote.
> 
> I am surprised that doesn't otherwise explode TBH - at the very least
> I'd expect it to show two PCMs to userspace which if I'm understanding
> your description correctly isn't really what's going on.

I fill in a single snd_soc_dai_link, it exposes a single PCM and works
like a charm. That is as long as I patch the playback/capture check in
question.

I read that to be the clear intention of ASoC code: a DAI link becomes
one snd_soc_pcm_runtime.

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

On Mon, Apr 25, 2022 at 03:11:14PM +0200, Martin Povišer wrote:
> > On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote:

> > I am surprised that doesn't otherwise explode TBH - at the very least
> > I'd expect it to show two PCMs to userspace which if I'm understanding
> > your description correctly isn't really what's going on.

> I fill in a single snd_soc_dai_link, it exposes a single PCM and works
> like a charm. That is as long as I patch the playback/capture check in
> question.

> I read that to be the clear intention of ASoC code: a DAI link becomes
> one snd_soc_pcm_runtime.

Yes, so long as you boil it down to a single link it works fine but the
bit on top of the binding where you tie the two CPU DAIs to what is
actually exposed is all in code.  The reason this stuff isn't filled in
is that connecting the thing that applications see to the physical links
isn't at all obvious and needs at least some driver sitting in the
middle to make the links - I'd imagine there's a DSP sitting there which
probably has quite a bit of flexability about how the various hardware
components available are actually related.  This makes figuring out what
to do with the relationship between the multiple CPU DAIs hard.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 3/5] HACK: ASoC: Tolerate N-cpus-to-M-codecs links

[Martin Povišer]

> On 25. 4. 2022, at 15:46, Mark Brown <broonie@kernel.org> wrote:
> 
> On Mon, Apr 25, 2022 at 03:11:14PM +0200, Martin Povišer wrote:
>>> On 25. 4. 2022, at 14:55, Mark Brown <broonie@kernel.org> wrote:
> 
>>> I am surprised that doesn't otherwise explode TBH - at the very least
>>> I'd expect it to show two PCMs to userspace which if I'm understanding
>>> your description correctly isn't really what's going on.
> 
>> I fill in a single snd_soc_dai_link, it exposes a single PCM and works
>> like a charm. That is as long as I patch the playback/capture check in
>> question.
> 
>> I read that to be the clear intention of ASoC code: a DAI link becomes
>> one snd_soc_pcm_runtime.
> 
> Yes, so long as you boil it down to a single link it works fine but the
> bit on top of the binding where you tie the two CPU DAIs to what is
> actually exposed is all in code.  The reason this stuff isn't filled in
> is that connecting the thing that applications see to the physical links
> isn't at all obvious and needs at least some driver sitting in the
> middle to make the links - I'd imagine there's a DSP sitting there which
> probably has quite a bit of flexability about how the various hardware
> components available are actually related.  This makes figuring out what
> to do with the relationship between the multiple CPU DAIs hard.

I get the gist. Anyway unless you tell me otherwise I will assume I need
to move to DPCM with the platform/machine driver.

[RFC PATCH 4/5] ASoC: Introduce snd_soc_of_get_dai_link_cpus

[Martin Povišer]

This function is an analogue of snd_soc_of_get_dai_link_codecs to help
machine drivers read CPU DAI lists from devicetrees.

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 include/sound/soc.h  |  4 +++
 sound/soc/soc-core.c | 80 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 0ab664500b8f..0bf9d1d0768c 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1266,6 +1266,10 @@ int snd_soc_of_get_dai_link_codecs(struct device *dev,
 				   struct device_node *of_node,
 				   struct snd_soc_dai_link *dai_link);
 void snd_soc_of_put_dai_link_codecs(struct snd_soc_dai_link *dai_link);
+int snd_soc_of_get_dai_link_cpus(struct device *dev,
+				 struct device_node *of_node,
+				 struct snd_soc_dai_link *dai_link);
+void snd_soc_of_put_dai_link_cpus(struct snd_soc_dai_link *dai_link);
 
 int snd_soc_add_pcm_runtime(struct snd_soc_card *card,
 			    struct snd_soc_dai_link *dai_link);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 0bf05d98ec0d..a97476ec01ab 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3400,6 +3400,86 @@ int snd_soc_of_get_dai_link_codecs(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(snd_soc_of_get_dai_link_codecs);
 
+/*
+ * snd_soc_of_put_dai_link_cpus - Dereference device nodes in the codecs array
+ * @dai_link: DAI link
+ *
+ * Dereference device nodes acquired by snd_soc_of_get_dai_link_cpus().
+ */
+void snd_soc_of_put_dai_link_cpus(struct snd_soc_dai_link *dai_link)
+{
+	struct snd_soc_dai_link_component *component;
+	int index;
+
+	for_each_link_cpus(dai_link, index, component) {
+		if (!component->of_node)
+			break;
+		of_node_put(component->of_node);
+		component->of_node = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(snd_soc_of_put_dai_link_cpus);
+
+/*
+ * snd_soc_of_get_dai_link_cpus - Parse a list of CPU DAIs in the devicetree
+ * @dev: Card device
+ * @of_node: Device node
+ * @dai_link: DAI link
+ *
+ * Is analogous to snd_soc_of_get_dai_link_codecs but parses a list of CPU DAIs
+ * instead.
+ *
+ * Returns 0 for success
+ */
+int snd_soc_of_get_dai_link_cpus(struct device *dev,
+				 struct device_node *of_node,
+				 struct snd_soc_dai_link *dai_link)
+{
+	struct of_phandle_args args;
+	struct snd_soc_dai_link_component *component;
+	char *name;
+	int index, num_codecs, ret;
+
+	/* Count the number of CODECs */
+	name = "sound-dai";
+	num_codecs = of_count_phandle_with_args(of_node, name,
+						"#sound-dai-cells");
+	if (num_codecs <= 0) {
+		if (num_codecs == -ENOENT)
+			dev_err(dev, "No 'sound-dai' property\n");
+		else
+			dev_err(dev, "Bad phandle in 'sound-dai'\n");
+		return num_codecs;
+	}
+	component = devm_kcalloc(dev,
+				 num_codecs, sizeof(*component),
+				 GFP_KERNEL);
+	if (!component)
+		return -ENOMEM;
+	dai_link->cpus = component;
+	dai_link->num_cpus = num_codecs;
+
+	/* Parse the list */
+	for_each_link_cpus(dai_link, index, component) {
+		ret = of_parse_phandle_with_args(of_node, name,
+						 "#sound-dai-cells",
+						 index, &args);
+		if (ret)
+			goto err;
+		component->of_node = args.np;
+		ret = snd_soc_get_dai_name(&args, &component->dai_name);
+		if (ret < 0)
+			goto err;
+	}
+	return 0;
+err:
+	snd_soc_of_put_dai_link_codecs(dai_link);
+	dai_link->cpus = NULL;
+	dai_link->num_cpus = 0;
+	return ret;
+}
+EXPORT_SYMBOL_GPL(snd_soc_of_get_dai_link_cpus);
+
 static int __init snd_soc_init(void)
 {
 	snd_soc_debugfs_init();
-- 
2.33.0

[RFC PATCH 5/5] ASoC: Add macaudio machine driver

[Martin Povišer]

Add ASoC machine driver for Apple Silicon Macs.

Signed-off-by: Martin Povišer <povik+lin@cutebit.org>
---
 sound/soc/apple/Kconfig    |  10 +
 sound/soc/apple/Makefile   |   3 +
 sound/soc/apple/macaudio.c | 597 +++++++++++++++++++++++++++++++++++++
 3 files changed, 610 insertions(+)
 create mode 100644 sound/soc/apple/Kconfig
 create mode 100644 sound/soc/apple/Makefile
 create mode 100644 sound/soc/apple/macaudio.c

diff --git a/sound/soc/apple/Kconfig b/sound/soc/apple/Kconfig
new file mode 100644
index 000000000000..afc0243b9309
--- /dev/null
+++ b/sound/soc/apple/Kconfig
@@ -0,0 +1,10 @@
+config SND_SOC_APPLE_MACAUDIO
+	tristate "ASoC machine driver for Apple Silicon Macs"
+	depends on ARCH_APPLE || COMPILE_TEST
+	select SND_SOC_APPLE_MCA
+	select SND_SIMPLE_CARD_UTILS
+	select APPLE_ADMAC
+	select COMMON_CLK_APPLE_NCO
+	default ARCH_APPLE
+	help
+	  This option enables an ASoC machine driver for Apple Silicon Macs.
diff --git a/sound/soc/apple/Makefile b/sound/soc/apple/Makefile
new file mode 100644
index 000000000000..d7a2df6311b5
--- /dev/null
+++ b/sound/soc/apple/Makefile
@@ -0,0 +1,3 @@
+snd-soc-macaudio-objs	:= macaudio.o
+
+obj-$(CONFIG_SND_SOC_APPLE_MACAUDIO)	+= snd-soc-macaudio.o
diff --git a/sound/soc/apple/macaudio.c b/sound/soc/apple/macaudio.c
new file mode 100644
index 000000000000..3e80f97a9b75
--- /dev/null
+++ b/sound/soc/apple/macaudio.c
@@ -0,0 +1,597 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ASoC machine driver for Apple Silicon Macs
+ *
+ * Copyright (C) The Asahi Linux Contributors
+ *
+ * Based on sound/soc/qcom/{sc7180.c|common.c}
+ *
+ * Copyright (c) 2018, Linaro Limited.
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/pcm.h>
+#include <sound/simple_card_utils.h>
+#include <sound/soc.h>
+#include <uapi/linux/input-event-codes.h>
+
+#define DRIVER_NAME "snd-soc-macaudio"
+
+struct macaudio_snd_data {
+	struct snd_soc_card card;
+	struct snd_soc_jack_pin pin;
+	struct snd_soc_jack jack;
+
+	struct macaudio_link_props {
+		unsigned int mclk_fs;
+	} *link_props;
+
+	const struct snd_pcm_chmap_elem *speaker_chmap;
+
+	unsigned int speaker_nchans_array[2];
+	struct snd_pcm_hw_constraint_list speaker_nchans_list;
+
+	struct list_head hidden_kcontrols;
+};
+
+static int macaudio_parse_of(struct macaudio_snd_data *ma, struct snd_soc_card *card)
+{
+	struct device_node *np;
+	struct device_node *codec = NULL;
+	struct device_node *cpu = NULL;
+	struct device *dev = card->dev;
+	struct snd_soc_dai_link *link;
+	struct macaudio_link_props *link_props;
+	int ret, num_links;
+	int i = 0;
+
+	ret = snd_soc_of_parse_card_name(card, "model");
+	if (ret) {
+		dev_err(dev, "Error parsing card name: %d\n", ret);
+		return ret;
+	}
+
+	ret = asoc_simple_parse_routing(card, NULL);
+	if (ret)
+		return ret;
+
+	/* Populate links */
+	num_links = of_get_available_child_count(dev->of_node);
+
+	/* Allocate the DAI link array */
+	card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link), GFP_KERNEL);
+	ma->link_props = devm_kcalloc(dev, num_links, sizeof(*ma->link_props), GFP_KERNEL);
+	if (!card->dai_link || !ma->link_props)
+		return -ENOMEM;
+
+	card->num_links = num_links;
+	link = card->dai_link;
+	link_props = ma->link_props;
+
+	for_each_available_child_of_node(dev->of_node, np) {
+		link->id = i++;
+
+		/* CPU side is bit and frame clock master, I2S with both clocks inverted */
+		link->dai_fmt = SND_SOC_DAIFMT_I2S |
+			SND_SOC_DAIFMT_CBC_CFC |
+			SND_SOC_DAIFMT_GATED |
+			SND_SOC_DAIFMT_IB_IF;
+
+		ret = of_property_read_string(np, "link-name", &link->name);
+		if (ret) {
+			dev_err(card->dev, "Missing link name\n");
+			goto err_put_np;
+		}
+
+		cpu = of_get_child_by_name(np, "cpu");
+		codec = of_get_child_by_name(np, "codec");
+
+		if (!codec || !cpu) {
+			dev_err(dev, "Missing DAI specifications for '%s'\n", link->name);
+			ret = -EINVAL;
+			goto err;
+		}
+
+		ret = snd_soc_of_get_dai_link_codecs(dev, codec, link);
+		if (ret < 0) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(card->dev, "%s: codec dai not found: %d\n",
+					link->name, ret);
+			goto err;
+		}
+
+		ret = snd_soc_of_get_dai_link_cpus(dev, cpu, link);
+		if (ret < 0) {
+			if (ret != -EPROBE_DEFER)
+				dev_err(card->dev, "%s: cpu dai not found: %d\n",
+					link->name, ret);
+			goto err;
+		}
+
+		link->num_platforms = 1;
+		link->platforms	= devm_kzalloc(dev, sizeof(*link->platforms),
+						GFP_KERNEL);
+		if (!link->platforms) {
+			ret = -ENOMEM;
+			goto err;
+		}
+		link->platforms->of_node = link->cpus->of_node;
+
+		of_property_read_u32(np, "mclk-fs", &link_props->mclk_fs);
+
+		link->stream_name = link->name;
+		link++;
+		link_props++;
+
+		of_node_put(cpu);
+		of_node_put(codec);
+	}
+
+	/*
+	 * TODO: Not sure I shouldn't do something about the ->of_node component
+	 * references I leave in dai_link (if successful here).
+	 */
+
+	return 0;
+err:
+	of_node_put(cpu);
+	of_node_put(codec);
+err_put_np:
+	for (i = 0; i < num_links; i++) {
+		snd_soc_of_put_dai_link_codecs(&card->dai_link[i]);
+		snd_soc_of_put_dai_link_cpus(&card->dai_link[i]);
+	}
+	of_node_put(np);
+	return ret;
+}
+
+static int macaudio_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(rtd->card);
+	struct macaudio_link_props *props = &ma->link_props[rtd->num];
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	struct snd_soc_dai *dai;
+	int i, mclk;
+
+	if (props->mclk_fs) {
+		mclk = params_rate(params) * props->mclk_fs;
+
+		for_each_rtd_codec_dais(rtd, i, dai)
+			snd_soc_dai_set_sysclk(dai, 0, mclk, SND_SOC_CLOCK_IN);
+
+		snd_soc_dai_set_sysclk(cpu_dai, 0, mclk, SND_SOC_CLOCK_OUT);
+	}
+
+	return 0;
+}
+
+static void macaudio_shutdown(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(rtd->card);
+	struct macaudio_link_props *props = &ma->link_props[rtd->num];
+	struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	struct snd_soc_dai *dai;
+	int i;
+
+	if (props->mclk_fs) {
+		for_each_rtd_codec_dais(rtd, i, dai)
+			snd_soc_dai_set_sysclk(dai, 0, 0, SND_SOC_CLOCK_IN);
+
+		snd_soc_dai_set_sysclk(cpu_dai, 0, 0, SND_SOC_CLOCK_OUT);
+	}
+}
+
+static bool macaudio_is_speakers(struct snd_soc_dai_link *dai_link)
+{
+	return !strcmp(rtd->dai_link->name, "Speaker")
+		|| !strcmp(rtd->dai_link->name, "Speakers");
+}
+
+static int macaudio_startup(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
+	struct snd_soc_card *card = rtd->card;
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
+	struct snd_pcm_hw_constraint_list *nchans_list = &ma->speaker_nchans_list;
+	unsigned int *nchans_array = ma->speaker_nchans_array;
+	int ret;
+
+	if (macaudio_is_speakers(rtd->dai_link)) {
+		if (rtd->num_codecs > 2) {
+			nchans_list->count = 2;
+			nchans_list->list = nchans_array;
+			nchans_array[0] = 2;
+			nchans_array[1] = rtd->num_codecs;
+
+			ret = snd_pcm_hw_constraint_list(substream->runtime, 0,
+					SNDRV_PCM_HW_PARAM_CHANNELS, nchans_list);
+			if (ret < 0)
+				return ret;
+		} else if (rtd->num_codecs == 2) {
+			ret = snd_pcm_hw_constraint_single(substream->runtime,
+					SNDRV_PCM_HW_PARAM_CHANNELS, 2);
+			if (ret < 0)
+				return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int macaudio_assign_tdm(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_card *card = rtd->card;
+	struct snd_soc_dai *dai, *cpu_dai;
+	int ret, i;
+	int nchans = 0, nslots = 0, slot_width = 32;
+
+	nslots = rtd->num_codecs;
+
+	for_each_rtd_codec_dais(rtd, i, dai) {
+		int codec_nchans = 1;
+		int mask = ((1 << codec_nchans) - 1) << nchans;
+
+		ret = snd_soc_dai_set_tdm_slot(dai, mask,
+					mask, nslots, slot_width);
+		if (ret == -EINVAL)
+			/* Try without the RX mask */
+			ret = snd_soc_dai_set_tdm_slot(dai, mask,
+					0, nslots, slot_width);
+
+		if (ret < 0) {
+			dev_err(card->dev, "DAI %s refuses TDM settings: %d",
+					dai->name, ret);
+			return ret;
+		}
+
+		nchans += codec_nchans;
+	}
+
+	cpu_dai = asoc_rtd_to_cpu(rtd, 0);
+	ret = snd_soc_dai_set_tdm_slot(cpu_dai, (1 << nslots) - 1,
+			(1 << nslots) - 1, nslots, slot_width);
+	if (ret < 0) {
+		dev_err(card->dev, "CPU DAI %s refuses TDM settings: %d",
+				cpu_dai->name, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int macaudio_init(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_card *card = rtd->card;
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
+	struct snd_soc_component *component;
+	int ret, i;
+
+	if (rtd->num_codecs > 1) {
+		ret = macaudio_assign_tdm(rtd);
+		if (ret < 0)
+			return ret;
+	}
+
+	for_each_rtd_components(rtd, i, component)
+		snd_soc_component_set_jack(component, &ma->jack, NULL);
+
+	return 0;
+}
+
+static void macaudio_exit(struct snd_soc_pcm_runtime *rtd)
+{
+	struct snd_soc_component *component;
+	int i;
+
+	for_each_rtd_components(rtd, i, component)
+		snd_soc_component_set_jack(component, NULL, NULL);
+}
+
+struct macaudio_kctlfix {
+	char *name;
+	char *value;
+} macaudio_kctlfixes[] = {
+	{"* ASI1 Sel", "Left"},
+	{"* ISENSE Switch", "Off"},
+	{"* VSENSE Switch", "Off"},
+	{ }
+};
+
+static bool macaudio_kctlfix_matches(const char *pattern, const char *name)
+{
+	if (pattern[0] == '*') {
+		int namelen, patternlen;
+
+		pattern++;
+		if (pattern[0] == ' ')
+			pattern++;
+
+		namelen = strlen(name);
+		patternlen = strlen(pattern);
+
+		if (namelen > patternlen)
+			name += (namelen - patternlen);
+	}
+
+	return !strcmp(name, pattern);
+}
+
+static struct macaudio_kctlfix *macaudio_find_kctlfix(const char *name)
+{
+	struct macaudio_kctlfix *fctl;
+
+	for (fctl = macaudio_kctlfixes; fctl->name != NULL; fctl++)
+		if (macaudio_kctlfix_matches(fctl->name, name))
+			return fctl;
+
+	return NULL;
+}
+
+static int macaudio_probe(struct snd_soc_card *card)
+{
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
+	int ret;
+
+	INIT_LIST_HEAD(&ma->hidden_kcontrols);
+
+	ma->pin.pin = "Headphones";
+	ma->pin.mask = SND_JACK_HEADSET | SND_JACK_HEADPHONE;
+	ret = snd_soc_card_jack_new(card, ma->pin.pin,
+			SND_JACK_HEADSET |
+			SND_JACK_HEADPHONE |
+			SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+			SND_JACK_BTN_2 | SND_JACK_BTN_3,
+			&ma->jack, &ma->pin, 1);
+
+	if (ret < 0)
+		dev_err(card->dev, "jack creation failed: %d\n", ret);
+
+	return ret;
+}
+
+/*
+ * Maybe this could be a general ASoC function?
+ */
+static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card,
+				struct snd_kcontrol *kcontrol, const char *strvalue)
+{
+	struct snd_ctl_elem_value value;
+	struct snd_ctl_elem_info info;
+	int sel, i, ret;
+
+	ret = kcontrol->info(kcontrol, &info);
+	if (ret < 0) {
+		dev_err(card->dev, "can't obtain info on control '%s': %d",
+			kcontrol->id.name, ret);
+		return;
+	}
+
+	switch (info.type) {
+	case SNDRV_CTL_ELEM_TYPE_ENUMERATED:
+		for (sel = 0; sel < info.value.enumerated.items; sel++) {
+			info.value.enumerated.item = sel;
+			kcontrol->info(kcontrol, &info);
+
+			if (!strcmp(strvalue, info.value.enumerated.name))
+				break;
+		}
+
+		if (sel == info.value.enumerated.items)
+			goto not_avail;
+
+		for (i = 0; i < info.count; i++)
+			value.value.enumerated.item[i] = sel;
+		break;
+
+	case SNDRV_CTL_ELEM_TYPE_BOOLEAN:
+		sel = !strcmp(strvalue, "On");
+
+		if (!sel && strcmp(strvalue, "Off"))
+			goto not_avail;
+
+		for (i = 0; i < info.count; i++)
+			value.value.integer.value[i] = sel;
+		break;
+
+	case SNDRV_CTL_ELEM_TYPE_INTEGER:
+		if (kstrtoint(strvalue, 10, &sel))
+			goto not_avail;
+
+		for (i = 0; i < info.count; i++)
+			value.value.integer.value[i] = sel;
+		break;
+
+	default:
+		dev_err(card->dev, "%s: control '%s' has unsupported type %d",
+			__func__, kcontrol->id.name, info.type);
+		return;
+	}
+
+	ret = kcontrol->put(kcontrol, &value);
+	if (ret < 0) {
+		dev_err(card->dev, "can't set control '%s' to '%s': %d",
+			kcontrol->id.name, strvalue, ret);
+		return;
+	}
+
+	dev_dbg(card->dev, "set '%s' to '%s'",
+			kcontrol->id.name, strvalue);
+	return;
+
+not_avail:
+	dev_err(card->dev, "option '%s' on control '%s' not available",
+			strvalue, kcontrol->id.name);
+	return;
+
+}
+
+static int macaudio_filter_controls(struct snd_soc_card *card,
+			 struct snd_kcontrol *kcontrol)
+{
+	struct macaudio_kctlfix *fctl = macaudio_find_kctlfix(kcontrol->id.name);
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
+
+	dev_dbg(card->dev, "visiting control %s, have match %d\n",
+		kcontrol->id.name, !!fctl);
+
+	if (!fctl)
+		return 0;
+
+	list_add_tail(&kcontrol->list, &ma->hidden_kcontrols);
+	return 1;
+}
+
+static int macaudio_late_probe(struct snd_soc_card *card)
+{
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
+	struct snd_kcontrol *kcontrol;
+	struct snd_soc_pcm_runtime *rtd;
+	int ret;
+
+	/*
+	 * Here we take it to be okay to fiddle with the kcontrols
+	 * we caught for ourselves.
+	 */
+	list_for_each_entry(kcontrol, &ma->hidden_kcontrols, list) {
+		struct macaudio_kctlfix *fctl = macaudio_find_kctlfix(kcontrol->id.name);
+
+		if (fctl)
+			snd_soc_kcontrol_set_strval(card, kcontrol, fctl->value);
+	}
+
+	for_each_card_rtds(card, rtd) {
+		if (macaudio_is_speakers(rtd->dai_link) && ma->speaker_chmap) {
+			ret = snd_pcm_add_chmap_ctls(rtd->pcm,
+				SNDRV_PCM_STREAM_PLAYBACK, ma->speaker_chmap,
+				rtd->num_codecs, 0, NULL);
+			if (ret < 0)
+				dev_err(card->dev, "failed to add channel map on '%s': %d\n",
+					rtd->dai_link->name, ret);
+		}
+	}
+
+	return 0;
+}
+
+static int macaudio_remove(struct snd_soc_card *card)
+{
+	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
+	struct snd_kcontrol *kcontrol;
+
+	list_for_each_entry(kcontrol, &ma->hidden_kcontrols, list)
+		snd_ctl_free_one(kcontrol);
+
+	return 0;
+}
+
+static const struct snd_soc_ops macaudio_ops = {
+	.startup	= macaudio_startup,
+	.shutdown	= macaudio_shutdown,
+	.hw_params	= macaudio_hw_params,
+};
+
+static const struct snd_soc_dapm_widget macaudio_snd_widgets[] = {
+	SND_SOC_DAPM_HP("Headphones", NULL),
+};
+
+static const struct snd_pcm_chmap_elem macaudio_j274_chmaps[] = {
+	{ .channels = 1,
+	  .map = { SNDRV_CHMAP_MONO } },
+	{ }
+};
+
+static const struct snd_pcm_chmap_elem macaudio_j293_chmaps[] = {
+	{ .channels = 2,
+	  .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_FR } },
+	{ .channels = 4,
+	  .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_FR,
+		   SNDRV_CHMAP_RL, SNDRV_CHMAP_RR } },
+	{ }
+};
+
+static const struct snd_pcm_chmap_elem macaudio_j314_chmaps[] = {
+	{ .channels = 2,
+	  .map = { SNDRV_CHMAP_FL, SNDRV_CHMAP_FR } },
+	{ .channels = 6,
+	  .map = { SNDRV_CHMAP_SL, SNDRV_CHMAP_SR,
+		   SNDRV_CHMAP_FL, SNDRV_CHMAP_FR,
+		   SNDRV_CHMAP_RL, SNDRV_CHMAP_RR } },
+	{ }
+};
+
+static const struct of_device_id macaudio_snd_device_id[]  = {
+	{ .compatible = "apple,j274-macaudio", .data = macaudio_j274_chmaps },
+	{ .compatible = "apple,j293-macaudio", .data = macaudio_j293_chmaps },
+	{ .compatible = "apple,j314-macaudio", .data = macaudio_j314_chmaps },
+	{ .compatible = "apple,macaudio", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, macaudio_snd_device_id);
+
+static int macaudio_snd_platform_probe(struct platform_device *pdev)
+{
+	struct snd_soc_card *card;
+	struct macaudio_snd_data *data;
+	struct device *dev = &pdev->dev;
+	struct snd_soc_dai_link *link;
+	const struct of_device_id *of_id;
+	int ret;
+	int i;
+
+	of_id = of_match_device(macaudio_snd_device_id, dev);
+	if (!of_id)
+		return -EINVAL;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->speaker_chmap = of_id->data;
+	card = &data->card;
+	snd_soc_card_set_drvdata(card, data);
+
+	card->owner = THIS_MODULE;
+	card->driver_name = DRIVER_NAME;
+	card->dev = dev;
+	card->dapm_widgets = macaudio_snd_widgets;
+	card->num_dapm_widgets = ARRAY_SIZE(macaudio_snd_widgets);
+	card->probe = macaudio_probe;
+	card->late_probe = macaudio_late_probe;
+	card->remove = macaudio_remove;
+	card->filter_controls = macaudio_filter_controls;
+	card->remove = macaudio_remove;
+
+	ret = macaudio_parse_of(data, card);
+	if (ret)
+		return ret;
+
+	for_each_card_prelinks(card, i, link) {
+		link->ops = &macaudio_ops;
+		link->init = macaudio_init;
+		link->exit = macaudio_exit;
+	}
+
+	return devm_snd_soc_register_card(dev, card);
+}
+
+static struct platform_driver macaudio_snd_driver = {
+	.probe = macaudio_snd_platform_probe,
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = macaudio_snd_device_id,
+		.pm = &snd_soc_pm_ops,
+	},
+};
+module_platform_driver(macaudio_snd_driver);
+
+MODULE_AUTHOR("Martin Povišer <povik+lin@cutebit.org>");
+MODULE_DESCRIPTION("Apple Silicon Macs machine-level sound driver");
+MODULE_LICENSE("GPL");
-- 
2.33.0

Re: [RFC PATCH 5/5] ASoC: Add macaudio machine driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2338 bytes --]

On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote:

> --- /dev/null
> +++ b/sound/soc/apple/macaudio.c
> @@ -0,0 +1,597 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ASoC machine driver for Apple Silicon Macs
> + *

Please make the entire comment a C++ one so things look more
intentional.

> +		/* CPU side is bit and frame clock master, I2S with both clocks inverted */

Please refer to clock providers here.

> +		ret = of_property_read_string(np, "link-name", &link->name);
> +		if (ret) {
> +			dev_err(card->dev, "Missing link name\n");
> +			goto err_put_np;
> +		}

This doesn't look like it's mandatory in the binding.

> +static int macaudio_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_card *card = rtd->card;
> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_component *component;
> +	int ret, i;
> +
> +	if (rtd->num_codecs > 1) {
> +		ret = macaudio_assign_tdm(rtd);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	for_each_rtd_components(rtd, i, component)
> +		snd_soc_component_set_jack(component, &ma->jack, NULL);

What is the jack configuration this is attempting to describe?  It looks
like you have some dedicated speaker driver devices which are going to
get attached to jacks here for example.

> +} macaudio_kctlfixes[] = {
> +	{"* ASI1 Sel", "Left"},
> +	{"* ISENSE Switch", "Off"},
> +	{"* VSENSE Switch", "Off"},
> +	{ }
> +};
> +
> +static bool macaudio_kctlfix_matches(const char *pattern, const char *name)
> +{
> +	if (pattern[0] == '*') {
> +		int namelen, patternlen;
> +
> +		pattern++;
> +		if (pattern[0] == ' ')
> +			pattern++;
> +
> +		namelen = strlen(name);
> +		patternlen = strlen(pattern);
> +
> +		if (namelen > patternlen)
> +			name += (namelen - patternlen);
> +	}
> +
> +	return !strcmp(name, pattern);
> +}

This looks worryingly like use case configuration.

> +/*
> + * Maybe this could be a general ASoC function?
> + */
> +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card,
> +				struct snd_kcontrol *kcontrol, const char *strvalue)

No, we should not be setting user visible control values from the
kernel.  This shouldn't be a machine driver function either.  What are
you trying to accomplish here?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 5/5] ASoC: Add macaudio machine driver

[Martin Povišer]

> On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote:
> 
>> --- /dev/null
>> +++ b/sound/soc/apple/macaudio.c
>> @@ -0,0 +1,597 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * ASoC machine driver for Apple Silicon Macs
>> + *
> 
> Please make the entire comment a C++ one so things look more
> intentional.
> 
>> +		/* CPU side is bit and frame clock master, I2S with both clocks inverted */
> 
> Please refer to clock providers here.
> 
>> +		ret = of_property_read_string(np, "link-name", &link->name);
>> +		if (ret) {
>> +			dev_err(card->dev, "Missing link name\n");
>> +			goto err_put_np;
>> +		}
> 
> This doesn't look like it's mandatory in the binding.

Good catch!

>> +static int macaudio_init(struct snd_soc_pcm_runtime *rtd)
>> +{
>> +	struct snd_soc_card *card = rtd->card;
>> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
>> +	struct snd_soc_component *component;
>> +	int ret, i;
>> +
>> +	if (rtd->num_codecs > 1) {
>> +		ret = macaudio_assign_tdm(rtd);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	for_each_rtd_components(rtd, i, component)
>> +		snd_soc_component_set_jack(component, &ma->jack, NULL);
> 
> What is the jack configuration this is attempting to describe?  It looks
> like you have some dedicated speaker driver devices which are going to
> get attached to jacks here for example.

We know the speakers will ignore the set_jack call. There’s one jack and
this way we know the jack codec will attach to it, for speakers it’s a no-op.
(If you prefer I will special-case it to the jack codec.)

>> +} macaudio_kctlfixes[] = {
>> +	{"* ASI1 Sel", "Left"},
>> +	{"* ISENSE Switch", "Off"},
>> +	{"* VSENSE Switch", "Off"},
>> +	{ }
>> +};
>> +
>> +static bool macaudio_kctlfix_matches(const char *pattern, const char *name)
>> +{
>> +	if (pattern[0] == '*') {
>> +		int namelen, patternlen;
>> +
>> +		pattern++;
>> +		if (pattern[0] == ' ')
>> +			pattern++;
>> +
>> +		namelen = strlen(name);
>> +		patternlen = strlen(pattern);
>> +
>> +		if (namelen > patternlen)
>> +			name += (namelen - patternlen);
>> +	}
>> +
>> +	return !strcmp(name, pattern);
>> +}
> 
> This looks worryingly like use case configuration.

I go over this in the cover letter! This is fixing the TDM slot selection
and disabling voltage/current sensing on the speaker amp codecs, which have
no business being exposed to userspace as options. This is not use case,
this not letting people blow their speakers from userspace.

> 
>> +/*
>> + * Maybe this could be a general ASoC function?
>> + */
>> +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card,
>> +				struct snd_kcontrol *kcontrol, const char *strvalue)
> 
> No, we should not be setting user visible control values from the
> kernel.  This shouldn't be a machine driver function either.  What are
> you trying to accomplish here?

See above.

Martin

Re: [RFC PATCH 5/5] ASoC: Add macaudio machine driver

[Martin Povišer]

> On 31. 3. 2022, at 14:08, Martin Povišer <povik@cutebit.org> wrote:
> 
>> 
>> On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote:
>> 
>> On Thu, Mar 31, 2022 at 02:04:49AM +0200, Martin Povišer wrote:
>> 
>>> --- /dev/null
>>> +++ b/sound/soc/apple/macaudio.c
>>> @@ -0,0 +1,597 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * ASoC machine driver for Apple Silicon Macs
>>> + *

(snip)

>>> +/*
>>> + * Maybe this could be a general ASoC function?
>>> + */
>>> +static void snd_soc_kcontrol_set_strval(struct snd_soc_card *card,
>>> +				struct snd_kcontrol *kcontrol, const char *strvalue)
>> 
>> No, we should not be setting user visible control values from the
>> kernel.  This shouldn't be a machine driver function either.  What are
>> you trying to accomplish here?
> 
> See above.
> 
> Martin

One thing I didn’t point out. The controls we are setting here are not
visible from userspace. That’s the point of the ‘filter’ card method
I am trying to establish in the other commit. With it, the card decides
which controls are okay to be exported and which should be hidden.

Here we are only setting hidden controls.

Martin

Re: [RFC PATCH 5/5] ASoC: Add macaudio machine driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

On Thu, Mar 31, 2022 at 02:08:51PM +0200, Martin Povišer wrote:
> > On 31. 3. 2022, at 13:59, Mark Brown <broonie@kernel.org> wrote:

> >> +	for_each_rtd_components(rtd, i, component)
> >> +		snd_soc_component_set_jack(component, &ma->jack, NULL);

> > What is the jack configuration this is attempting to describe?  It looks
> > like you have some dedicated speaker driver devices which are going to
> > get attached to jacks here for example.

> We know the speakers will ignore the set_jack call. There’s one jack and
> this way we know the jack codec will attach to it, for speakers it’s a no-op.
> (If you prefer I will special-case it to the jack codec.)

It would be better to special case, this looks obviously wrong and will
break if someone adds error handling.

> >> +	return !strcmp(name, pattern);
> >> +}

> > This looks worryingly like use case configuration.

> I go over this in the cover letter! This is fixing the TDM slot selection
> and disabling voltage/current sensing on the speaker amp codecs, which have
> no business being exposed to userspace as options. This is not use case,
> this not letting people blow their speakers from userspace.

Your comments in the cover letter are all pretty vague too, that just
says that these controls are "ridiculous" which isn't terribly specific
about what the actual goal is.  If it's just "I can't see why anyone
would want to configure this" then that's a decision you're taking about
what people might want to do which is broadly a use case configuration
and the control should be left there in case someone comes up with an
idea.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1706 bytes --]

On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote:

> I put together a machine-level ASoC driver for recent Apple Macs (the
> ones with ARM64 SoCs) and want to gauge opinions.

This would be a bit easier to review with a description of the hardware.

> Commit 2 adds a new ASoC card method (filter_controls) to let the card
> prevent some codec kcontrols from being visible to userspace. For example
> the TAS2770 speaker amp driver would be happy to expose TDM slot selection
> and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to
> make the patch acceptable to upstream.

The broad issue here is that what you consider ridiculous someone else
might have some bright ideas for configuring dynamically - if things are
being exposed for dynamic configuration it's probably because someone
wanted them, if the control is genuinely useless then it should just be
removed.  Rather than getting in the way of people's policy arguments
about how to set things we expose them to userspace and let userspace
worry about it, usually with the help of UCM files.  The general
userspace model is that people interact with their sound server more
than the hardware card.  This is also helpful for people developing use
cases, it means they're not having to get the kernel rebuilt to tune
things.

The TDM swap thing you're mentioning looks like it's a left/right
selection which people do use sometimes as a way of doing mono mixes and
reorientation.  The ISENSE/VSENSE is less obvious, though it's possible
there's issues with not having enough slots on a heavily used TDM bus or
sometimes disabling the speaker protection processing for whatever
reason.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 31. 3. 2022, at 14:34, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote:
> 
>> I put together a machine-level ASoC driver for recent Apple Macs (the
>> ones with ARM64 SoCs) and want to gauge opinions.
> 
> This would be a bit easier to review with a description of the hardware.

The typical hardware configuration the driver is supposed to be used with
is this:

 * SoC with couple of I2S ports.

 * Array of speakers with individual speaker amp chips, hooked to a single
   I2S bus or split between two of the SoC’s I2S ports. Speakers can be
   one, two, four or six in total. The speaker amp chips resemble either
   TAS2770 or TAS2764. 

 * Jack codec hooked to a separate I2S port, operating independently.
   (Codec driver supports set_jack.)

The example in the binding patch describes an actual arrangement on one
piece of hardware.

>> Commit 2 adds a new ASoC card method (filter_controls) to let the card
>> prevent some codec kcontrols from being visible to userspace. For example
>> the TAS2770 speaker amp driver would be happy to expose TDM slot selection
>> and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to
>> make the patch acceptable to upstream.
> 
> The broad issue here is that what you consider ridiculous someone else
> might have some bright ideas for configuring dynamically - if things are
> being exposed for dynamic configuration it's probably because someone
> wanted them, if the control is genuinely useless then it should just be
> removed.  Rather than getting in the way of people's policy arguments
> about how to set things we expose them to userspace and let userspace
> worry about it, usually with the help of UCM files.  The general
> userspace model is that people interact with their sound server more
> than the hardware card.  This is also helpful for people developing use
> cases, it means they're not having to get the kernel rebuilt to tune
> things.

Well but these are codec drivers reused on different systems, it can both
be 'not genuinely useless’ on some system and ridiculous to leave open on
the systems I am trying to write drivers for.

> The TDM swap thing you're mentioning looks like it's a left/right
> selection which people do use sometimes as a way of doing mono mixes and
> reorientation.  The ISENSE/VSENSE is less obvious, though it's possible
> there's issues with not having enough slots on a heavily used TDM bus or
> sometimes disabling the speaker protection processing for whatever
> reason.

Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’
meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell
me it’s okay to change that behaviour and it won’t be considered backwards
compatibility breaking, that would be part of the solution I am seeking
here.

But even then, what for example if the system has a single speaker (as it
does on the Mac mini to be covered by this driver) and the I2S bus is left
undriven for the duration of unused TDM slots? That may genuinely pose
a risk of people blowing their speakers by switching something in alsamixer.
Now I can actually make sure the I2S data lines are always zeroed out in
the ASoC platform driver, but I would rather not even have to tie these
loose ends for a control there’s no reason to expose in the first place
(again, on this system).

The ISENSE/VSENSE controls are also actually useless on these systems as we
are not doing anything to pick up the measured values (which are sent back
over the I2S lines). I don’t know if there can be driver conflict between
two speaker amps trying to drive the I2S lines at the same time should
the user happen to enable SENSE facilities on more than one of them.
Now I can grudgingly study that and rule it out but I would rather hide
the controls altogether.

That’s the reasoning anyway. To reiterate, seems to me the controls
are useless/confusing at best and dangerous at worst.

Martin

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4446 bytes --]

On Thu, Mar 31, 2022 at 03:28:12PM +0200, Martin Povišer wrote:
> > On 31. 3. 2022, at 14:34, Mark Brown <broonie@kernel.org> wrote:

> > The broad issue here is that what you consider ridiculous someone else
> > might have some bright ideas for configuring dynamically - if things are
> > being exposed for dynamic configuration it's probably because someone
> > wanted them, if the control is genuinely useless then it should just be

> Well but these are codec drivers reused on different systems, it can both
> be 'not genuinely useless’ on some system and ridiculous to leave open on
> the systems I am trying to write drivers for.

It wouldn't be the first time that we've had someone turn up with a new
idea for how to configure an already existing bit of hardware, part of
the reason for this approach is that people do get surprised by user
creativity with their systems.

> > The TDM swap thing you're mentioning looks like it's a left/right
> > selection which people do use sometimes as a way of doing mono mixes and
> > reorientation.  The ISENSE/VSENSE is less obvious, though it's possible
> > there's issues with not having enough slots on a heavily used TDM bus or
> > sometimes disabling the speaker protection processing for whatever
> > reason.

> Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’
> meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell
> me it’s okay to change that behaviour and it won’t be considered backwards
> compatibility breaking, that would be part of the solution I am seeking
> here.

Having the default state be muted or not routed is quite common, UCM
files or equivalent are typically required for embedded style hardware
like this.

> But even then, what for example if the system has a single speaker (as it
> does on the Mac mini to be covered by this driver) and the I2S bus is left
> undriven for the duration of unused TDM slots? That may genuinely pose
> a risk of people blowing their speakers by switching something in alsamixer.

Right, so that's a more sensible and valid use case.  We do have the
platform_max feature available for precisely this reason - that's
probably more appropriate here since if there's a danger of people
blowing their speaker with a floating input they could also blow their
speaker with just a very loud audio signal so limiting the volume people
can set on the speaker driver seems sensible and would also cover them
for misrouting.  Whatever the device might pick up from noise on an
undriven bus could also be played as audio down the bus.  This does
become a little fun with speaker protection as we'd want to raise the
kernel limit so that userspace can dynamically manage the volume to
contorl power (though that might be done with software control), but
it's easy enoguh to raise limits later.

On the other hand it seems like userspace might reasonably choose to do
a mono mix for this output entirely in software, in which case telling
the speaker amp to pick up one channel would make sense, or to just play
out a stereo signal over I2S and have the amplifier do a mono mix and
I'm not seeing why we'd force one or the other in the machine driver.

> The ISENSE/VSENSE controls are also actually useless on these systems as we
> are not doing anything to pick up the measured values (which are sent back
> over the I2S lines). I don’t know if there can be driver conflict between

Presumably someone might want to work on figuring that out though, and
from a hardware safety point of view it would be better if they did.

> two speaker amps trying to drive the I2S lines at the same time should
> the user happen to enable SENSE facilities on more than one of them.
> Now I can grudgingly study that and rule it out but I would rather hide
> the controls altogether.

Yes, having two devices driving the bus at the same time wouldn't be
great.  How is the TDM slot selection for the signals done in the
hardware, I'm not seeing anything immediately obvious in the driver?
I'd have thought that things would be implemented such that you could
implement speaker protection on all speakers simultaneously but perhaps
not.

> That’s the reasoning anyway. To reiterate, seems to me the controls
> are useless/confusing at best and dangerous at worst.

I'm just not seeing an issue for the slot selection.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 31. 3. 2022, at 16:18, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 03:28:12PM +0200, Martin Povišer wrote:
>>> On 31. 3. 2022, at 14:34, Mark Brown <broonie@kernel.org> wrote:
> 
>>> The broad issue here is that what you consider ridiculous someone else
>>> might have some bright ideas for configuring dynamically - if things are
>>> being exposed for dynamic configuration it's probably because someone
>>> wanted them, if the control is genuinely useless then it should just be
> 
>> Well but these are codec drivers reused on different systems, it can both
>> be 'not genuinely useless’ on some system and ridiculous to leave open on
>> the systems I am trying to write drivers for.
> 
> It wouldn't be the first time that we've had someone turn up with a new
> idea for how to configure an already existing bit of hardware, part of
> the reason for this approach is that people do get surprised by user
> creativity with their systems.
> 
>>> The TDM swap thing you're mentioning looks like it's a left/right
>>> selection which people do use sometimes as a way of doing mono mixes and
>>> reorientation.  The ISENSE/VSENSE is less obvious, though it's possible
>>> there's issues with not having enough slots on a heavily used TDM bus or
>>> sometimes disabling the speaker protection processing for whatever
>>> reason.
> 
>> Not only that. On TAS2770 the default value for ‘ASI1 Sel’ is ‘I2C offset’
>> meaning the speaker amp driver ignores my set_tdm_slot calls. If you tell
>> me it’s okay to change that behaviour and it won’t be considered backwards
>> compatibility breaking, that would be part of the solution I am seeking
>> here.
> 
> Having the default state be muted or not routed is quite common, UCM
> files or equivalent are typically required for embedded style hardware
> like this.
> 
>> But even then, what for example if the system has a single speaker (as it
>> does on the Mac mini to be covered by this driver) and the I2S bus is left
>> undriven for the duration of unused TDM slots? That may genuinely pose
>> a risk of people blowing their speakers by switching something in alsamixer.
> 
> Right, so that's a more sensible and valid use case.  We do have the
> platform_max feature available for precisely this reason - that's
> probably more appropriate here since if there's a danger of people
> blowing their speaker with a floating input they could also blow their
> speaker with just a very loud audio signal so limiting the volume people
> can set on the speaker driver seems sensible and would also cover them
> for misrouting.  Whatever the device might pick up from noise on an
> undriven bus could also be played as audio down the bus.  This does
> become a little fun with speaker protection as we'd want to raise the
> kernel limit so that userspace can dynamically manage the volume to
> contorl power (though that might be done with software control), but
> it's easy enoguh to raise limits later.
> 
> On the other hand it seems like userspace might reasonably choose to do
> a mono mix for this output entirely in software, in which case telling
> the speaker amp to pick up one channel would make sense, or to just play
> out a stereo signal over I2S and have the amplifier do a mono mix and
> I'm not seeing why we'd force one or the other in the machine driver.

Granted. If we make sure the volume caps are there to prevent damage
under arbitrary input (which we should anyway) that covers slot
misconfiguration too.

>> The ISENSE/VSENSE controls are also actually useless on these systems as we
>> are not doing anything to pick up the measured values (which are sent back
>> over the I2S lines). I don’t know if there can be driver conflict between
> 
> Presumably someone might want to work on figuring that out though, and
> from a hardware safety point of view it would be better if they did.
> 
>> two speaker amps trying to drive the I2S lines at the same time should
>> the user happen to enable SENSE facilities on more than one of them.
>> Now I can grudgingly study that and rule it out but I would rather hide
>> the controls altogether.
> 
> Yes, having two devices driving the bus at the same time wouldn't be
> great.  How is the TDM slot selection for the signals done in the
> hardware, I'm not seeing anything immediately obvious in the driver?
> I'd have thought that things would be implemented such that you could
> implement speaker protection on all speakers simultaneously but perhaps
> not.

I don’t know. I would have to go study the details of this. Should I see
if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings
that would lead to driver conflict on one of the models, or is there
a chance we could hide those controls just on the basis of ‘it doesn’t
do anything usable and is possibly dangerous’?

>> That’s the reasoning anyway. To reiterate, seems to me the controls
>> are useless/confusing at best and dangerous at worst.
> 
> I'm just not seeing an issue for the slot selection.

Yeah, agreed there’s no (damage) issue as we should to proper volume
caps anyway.

Martin

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1907 bytes --]

On Thu, Mar 31, 2022 at 05:04:32PM +0200, Martin Povišer wrote:
> > On 31. 3. 2022, at 16:18, Mark Brown <broonie@kernel.org> wrote:

> > Yes, having two devices driving the bus at the same time wouldn't be
> > great.  How is the TDM slot selection for the signals done in the
> > hardware, I'm not seeing anything immediately obvious in the driver?
> > I'd have thought that things would be implemented such that you could
> > implement speaker protection on all speakers simultaneously but perhaps
> > not.

> I don’t know. I would have to go study the details of this. Should I see
> if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings
> that would lead to driver conflict on one of the models, or is there
> a chance we could hide those controls just on the basis of ‘it doesn’t
> do anything usable and is possibly dangerous’?

If ISENSE and VSENSE output are controlled by the same mux as routing
then we should lock one of the controls out for at least stereo devices
(it might be a good idea to check if the output is actually high Z when
ISENSE and VSENSE are off rather than just driving zeros, if not it
definitely has to be the routing control).  My instinct is that it's
better to preserve the ability to implement speaker protection in future
since that is something that'd be broadly useful, especially if someone
comes up with a generic speaker protection implementation in which case
there should be an awful lot of systems out there which could benefit. 

> >> That’s the reasoning anyway. To reiterate, seems to me the controls
> >> are useless/confusing at best and dangerous at worst.

> > I'm just not seeing an issue for the slot selection.

> Yeah, agreed there’s no (damage) issue as we should to proper volume
> caps anyway.

Though see above about how ISENSE/VSENSE output slot is controlled I guess :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 31. 3. 2022, at 17:36, Mark Brown <broonie@kernel.org> wrote:
> 
> On Thu, Mar 31, 2022 at 05:04:32PM +0200, Martin Povišer wrote:
>>> On 31. 3. 2022, at 16:18, Mark Brown <broonie@kernel.org> wrote:
> 
>>> Yes, having two devices driving the bus at the same time wouldn't be
>>> great.  How is the TDM slot selection for the signals done in the
>>> hardware, I'm not seeing anything immediately obvious in the driver?
>>> I'd have thought that things would be implemented such that you could
>>> implement speaker protection on all speakers simultaneously but perhaps
>>> not.
> 
>> I don’t know. I would have to go study the details of this. Should I see
>> if I can find a combination of ‘ASI1 Sel’ ‘VSENSE’ ‘ISENSE’ settings
>> that would lead to driver conflict on one of the models, or is there
>> a chance we could hide those controls just on the basis of ‘it doesn’t
>> do anything usable and is possibly dangerous’?
> 
> If ISENSE and VSENSE output are controlled by the same mux as routing
> then we should lock one of the controls out for at least stereo devices
> (it might be a good idea to check if the output is actually high Z when
> ISENSE and VSENSE are off rather than just driving zeros, if not it
> definitely has to be the routing control).  My instinct is that it's
> better to preserve the ability to implement speaker protection in future
> since that is something that'd be broadly useful, especially if someone
> comes up with a generic speaker protection implementation in which case
> there should be an awful lot of systems out there which could benefit. 

Sorry for having put this on hold for a while.

I looked in the TAS2770 and TAS2764 drivers/datasheets, and to answer
the questions we had:

 * VSENSE/ISENSE output slots are configured independently of audio samples
   routing. Kernel drivers configure the slots based on the 'ti,imon-slot-no'
   and 'ti,vmon-slot-no' properties of devicetree.

 * By default codecs transmit Hi-Z for duration of unused slots.

So once we supply the devicetree props it should be electrically sound
under any configuration of userspace knobs.

One final thought on the playback routing controls: On systems with >2
speakers, the codecs need to be assigned slots through set_tdm_slot.
The macaudio driver RFCed here assigns a single slot to each speaker,
making the effect of each speaker's routing control this:

  'I2C offset' -- uses a random slot

  'Left' 'Right' 'LeftRight' -- uses the single slot we configured

I suppose I better assign two slots to speakers in each left-right pair
of the same kind (e.g. woofer 1, woofer 2, tweeter). This way the
routing control will mimic its behavior from simple stereo systems but
replicated within each left-right pair.  (I would prefer to hide the
controls altogether, but as I learned that hiding things unless proven
dangerous is an ASoC non-goal, this way I can make the controls do
something interesting.)

Martin

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote:

> I looked in the TAS2770 and TAS2764 drivers/datasheets, and to answer
> the questions we had:

>  * VSENSE/ISENSE output slots are configured independently of audio samples
>    routing. Kernel drivers configure the slots based on the 'ti,imon-slot-no'
>    and 'ti,vmon-slot-no' properties of devicetree.

>  * By default codecs transmit Hi-Z for duration of unused slots.

> So once we supply the devicetree props it should be electrically sound
> under any configuration of userspace knobs.

Great, that's a relief.

> One final thought on the playback routing controls: On systems with >2
> speakers, the codecs need to be assigned slots through set_tdm_slot.
> The macaudio driver RFCed here assigns a single slot to each speaker,
> making the effect of each speaker's routing control this:

>   'I2C offset' -- uses a random slot

>   'Left' 'Right' 'LeftRight' -- uses the single slot we configured

> I suppose I better assign two slots to speakers in each left-right pair
> of the same kind (e.g. woofer 1, woofer 2, tweeter). This way the
> routing control will mimic its behavior from simple stereo systems but
> replicated within each left-right pair.  (I would prefer to hide the
> controls altogether, but as I learned that hiding things unless proven
> dangerous is an ASoC non-goal, this way I can make the controls do
> something interesting.)

I don't quite grasp the difference between the arrangement you're
proposing and assigning a single slot to each speaker?  Possibly it's
just a reordering of the slots?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 22. 4. 2022, at 13:19, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote:
> 
>> I looked in the TAS2770 and TAS2764 drivers/datasheets, and to answer
>> the questions we had:
> 
>> * VSENSE/ISENSE output slots are configured independently of audio samples
>>   routing. Kernel drivers configure the slots based on the 'ti,imon-slot-no'
>>   and 'ti,vmon-slot-no' properties of devicetree.
> 
>> * By default codecs transmit Hi-Z for duration of unused slots.
> 
>> So once we supply the devicetree props it should be electrically sound
>> under any configuration of userspace knobs.
> 
> Great, that's a relief.
> 
>> One final thought on the playback routing controls: On systems with >2
>> speakers, the codecs need to be assigned slots through set_tdm_slot.
>> The macaudio driver RFCed here assigns a single slot to each speaker,
>> making the effect of each speaker's routing control this:
> 
>>  'I2C offset' -- uses a random slot
> 
>>  'Left' 'Right' 'LeftRight' -- uses the single slot we configured
> 
>> I suppose I better assign two slots to speakers in each left-right pair
>> of the same kind (e.g. woofer 1, woofer 2, tweeter). This way the
>> routing control will mimic its behavior from simple stereo systems but
>> replicated within each left-right pair.  (I would prefer to hide the
>> controls altogether, but as I learned that hiding things unless proven
>> dangerous is an ASoC non-goal, this way I can make the controls do
>> something interesting.)
> 
> I don't quite grasp the difference between the arrangement you're
> proposing and assigning a single slot to each speaker?  Possibly it's
> just a reordering of the slots?

Ah, maybe what’s missing is the fact that the way the speaker amp drivers
are written, if they are assigned two slots with a call to set_tdm_slot,
the first slot is considered 'left' and the second is 'right'.

So in the arrangement I am proposing the 'Left', 'Right' and 'LeftRight'
values of the routing control have the nominal effect (within the left-right
speaker pair), while in the other arrangement it is as I described above.

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1226 bytes --]

On Fri, Apr 22, 2022 at 01:28:20PM +0200, Martin Povišer wrote:
> > On 22. 4. 2022, at 13:19, Mark Brown <broonie@kernel.org> wrote:
> > On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote:

> >> One final thought on the playback routing controls: On systems with >2
> >> speakers, the codecs need to be assigned slots through set_tdm_slot.
> >> The macaudio driver RFCed here assigns a single slot to each speaker,
> >> making the effect of each speaker's routing control this:

...

> > I don't quite grasp the difference between the arrangement you're
> > proposing and assigning a single slot to each speaker?  Possibly it's
> > just a reordering of the slots?

> Ah, maybe what’s missing is the fact that the way the speaker amp drivers
> are written, if they are assigned two slots with a call to set_tdm_slot,
> the first slot is considered 'left' and the second is 'right'.

> So in the arrangement I am proposing the 'Left', 'Right' and 'LeftRight'
> values of the routing control have the nominal effect (within the left-right
> speaker pair), while in the other arrangement it is as I described above.

So previously each speaker would get two slots but now it just gets one?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 22. 4. 2022, at 13:33, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 01:28:20PM +0200, Martin Povišer wrote:
>>> On 22. 4. 2022, at 13:19, Mark Brown <broonie@kernel.org> wrote:
>>> On Fri, Apr 22, 2022 at 12:43:30PM +0200, Martin Povišer wrote:
> 
>>>> One final thought on the playback routing controls: On systems with >2
>>>> speakers, the codecs need to be assigned slots through set_tdm_slot.
>>>> The macaudio driver RFCed here assigns a single slot to each speaker,
>>>> making the effect of each speaker's routing control this:
> 
> ...
> 
>>> I don't quite grasp the difference between the arrangement you're
>>> proposing and assigning a single slot to each speaker?  Possibly it's
>>> just a reordering of the slots?
> 
>> Ah, maybe what’s missing is the fact that the way the speaker amp drivers
>> are written, if they are assigned two slots with a call to set_tdm_slot,
>> the first slot is considered 'left' and the second is 'right'.
> 
>> So in the arrangement I am proposing the 'Left', 'Right' and 'LeftRight'
>> values of the routing control have the nominal effect (within the left-right
>> speaker pair), while in the other arrangement it is as I described above.
> 
> So previously each speaker would get two slots but now it just gets one?

No the other way around. Previously (with the driver as it is RFCed),
each speaker gets a single slot, and 'Left', 'Right' and ‘LeftRight'
values of the routing control don't do anything different from each
other (well except maybe 'LeftRight' lessens the volume due to how
the chip handles the edge case of mixing down two channels from the
same slot).

With the new arrangement I am proposing, the two speakers in a left-right
pair get both the same two slots, meaning they get to choose one of the
two slots based on the 'Left' 'Right' value of their routing control.

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 949 bytes --]

On Fri, Apr 22, 2022 at 01:44:06PM +0200, Martin Povišer wrote:

> > So previously each speaker would get two slots but now it just gets one?

> No the other way around. Previously (with the driver as it is RFCed),
> each speaker gets a single slot, and 'Left', 'Right' and ‘LeftRight'
> values of the routing control don't do anything different from each
> other (well except maybe 'LeftRight' lessens the volume due to how
> the chip handles the edge case of mixing down two channels from the
> same slot).

> With the new arrangement I am proposing, the two speakers in a left-right
> pair get both the same two slots, meaning they get to choose one of the
> two slots based on the 'Left' 'Right' value of their routing control.

Ah, I think the confusion here is that I'm using slot and channel
interchangably whereas you're saying that previously the driver would
allocate two channels to each speaker with duplicate data?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 22. 4. 2022, at 14:22, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 01:44:06PM +0200, Martin Povišer wrote:
> 
>>> So previously each speaker would get two slots but now it just gets one?
> 
>> No the other way around. Previously (with the driver as it is RFCed),
>> each speaker gets a single slot, and 'Left', 'Right' and ‘LeftRight'
>> values of the routing control don't do anything different from each
>> other (well except maybe 'LeftRight' lessens the volume due to how
>> the chip handles the edge case of mixing down two channels from the
>> same slot).
> 
>> With the new arrangement I am proposing, the two speakers in a left-right
>> pair get both the same two slots, meaning they get to choose one of the
>> two slots based on the 'Left' 'Right' value of their routing control.
> 
> Ah, I think the confusion here is that I'm using slot and channel
> interchangably whereas you're saying that previously the driver would
> allocate two channels to each speaker with duplicate data?

I guess you could say that. Not that there’s duplicate data on the I2S
bus, but the speaker amp would previously be configured to look for the
left and right channel in the same TDM slot (see e.g. set_tdm_slot of
tas2770 [0]).  (Each speaker amp drives a single speaker, but it still
has a notion of left and right channel.)

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/codecs/tas2770.c#n416

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On Fri, Apr 22, 2022 at 02:36:03PM +0200, Martin Povišer wrote:

> > Ah, I think the confusion here is that I'm using slot and channel
> > interchangably whereas you're saying that previously the driver would
> > allocate two channels to each speaker with duplicate data?

> I guess you could say that. Not that there’s duplicate data on the I2S
> bus, but the speaker amp would previously be configured to look for the
> left and right channel in the same TDM slot (see e.g. set_tdm_slot of
> tas2770 [0]).  (Each speaker amp drives a single speaker, but it still
> has a notion of left and right channel.)

Oh, I see - the speaker actually allows configuration of the slots
independently.  Usually the left/right thing on mono devices only does
something for I2S where the bus clocking enforces that there be both
left and right channels.  Either configuration is fine by me TBH, if you
can do that then you could just keep them mapped to the same channel
then mark the control as disabled since it should have no effect.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 22. 4. 2022, at 14:44, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 02:36:03PM +0200, Martin Povišer wrote:
> 
>>> Ah, I think the confusion here is that I'm using slot and channel
>>> interchangably whereas you're saying that previously the driver would
>>> allocate two channels to each speaker with duplicate data?
> 
>> I guess you could say that. Not that there’s duplicate data on the I2S
>> bus, but the speaker amp would previously be configured to look for the
>> left and right channel in the same TDM slot (see e.g. set_tdm_slot of
>> tas2770 [0]).  (Each speaker amp drives a single speaker, but it still
>> has a notion of left and right channel.)
> 
> Oh, I see - the speaker actually allows configuration of the slots
> independently.  Usually the left/right thing on mono devices only does
> something for I2S where the bus clocking enforces that there be both
> left and right channels.  Either configuration is fine by me TBH, if you
> can do that then you could just keep them mapped to the same channel
> then mark the control as disabled since it should have no effect.

Well but is there some established way to mark a control as disabled?

Another issue here is that if I disable it I can’t leave the routing
control in it’s default value, which is ‘I2C Offset’ and makes the speaker
amp ignore the slot mapping.

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1022 bytes --]

On Fri, Apr 22, 2022 at 02:53:54PM +0200, Martin Povišer wrote:

> > Oh, I see - the speaker actually allows configuration of the slots
> > independently.  Usually the left/right thing on mono devices only does
> > something for I2S where the bus clocking enforces that there be both
> > left and right channels.  Either configuration is fine by me TBH, if you
> > can do that then you could just keep them mapped to the same channel
> > then mark the control as disabled since it should have no effect.

> Well but is there some established way to mark a control as disabled?

snd_ctl_activate_id().

> Another issue here is that if I disable it I can’t leave the routing
> control in it’s default value, which is ‘I2C Offset’ and makes the speaker
> amp ignore the slot mapping.

Sure, that's fine - if a control genuinely has no effect it's fine to
hide it from userspace.  The issue is where it's just that you don't see
the use, if the control demonstrably does nothing then that's fine.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Martin Povišer]

> On 22. 4. 2022, at 15:06, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, Apr 22, 2022 at 02:53:54PM +0200, Martin Povišer wrote:
> 
>>> Oh, I see - the speaker actually allows configuration of the slots
>>> independently.  Usually the left/right thing on mono devices only does
>>> something for I2S where the bus clocking enforces that there be both
>>> left and right channels.  Either configuration is fine by me TBH, if you
>>> can do that then you could just keep them mapped to the same channel
>>> then mark the control as disabled since it should have no effect.
> 
>> Well but is there some established way to mark a control as disabled?
> 
> snd_ctl_activate_id().

Ha! Great.

>> Another issue here is that if I disable it I can’t leave the routing
>> control in it’s default value, which is ‘I2C Offset’ and makes the speaker
>> amp ignore the slot mapping.
> 
> Sure, that's fine - if a control genuinely has no effect it's fine to
> hide it from userspace.  The issue is where it's just that you don't see
> the use, if the control demonstrably does nothing then that's fine.

So I assume I can set the control from the machine driver then disable it.

Anyway, good, this is what I meant earlier when I said the controls I want
to hide are 'useless/confusing at best’. I must walk back that they are
‘dangerous at worst’, but I am glad we can hide them anyway. (Not all of
them of course, ISENSE/VSENSE will not be hidden, neither the routing
control on systems with single mono speaker.)

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Hector Martin]

On 31/03/2022 21.34, Mark Brown wrote:
> On Thu, Mar 31, 2022 at 02:04:44AM +0200, Martin Povišer wrote:
> 
>> I put together a machine-level ASoC driver for recent Apple Macs (the
>> ones with ARM64 SoCs) and want to gauge opinions.
> 
> This would be a bit easier to review with a description of the hardware.
> 
>> Commit 2 adds a new ASoC card method (filter_controls) to let the card
>> prevent some codec kcontrols from being visible to userspace. For example
>> the TAS2770 speaker amp driver would be happy to expose TDM slot selection
>> and ISENSE/VSENSE enables which is ridiculous. I am all ears on how to
>> make the patch acceptable to upstream.
> 
> The broad issue here is that what you consider ridiculous someone else
> might have some bright ideas for configuring dynamically - if things are
> being exposed for dynamic configuration it's probably because someone
> wanted them, if the control is genuinely useless then it should just be
> removed.  Rather than getting in the way of people's policy arguments
> about how to set things we expose them to userspace and let userspace
> worry about it, usually with the help of UCM files.  The general
> userspace model is that people interact with their sound server more
> than the hardware card.  This is also helpful for people developing use
> cases, it means they're not having to get the kernel rebuilt to tune
> things.

The problem with this model is that, in particular in the case of
speaker amps, incorrect settings can cause your speakers to blow up.
This has been a longstanding problem with ASoC platforms (I should know,
I *melted* the speakers in a Chromebook by toggling the wrong alsamixer
control once, it even warped the external case, all without making any
audible noise).

It's the kernel's job to ensure that broadly exposed user controls are
safe and cannot be used to cause hardware damage; if that is possible,
then that's a kernel security vulnerability worthy of a CVE, in my
opinion. I think this idea of exposing what is effectively raw codec
chip registers as ALSA controls that is so popular these days was a
terrible idea from the start, and only makes some sense within the world
of highly integrated vendor-controlled embedded platforms running
kiosk-style software with no user control. It is completely unsuitable
for a desktop Linux system, since it means users *will* destroy their
hardware accidentally. So, some way or another, whatever is exposed has
to be sanitized so that it can't go outside the envelope of what is safe
for the hardware design. That cannot be known at the level of codec
chips and speaker amp chips; it requires platform integration knowledge.

That knowledge is what is (intended to be) encoded in the macaudio
driver. It's supposed to know how to drive the underlying codec chips
and disable access to things that don't make any sense on the platform,
and expose controls to the user that are reasonable for what a user
would want to do on that specific hardware platform, and no more.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2036 bytes --]

On Thu, Mar 31, 2022 at 10:28:56PM +0900, Hector Martin wrote:

> The problem with this model is that, in particular in the case of
> speaker amps, incorrect settings can cause your speakers to blow up.
> This has been a longstanding problem with ASoC platforms (I should know,
> I *melted* the speakers in a Chromebook by toggling the wrong alsamixer
> control once, it even warped the external case, all without making any
> audible noise).

Yes, that's why we have platform_max - it was added for use with
Chromebooks originally, someone else had the same idea you did.  It's
used less often than I'd like since most embedded systems and even
things like Chromebooks have a software model where the actual sound
card isn't accessible to normal users but that's not the case once you
try to run a general purpose distro on there.

> kiosk-style software with no user control. It is completely unsuitable
> for a desktop Linux system, since it means users *will* destroy their
> hardware accidentally. So, some way or another, whatever is exposed has
> to be sanitized so that it can't go outside the envelope of what is safe
> for the hardware design. That cannot be known at the level of codec
> chips and speaker amp chips; it requires platform integration knowledge.

Yes, we should be trying to exclude configurations that could be
physically destructive but that's not what had been articulated and like
I said in reply to his last mail it's really not clear to me that what's
being proposed would actually accomplish the intended goal.  Targeted
restrictions that protect the system are fine and good, random "why
would anyone want this?" or "this is how you accomplish use case X" ones
are not since we do get users turning up with new ideas.

This is one reason why it's important to articulate what the intended
goal of changes is, what you've written above is perfectly fine and
reasonable but there was nothing about this in the original changelogs,
just statements about how silly it would be to configure these controls.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [RFC PATCH 0/5] Apple Macs machine-level ASoC driver

[Mark Brown]

On Thu, 31 Mar 2022 02:04:44 +0200, Martin Povišer wrote:
> I put together a machine-level ASoC driver for recent Apple Macs (the
> ones with ARM64 SoCs) and want to gauge opinions.
> 
> Commit 1 is the binding. It is some subset of simple-audio-card with
> the extra distinction of allowing multiple CPU/CODEC DAIs per a DAI
> link. I want to draw special attention to the issue of describing
> speaker topologies. The way it now works is that the driver expects
> the speakers to be declared in a fixed order in the sound-dai= list.
> This populates a topology the driver expects on a particular machine
> model. Mark (in CC) has made the suggestion of keeping the topology
> descriptions with the codec nodes themselves in some generic manner,
> akin to how sound-name-prefix= already helps identify codecs to the
> user.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[4/5] ASoC: Introduce snd_soc_of_get_dai_link_cpus
      commit: 900dedd7e47cc3f8d93dfa0ae6ac6cf49eda0c97

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark