Re: [PATCH v1 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver

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

On Mon, Mar 23, 2026 at 09:45:45AM +0530, Niranjan H Y wrote:
> Add codec driver for tac5xx2 family of devices.
> This includes the support for
>   1. tac5572 - DAC, PDM, UAJ and HID
>   2. tas2883 - Amplifier with DSP
>   3. tac5672 - Similar to tac5572 with feedback
>   4. tac5682 - Similar to tac5672 with Amplifier DSP.
> 
> Signed-off-by: Niranjan H Y <niranjan.hy@ti.com>
> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
> +static int tac_fu_event(struct snd_soc_dapm_widget *w,
> +			struct snd_kcontrol *k, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct tac5xx2_prv *tac_dev = snd_soc_component_get_drvdata(component);
> +	int mute;
> +	int channel;
> +	int function_number, fu_entity;
> +
> +	/* right channel and mono case uses ch 2.*/
> +	if (strstr(w->name, "_L"))
> +		channel = TAC_CHANNEL_LEFT;
> +	else
> +		channel = TAC_CHANNEL_RIGHT;
> +
> +	if (strstr(w->name, "FU21")) {
> +		fu_entity = TAC_SDCA_ENT_FU21;
> +		function_number = TAC_FUNCTION_ID_SA;
> +	} else if (strstr(w->name, "FU23")) {
> +		fu_entity = TAC_SDCA_ENT_FU23;
> +		function_number = TAC_FUNCTION_ID_SA;
> +	} else if (strstr(w->name, "FU11")) {
> +		fu_entity = TAC_SDCA_ENT_FU11;
> +		function_number = TAC_FUNCTION_ID_SM;
> +	} else if (strstr(w->name, "FU113")) {
> +		fu_entity = TAC_SDCA_ENT_FU113;
> +		function_number = TAC_FUNCTION_ID_SM;
> +	} else if (strstr(w->name, "FU26")) {
> +		fu_entity = TAC_SDCA_ENT_FU26;
> +		function_number = TAC_FUNCTION_ID_SA;
> +	} else if (strstr(w->name, "FU13")) {
> +		fu_entity = TAC_SDCA_ENT_FU13;
> +		function_number = TAC_FUNCTION_ID_SM;
> +	} else if (strstr(w->name, "FU41")) {
> +		fu_entity = TAC_SDCA_ENT_FU41;
> +		function_number = TAC_FUNCTION_ID_UAJ;
> +	} else if (strstr(w->name, "FU36")) {
> +		fu_entity = TAC_SDCA_ENT_FU36;
> +		function_number = TAC_FUNCTION_ID_UAJ;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		mute = 0;
> +		break;
> +	case SND_SOC_DAPM_PRE_PMD:
> +		mute = 1;
> +		break;
> +	default:
> +		return 0;
> +	}
> +
> +	return regmap_write(tac_dev->regmap,
> +			       SDW_SDCA_CTL(function_number, fu_entity,
> +					    TAC_SDCA_CHANNEL_MUTE, channel),
> +			       mute);

If all this does is write the register why not just pass the
register to the DAPM widget and not both having a callback at
all?

Same for many of the other power callbacks.

> +static int tac_sdw_hw_params(struct snd_pcm_substream *substream,
> +			     struct snd_pcm_hw_params *params,
> +			     struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_component *component = dai->component;
> +	struct tac5xx2_prv *tac_dev = snd_soc_component_get_drvdata(component);
> +	struct sdw_stream_config stream_config = {0};
> +	struct sdw_port_config port_config = {0};
> +	struct sdw_stream_runtime *sdw_stream;
> +	struct sdw_slave *sdw_peripheral = tac_dev->sdw_peripheral;
> +	unsigned long time;
> +	int ret, retry;
> +	int function_id;
> +	int pde_entity;
> +	int port_num;
> +	u8 sample_rate_idx = 0;
> +
> +	time = wait_for_completion_timeout(&sdw_peripheral->initialization_complete,
> +					   msecs_to_jiffies(TAC5XX2_PROBE_TIMEOUT));
> +	if (!time) {
> +		dev_warn(tac_dev->dev, "%s: hw initialization timeout\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +	if (!tac_dev->hw_init) {
> +		dev_err(tac_dev->dev,
> +			"error: operation without hw initialization");
> +		return -EINVAL;
> +	}

This is a bit weird should these not be handled by runtime
resume? This happens in a couple other odd places too.

> +
> +	sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	if (!sdw_stream) {
> +		dev_err(tac_dev->dev, "failed to get dma data");
> +		return -EINVAL;
> +	}
> +
> +	ret = tac_clear_latch(tac_dev);
> +	if (ret)
> +		dev_warn(tac_dev->dev, "clear latch failed, err=%d", ret);
> +
> +	if (dai->id == TAC5XX2_DMIC) {
> +		function_id = TAC_FUNCTION_ID_SM;
> +		pde_entity = TAC_SDCA_ENT_PDE11;
> +		port_num = TAC_SDW_PORT_NUM_DMIC;
> +	} else if (dai->id == TAC5XX2_UAJ) {
> +		function_id = TAC_FUNCTION_ID_UAJ;
> +		pde_entity = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> +				TAC_SDCA_ENT_PDE47 : TAC_SDCA_ENT_PDE34;
> +		port_num = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> +				TAC_SDW_PORT_NUM_UAJ_PLAYBACK :
> +				TAC_SDW_PORT_NUM_UAJ_CAPTURE;
> +		/* Detect and set jack type for UAJ path before playback.
> +		 * This is required as jack is not triggering interrupt
> +		 * when the device is in suspended mode.
> +		 */
> +		tac5xx2_sdca_headset_detect(tac_dev);
> +	} else if (dai->id == TAC5XX2_SPK) {
> +		function_id = TAC_FUNCTION_ID_SA;
> +		pde_entity = TAC_SDCA_ENT_PDE23;
> +		port_num = substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> +				TAC_SDW_PORT_NUM_SPK_PLAYBACK :
> +				TAC_SDW_PORT_NUM_SPK_CAPTURE;
> +	} else {
> +		dev_err(tac_dev->dev, "Invalid dai id: %d", dai->id);
> +		return -EINVAL;
> +	}

Maybe worth considering sdca_asoc_get_port() although if you are
super confident your ports will never change it probably doesn't
add much.

> +
> +	ret = regmap_write(tac_dev->regmap, SDW_SDCA_CTL(function_id, pde_entity,
> +							 TAC_SDCA_REQUESTED_PS, 0),
> +			   0x03);

Agree with Pierre its a bit weird to be handling PDEs outside of
DAPM, at least a comment to explain why would make sense.

Thanks,
Charles