Re: [RFC PATCH 8/8] ASoC: codecs: add Qualcomm Tambora (WCD9378) SDCA codec

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Thu, Jul 23, 2026 at 12:42:18AM +0100, Srinivas Kandagatla wrote:
> Add support for the Qualcomm Tambora (WCD9378) headset codec in SDCA
> mode over SoundWire.
> 
> WCD9378 ("Tambora") is a low-power stereo audio codec used across
> Qualcomm mobile and compute platforms.  It integrates a headphone
> amplifier, mono earpiece output, three analogue microphone inputs
> with independent programmable mic-bias supplies, jack and button
> detection, and SoundWire dataports for both playback and capture.
> 
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
> ---
> +	if (entity_id == WCD9378_PDE34_ENTITY_ID) {
> +		unsigned int mic_bias = SDW_SDCA_CTL(function_id,
> +						     WCD9378_IT33_ENTITY_ID,
> +						     WCD9378_IT33_MICBIAS_SEL, 0);
> +
> +		regcache_drop_region(regmap, mic_bias, mic_bias);
> +		ret = regmap_write(regmap, mic_bias, ctrl_it33_micbias_vals[0]);
> +		if (ret)
> +			dev_err(&slave->dev, "IT33 MIC_BIAS: %d\n", ret);
> +	} else if (entity_id == WCD9378_PDE47_ENTITY_ID) {
> +		/* Announce commit-group count for the FU42 writes in pde_post_pmu. */
> +		ret = regmap_write(core->dev_regmap,
> +				   WCD9378_ENT0_COMMIT_GROUP_COUNT_ADDR,
> +				   WCD9378_ENT0_COMMIT_GROUP_COUNT_TWO);
> +		if (ret)
> +			dev_err(&slave->dev, "Commit_Group_Count: %d\n", ret);
> +
> +		/* Let the master's WR FIFO drain before PDE47_REQ_PS=0. */
> +		usleep_range(2000, 2500);

Feels a bit odd using these callbacks for commit groups, really
that is a thing that should be implemented more generically. That
is a pretty serious task, so I can probably look past this as
vendor specific thing for now. I guess my only points would be
can you live without the commit groups and just use the current
values? And if not, are you prepared that there is a good chance
this gets missed when someone does implement commit groups and
this breaks.

> +	/*
> +	 * Unmute FU42 CH1/CH2 and stage 0 dB volume via the _CN (NEXT) alias
> +	 * so all four staged writes activate together on the SCP_COMMIT
> +	 * below.  Writes go to the vendor slave regmap because FU42
> +	 * addresses are outside the SDCA class regmap's routing.
> +	 */
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_MUTE_CH1_ADDR,
> +			   WCD9378_FU42_MUTE_UNMUTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 MUTE_CH1: %d\n", ret);
> +
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_MUTE_CH2_ADDR,
> +			   WCD9378_FU42_MUTE_UNMUTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 MUTE_CH2: %d\n", ret);
> +
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_CH_VOL_CH1_MSB_ADDR,
> +			   WCD9378_FU42_CH_VOL_UNITY_BYTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 CH1_MSB: %d\n", ret);
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_CH_VOL_CH1_LSB_ADDR,
> +			   WCD9378_FU42_CH_VOL_UNITY_BYTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 CH1_LSB: %d\n", ret);
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_CH_VOL_CH2_MSB_ADDR,
> +			   WCD9378_FU42_CH_VOL_UNITY_BYTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 CH2_MSB: %d\n", ret);
> +	ret = regmap_write(core->dev_regmap, WCD9378_FU42_CH_VOL_CH2_LSB_ADDR,
> +			   WCD9378_FU42_CH_VOL_UNITY_BYTE);
> +	if (ret)
> +		dev_err(&slave->dev, "FU42 CH2_LSB: %d\n", ret);

These should all really be part of the function regmap they are
function controls. Also would mean you have the MBQ so you don't
need to do two writes per register. I assume, with the commit
group stuff, these are writing the next value rather than the
current? But nothing here updates the functions regmap for the
new values, is it ok that those two get out of sync? Or do we
not define these controls at the function level?

Thanks,
Charles