Re: [PATCH v2 09/21] ASoC: apple: Add macaudio machine driver

[Cezary Rojewski <cezary.rojewski@intel.com>]

On 10/4/2026 8:03 AM, James Calligeros wrote:
> From: Martin Povišer <povik+lin@cutebit.org>
> 
> 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.
> 
> Add an ASoC machine driver for this platform.

> +struct ma_codec_idle {
> +	int idle_mode;
> +	u32 idle_mask;
> +};
> +
> +struct macaudio_snd_data {
> +	struct snd_soc_card card;
> +	struct snd_soc_jack jack;

Hi,

Typically storing both card and jack heralds that the design is off.
Especially when stored as fields. Looks to me as if you've made a one
structure to catch them all.

Let's start with the card. Below are the users - I'll skip dev_xxx()s as
that's an easy fix:

macaudio_dpcm_hw_params()
macaudio_be_hw_free()
	both can access the card from the substream/dai without
	relying on the ma-context

macaudio_parse_of()
macaudio_parse_of_be_dai_link()
	called in context which creates the very card

macaudio_vlimit_enable_timeout()
macaudio_vlimit_disable_timeout()
macaudio_vlimit_update()
	called in context of delayed part of the trigger() callback.
	I'd need more information on the trigger() implementation of
	yours to present a solution. See the comments below.

Skipped macaudio_vlimit_unlock() as it's called in context of
macaudio_vlimit_update() and macaudio_late_probe(). The latter has
direct access to the card while the former is mentioned above.


> +	int jack_plugin_state;
> +
> +	const struct macaudio_platform_cfg *cfg;
> +	bool has_speakers;
> +	bool has_sense;
> +	bool has_safety;
> +	unsigned int max_channels;
> +
> +	struct macaudio_link_props {
> +		/* frontend props */
> +		unsigned int bclk_ratio;
> +		bool is_sense;
> +
> +		/* backend props */
> +		bool is_speakers;
> +		bool is_headphones;
> +		unsigned int tdm_mask;
> +		struct ma_codec_idle *codecs;
> +	} *link_props;
> +
> +	int speaker_sample_rate;
> +	struct snd_kcontrol *speaker_sample_rate_kctl;
> +
> +	struct mutex be_link_mutex;
> +
> +	struct mutex volume_lock_mutex;
> +	bool speaker_volume_unlocked;
> +	bool speaker_volume_was_locked;
> +	struct snd_kcontrol *speaker_lock_kctl;
> +	u64 bes_active;
> +	bool speaker_lock_timeout_enabled;
> +	ktime_t speaker_lock_timeout;
> +	ktime_t speaker_lock_remain;
> +	struct delayed_work lock_timeout_work;
> +	struct work_struct lock_update_work;
> +
> +};

> +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];
> +
> +	guard(mutex)(&ma->be_link_mutex);

As already pointed out by Ajay, there is no 'nonatomic' flag set and
without such, you will hit a problem, eventually.

> +	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;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		schedule_work(&ma->lock_update_work);

trigger() should enable DMA work and/or signal other components
participating in the streaming to start their tasks too.

Please correct me if I'm wrong but it seems the scheduled work does
speaker-volume operations only. Moreover, it seems that
->lock_update_work may schedule a follow up work in form of
->lock_timeout_work. Are you sure trigger() is the right place to do
volume-control in delayed manner?

Once we clear the mist around the main structure and the trigger()
callback, I can happily review the "easier" parts of the patch.

> +	}
> +
> +	return 0;
> +}