Re: [PATCH 16/28] ASoC: apple: Add macaudio machine driver

[Mark Brown <broonie@kernel.org>]

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

On Sun, Sep 20, 2026 at 02:53:55PM +1000, James Calligeros wrote:

> Apple Silicon Macs have a complex audio subsystem consisting
> of an I2S peripheral (MCA) and multiple codecs of various
> models and capabilities. Some machines have a basic mono
> speaker with hardware downmix, while others have a very
> intricate stereo system consisting of multiple codecs and
> drivers per L/R channel. Some machines report voice coil
> voltage and current information back to the SoC, and others
> do not. All machines have a headset jack.

> +config SND_SOC_APPLE_MACAUDIO
> +	tristate "Sound support for Apple Silicon Macs"
> +	depends on ARCH_APPLE || COMPILE_TEST
> +	select SND_SOC_APPLE_MCA
> +	select SND_SIMPLE_CARD_UTILS

Do we actually use simple-card?

> --- a/sound/soc/apple/Makefile
> +++ b/sound/soc/apple/Makefile
> @@ -2,3 +2,7 @@
>  snd-soc-apple-mca-y	:= mca.o
>  
>  obj-$(CONFIG_SND_SOC_APPLE_MCA)	+= snd-soc-apple-mca.o
> +
> +snd-soc-macaudio-objs	:= macaudio.o
> +
> +obj-$(CONFIG_SND_SOC_APPLE_MACAUDIO)	+= snd-soc-macaudio.o

Note the -y vs -objs style difference here.

> @@ -0,0 +1,1729 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ASoC machine driver for Apple Silicon Macs
> + *

Please kep the entire comment block C++ so things look more consistent.

