[RFC PATCH 0/5] Apple Macs machine-level ASoC driver
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
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>; + }; + }; + };
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,
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.
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) &&
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...
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.
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).
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.
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.
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.
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.
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.
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();
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");
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?
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.
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.
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.
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
participants (4)
-
Hector Martin -
Mark Brown -
Martin Povišer -
Martin Povišer