Re: [PATCH v3 11/21] arm64: dts: apple: t8103-j313: Add speaker/headset jack nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] ASoC: apple: macaudio: macaudio_be_assign_tdm() skips idle_mode for 1-codec BE
- [Medium] arm64: dts: apple: t8103-j313: jack_codec supplies merge into one /regulator
- [Medium] ASoC: apple: macaudio: unreleased OF node refs in macaudio_parse_of()
--

commit 802339e5380e3a87c88c58c583fe94106f5d49a8
Author: James Calligeros <jcalligeros99@gmail.com>

arm64: dts: apple: t8103-j313: Add speaker/headset jack nodes

This commit adds the TI TAS5770L speaker amplifier nodes and Cirrus CS42L83
headset jack codec node to the M1 MacBook Air (J313) device tree and wires
them to the macaudio machine driver via the sound node.

> diff --git a/arch/arm64/boot/dts/apple/t8103-j313.dts b/arch/arm64/boot/dts/apple/t8103-j313.dts
> index f8b2b1637b9dc..ed0eb358809b4 100644
> --- a/arch/arm64/boot/dts/apple/t8103-j313.dts
> +++ b/arch/arm64/boot/dts/apple/t8103-j313.dts
> @@ -55,4 +55,91 @@ &typec1 {

[ ... ]

> +	jack_codec: codec@48 {
> +		compatible = "cirrus,cs42l83";
> +		reg = <0x48>;
> +		interrupts-extended = <&pinctrl_ap 183 IRQ_TYPE_LEVEL_LOW>;
> +		reset-gpios = <&pinctrl_nub 11 GPIO_ACTIVE_HIGH>;
> +		#sound-dai-cells = <0>;
> +		sound-name-prefix = "Jack";
> +		cirrus,ts-inv = <1>;
> +		VA-supply = <&jack_va_reg>;
> +		VP-supply = <&jack_vp_reg>;
> +		VCP-supply = <&jack_vcp_reg>;
> +		VD_FILT-supply = <&jack_vd_reg>;
> +		VL-supply = <&jack_vl_reg>;
> +	};
> +};

[Severity: Medium]
Do these five supply phandles on jack_codec resolve to five distinct
regulators in the compiled DTB?

In arch/arm64/boot/dts/apple/t8103-jxxx.dtsi (included by t8103-j313.dts),
all five fixed regulators are defined as direct children of / with the
identical node name regulator:

arch/arm64/boot/dts/apple/t8103-jxxx.dtsi:
	jack_va_reg: regulator {
		compatible = "regulator-fixed";
		regulator-name = "jack-va";
	};

	/* CS42L83 VCP and VCP_FILT */
	jack_vcp_reg: regulator {
		compatible = "regulator-fixed";
		regulator-name = "jack-vcp";
	};

	jack_vl_reg: regulator {
		compatible = "regulator-fixed";
		regulator-name = "jack-vl";
	};

	/* VD_FILT */
	jack_vd_reg: regulator {
		compatible = "regulator-fixed";
		regulator-name = "jack-vd";
	};

	jack_vp_reg: regulator {
		compatible = "regulator-fixed";
		regulator-name = "jack-vp";
	};

Because they share the same /regulator path, dtc merges all five definitions
into a single node, overwriting regulator-name to "jack-vp" and attaching
all five labels to that single node, so VA-supply, VP-supply, VCP-supply,
VD_FILT-supply, and VL-supply all point to the same regulator instance.