> +static void macaudio_vlimit_unlock(struct macaudio_snd_data *ma, bool unlock)
> +{

> +	case AMP_TAS5770:
> +		if (unlock)
> +			max = TAS5770_0DB;
> +		else
> +			max = 1; //TAS5770_DB(ma->cfg->safe_vol);
> +		break;
> +	case AMP_SN012776:
> +		if (unlock)
> +			max = SN012776_0DB;
> +		else
> +			max = 1; //SN012776_DB(ma->cfg->safe_vol);
> +		break;

What's going on with those comments?

> +static void macaudio_vlimit_update(struct macaudio_snd_data *ma)
> +{

> +
> +	/* Check that *every* limited control is locked by the same owner */
> +	list_for_each_entry(kctl, &ma->card.snd_card->controls, list) {
> +		if (!snd_soc_control_matches(kctl, volume_control_names[ma->cfg->amp]))
> +			continue;

This is used from the volume limit timeout work which doesn't hold the
controls_rwsem, userspace can add or remove user controls which would
change the list so the work needs to lock the controls list.

> +/*
> + * Parse one DPCM backend from the devicetree. This means taking one
> + * of the CPU DAIs and combining it with one or more CODEC DAIs.
> + */
> +static int macaudio_parse_of_be_dai_link(struct macaudio_snd_data *ma,
> +				struct snd_soc_dai_link *link,
> +				int be_index, int ncodecs_per_be,
> +				struct device_node *cpu,
> +				struct device_node *codec)

> +	ret = macaudio_parse_of_component(cpu, be_index, link->cpus);
> +	if (ret)
> +		return dev_err_probe(ma->card.dev, ret, "parsing CPU DAI of link '%s' at %pOF\n",
> +				     link->name, codec);

Cut'n'paste with logging the codec node.

> +static int macaudio_get_codec_idle_props(struct device_node *np,
> +					 struct ma_codec_idle *idle, int i,
> +					 int be_index, int ncodecs_per_cpu)
> +{
> +	char propname[32];
> +	int ret, sys_codec;
> +
> +	/*
> +	 * Depending on the BE index on the DAI, we need to offset our i
> +	 */
> +	sys_codec = i + (be_index * ncodecs_per_cpu);
> +
> +	snprintf(propname, 32, "dai-tdm-idle-mode-%d", sys_codec);
> +	ret = of_property_match_string(np, propname, "zero");
> +	if (!ret) {
> +		idle->idle_mode = SND_SOC_DAI_TDM_IDLE_ZERO;
> +	} else {
> +		ret = of_property_match_string(np, propname, "pulldown");
> +		if (!ret)
> +			idle->idle_mode = SND_SOC_DAI_TDM_IDLE_PULLDOWN;
> +		else
> +			return ret;
> +	}

Should we warn on unknown values?

> +static int macaudio_dpcm_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_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->dai_link->id];
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> +	struct snd_interval *rate = hw_param_interval(params,
> +						      SNDRV_PCM_HW_PARAM_RATE);
> +	int bclk_ratio = macaudio_get_runtime_bclk_ratio(substream);
> +	int i;
> +
> +	if (props->is_sense) {
> +		rate->min = rate->max = cpu_dai->symmetric_rate;
> +		return 0;
> +	}

It feels like this DAI ought to have separate ops...  Also, for the
sense link will we definitely already have a rate set up?

> +static int macaudio_be_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_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->dai_link->id];
> +
> +	if (props->is_speakers && substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		switch (cmd) {
> +		case SNDRV_PCM_TRIGGER_START:
> +		case SNDRV_PCM_TRIGGER_RESUME:
> +		case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +			ma->bes_active |= BIT(rtd->dai_link->id);
> +			break;
> +		case SNDRV_PCM_TRIGGER_SUSPEND:
> +		case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		case SNDRV_PCM_TRIGGER_STOP:
> +			ma->bes_active &= ~BIT(rtd->dai_link->id);
> +			break;

Do we have adequate locking here, this is shared between BEs but the BEs
have their own locks?

> +static int macaudio_be_assign_tdm(struct snd_soc_pcm_runtime *rtd)
> +{

> +	for_each_rtd_codec_dais(rtd, i, dai) {

> +		if (props->codecs[i].idle_mode) {
> +			ret = snd_soc_dai_set_tdm_idle(dai, props->codecs[i].idle_mask,
> +						       0, props->codecs[i].idle_mode, 0);
> +		}
> +	}
> +
> +	return 0;

We store ret but don't pay attention to the result.

> +#define CHECK(call, pattern, value, min)                                       \
> +	{                                                                      \
> +		int ret = call(card, pattern, value);                          \
> +		int err = (ret >= 0 && ret < min) ? -ERANGE : ret;             \
> +		if (err < 0) {                                                 \
> +			dev_err(card->dev, "%s on '%s': %d\n", #call, pattern, \
> +				ret);                                          \
> +			if (please_blow_up_my_speakers < 2)                    \
> +				return err;                                    \
> +		} else {                                                       \
> +			dev_dbg(card->dev, "%s on '%s': %d hits\n", #call,     \
> +				pattern, ret);                                 \
> +		}                                                              \
> +	}
> +
> +#define CHECK_CONCAT(call, suffix, value) \
> +	{ \
> +		snprintf(buf, sizeof(buf), "%s%s", prefix, suffix); \
> +		CHECK(call, buf, value, 1); \
> +	}

This only requires one match, things might go wrong for a stereo setup.

> +static const struct snd_kcontrol_new macaudio_hp_mux =
> +	SOC_DAPM_ENUM("Headphones Playback Mux", macaudio_hp_mux_enum);

> +	SND_SOC_DAPM_SPK("Speaker (Static)", NULL),

Is this used?

> +	SND_SOC_DAPM_MUX("Headphone Playback Mux", SND_SOC_NOPM, 0, 0, &macaudio_hp_mux),

Headphone or Headphones?

> +static int macaudio_slk_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *uvalue)
> +{
> +	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
> +
> +	if (!ma->speaker_lock_owner)
> +		return -EPERM;
> +
> +	if (uvalue->value.integer.value[0] != SPEAKER_MAGIC_VALUE)
> +		return -EINVAL;
> +
> +	/* Serves as a notification that the lock was lost at some point */
> +	if (ma->speaker_volume_was_locked) {
> +		ma->speaker_volume_was_locked = false;
> +		return -ETIMEDOUT;
> +	}
> +
> +	mutex_lock(&ma->volume_lock_mutex);

Shouldn't we take the mutex before checking (and potentially changing)
the state?  Otherwise we've got TOCTOU issues.

> +static void macaudio_slk_unlock(struct snd_kcontrol *kcontrol)
> +{
> +	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
> +
> +	ma->speaker_lock_owner = NULL;
> +	ma->speaker_lock_timeout = 0;
> +	macaudio_vlimit_update(ma);
> +}

This seems to similarly be missing locking.

> +/*	enable	amp		speakers	stereo	gain	safe_vol */
> +struct macaudio_platform_cfg macaudio_j180_cfg = {
> +	false,	AMP_SN012776,	SPKR_1W1T,	false,	10,	-20,
> +};

These should be static, they're not used outside this file.  Probably
also const.

> +static const struct of_device_id macaudio_snd_device_id[]  = {

> +	/* j375    AID10   sn012776    15      1× 1W */
> +	{ .compatible = "apple,j375-macaudio", .data = &macaudio_j37x_j47x_cfg },

The j47x entries in the table use 20 not 15.

> +static void macaudio_snd_platform_remove(struct platform_device *pdev)
> +{
> +	struct macaudio_snd_data *ma = dev_get_drvdata(&pdev->dev);
> +
> +	cancel_delayed_work_sync(&ma->lock_timeout_work);
> +}

What ensures that lock_update_work is cancelled?

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