> +
> +/ {
> +	sound: sound {
> +		compatible = "apple,j313-macaudio";
> +		model = "MacBook Air J313";
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		dai-link@0 {
> +			reg = <0>;
> +
> +			dai-tdm-idle-mode-0 = "zero";
> +			dai-tdm-idle-mode-1 = "zero";
> +
> +			link-name = "Speakers";
> +
> +			codec {
> +				sound-dai = <&speaker_left>, <&speaker_right>;
> +			};
> +
> +			cpu {
> +				sound-dai = <&mca 0>, <&mca 1>;
> +			};
> +		};

[Severity: Medium]
Will dai-tdm-idle-mode-0 and dai-tdm-idle-mode-1 actually be applied to
speaker_left and speaker_right here?

In dai-link@0, there are 2 codecs (<&speaker_left>, <&speaker_right>) and
2 CPU DAIs (<&mca 0>, <&mca 1>). When macaudio_snd_platform_probe() ->
macaudio_parse_of() parses this node in sound/soc/apple/macaudio.c,
ncodecs_per_cpu is num_codecs / num_bes = 2 / 2 = 1. Two backend DAI links
are created with num_codecs = 1, and macaudio_get_codec_idle_props() stores
dai-tdm-idle-mode-0 and dai-tdm-idle-mode-1 into each backend link's
link_props->codecs[0].idle_mode.

However, when macaudio_be_init() -> macaudio_be_assign_tdm() runs, it takes
the single-codec early return without configuring idle_mode:

sound/soc/apple/macaudio.c:macaudio_be_assign_tdm() {
	...
	if (rtd->dai_link->num_codecs == 1) {
		ret = snd_soc_dai_set_tdm_slot(snd_soc_rtd_to_codec(rtd, 0), mask,
					       0, nslots, MACAUDIO_SLOTWIDTH);

		/*
		 * Headphones get a pass on -ENOTSUPP (see the comment
		 * around bclk_ratio value for primary FE).
		 */
		if (ret == -ENOTSUPP && props->is_headphones)
			return 0;

		return ret;
	}
	...
}

Because macaudio_be_assign_tdm() returns before calling
snd_soc_dai_set_tdm_idle(), tas2770_set_dai_tdm_idle() is never called for
either speaker amplifier on J313, leaving the SDOUT TDM idle slots
unconfigured while both codecs share the IV-sense capture TDM bus.

[Severity: Medium]
Also, can binding apple,j313-macaudio with this multi-BE dai-link@0 leak
device_node references in macaudio_parse_of() when probing defers or fails,
or on driver unbind?

First, in macaudio_parse_of_component(), of_parse_phandle_with_args()
acquires a reference on args.np and assigns it to comp->of_node before
calling snd_soc_get_dai_name():

sound/soc/apple/macaudio.c:macaudio_parse_of_component() {
	ret = of_parse_phandle_with_args(node, "sound-dai", "#sound-dai-cells",
						index, &args);
	if (ret)
		return ret;
	comp->of_node = args.np;
	return snd_soc_get_dai_name(&args, &comp->dai_name);
}

When speaker_left, speaker_right, jack_codec, or mca has not registered its
ASoC component yet, snd_soc_get_dai_name() returns -EPROBE_DEFER while
comp->of_node still holds the reference. In macaudio_parse_of(), the
for (be_index = 0; be_index < num_bes; be_index++) loop populates
card->dai_link[num_links + be_index], but num_links is only incremented by
num_bes after the entire be_index loop finishes:

sound/soc/apple/macaudio.c:macaudio_parse_of() {
		for (be_index = 0; be_index < num_bes; be_index++) {
			...
			ret = macaudio_parse_of_be_dai_link(card, link, be_index,
							    ncodecs_per_cpu, cpu, codec);
			if (ret)
				goto err_free;
			...
			link_props->codecs = devm_kcalloc(dev, ncodecs_per_cpu,
							  sizeof(struct ma_codec_idle), GFP_KERNEL);
			if (!link_props->codecs)
				return -ENOMEM;
			...
		}
		...
		num_links += num_bes;
}

When macaudio_parse_of_be_dai_link() jumps to err_free, the cleanup loop
only iterates up to num_links, missing the comp->of_node references acquired
in card->dai_link[num_links .. num_links + be_index] during the failing
iteration:

sound/soc/apple/macaudio.c:macaudio_parse_of() {
err_free:
	of_node_put(codec);
	of_node_put(cpu);
	of_node_put(np);

	if (!card->dai_link)
		return ret;

	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]);
	}
}

Second, if devm_kcalloc() for link_props->codecs fails inside
for_each_available_child_of_node(dev->of_node, np), macaudio_parse_of()
returns -ENOMEM directly instead of jumping to err_free, leaking np, cpu,
codec, and all previously parsed DAI link component node references.

Finally, once macaudio_parse_of() succeeds, neither a failure in
devm_snd_soc_register_card() in macaudio_snd_platform_probe() nor
macaudio_snd_platform_remove() calls snd_soc_of_put_dai_link_codecs() and
snd_soc_of_put_dai_link_cpus() on the backend DAI links.

[ ... ]

> -- 
> 2.43.0

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018-macaudio-v3-0-abdff7741dea@gmail.com?part=11