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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

On 5/4/26 14:57, 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>

Thanks for bearing with my reviews, this driver looks pretty good to me at this point

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>

I added additional comments that can be handled in future updates if desired.


> +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_slave *sdw_peripheral = tac_dev->sdw_peripheral;
> +	struct sdw_stream_runtime *sdw_stream;
> +	struct sdw_stream_config stream_config = {0};
> +	struct sdw_port_config port_config = {0};
> +	u8 sample_rate_idx = 0;
> +	int function_id;
> +	int pde_entity;
> +	int port_num;
> +	int ret;
> +
> +	if (!tac_dev->hw_init) {
> +		dev_err(tac_dev->dev,
> +			"error: operation without hw initialization");
> +		return -EINVAL;
> +	}
> +
> +	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);
> +
> +	switch (dai->id) {
> +	case TAC5XX2_DMIC:
> +		function_id = TAC_FUNCTION_ID_SM;
> +		pde_entity = TAC_SDCA_ENT_PDE11;
> +		port_num = TAC_SDW_PORT_NUM_DMIC;
> +		break;
> +	case 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;
> +		break;
> +	case 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;
> +		break;
> +	default:
> +		dev_err(tac_dev->dev, "Invalid dai id: %d for power up\n", dai->id);
> +		return -EINVAL;
> +	}
> +
> +	snd_sdw_params_to_config(substream, params, &stream_config, &port_config);
> +	port_config.num = port_num;
> +	ret = sdw_stream_add_slave(sdw_peripheral, &stream_config,
> +				   &port_config, 1, sdw_stream);
> +	if (ret) {
> +		dev_err(dai->dev,
> +			"Unable to configure port %d: %d\n", port_num, ret);
> +		return ret;
> +	}
> +
> +	switch (params_rate(params)) {
> +	case 48000:
> +		sample_rate_idx = 0x01;
> +		break;
> +	case 44100:
> +		sample_rate_idx = 0x02;
> +		break;
> +	case 96000:
> +		sample_rate_idx = 0x03;
> +		break;
> +	case 88200:
> +		sample_rate_idx = 0x04;
> +		break;
> +	default:
> +		dev_dbg(tac_dev->dev, "Unsupported sample rate: %d Hz",
> +			params_rate(params));
> +		return -EINVAL;
> +	}
> +
> +	switch (function_id) {
> +	case TAC_FUNCTION_ID_SM:
> +		ret = regmap_write(tac_dev->regmap,
> +				   SDW_SDCA_CTL(function_id, TAC_SDCA_ENT_CS113,
> +						TAC_SDCA_CTL_CS_SAMP_RATE_IDX, 0),
> +			sample_rate_idx);
> +		if (ret) {
> +			dev_err(tac_dev->dev, "Failed to set CS113 sample rate: %d", ret);
> +			return ret;
> +		}
> +
> +		break;
> +	case TAC_FUNCTION_ID_UAJ:
> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +			ret = regmap_write(tac_dev->regmap,
> +					   SDW_SDCA_CTL(function_id, TAC_SDCA_ENT_CS41,
> +							TAC_SDCA_CTL_CS_SAMP_RATE_IDX, 0),
> +					sample_rate_idx);
> +			if (ret) {
> +				dev_err(tac_dev->dev, "Failed to set CS41 sample rate: %d", ret);
> +				return ret;
> +			}
> +		} else {
> +			ret = regmap_write(tac_dev->regmap,
> +					   SDW_SDCA_CTL(function_id, TAC_SDCA_ENT_CS36,
> +							TAC_SDCA_CTL_CS_SAMP_RATE_IDX, 0),
> +					sample_rate_idx);
> +			if (ret) {
> +				dev_err(tac_dev->dev, "Failed to set CS36 sample rate: %d", ret);
> +				return ret;
> +			}
> +		}
> +		break;
> +	case TAC_FUNCTION_ID_SA:
> +		/* SmartAmp: no additional sample rate configuration needed */
> +		break;
> +	}
> +
> +	ret = regmap_write(tac_dev->regmap, SDW_SDCA_CTL(function_id, pde_entity,
> +							 TAC_SDCA_REQUESTED_PS, 0), 0);
> +	if (ret) {
> +		dev_err(tac_dev->dev,
> +			"failed to set func %d, entity %d's requested PS to 0: %d\n",
> +			function_id, pde_entity, ret);
> +		return ret;
> +	}
> +
> +	ret = sdca_asoc_pde_poll_actual_ps(tac_dev->dev, tac_dev->regmap, function_id, pde_entity,
> +					   SDCA_PDE_PS3, SDCA_PDE_PS0, NULL, 0);
> +	if (ret)
> +		dev_err(tac_dev->dev, "failed to transition func %d, pde %d from PS3 -> PS0, err=%d\n",
> +			function_id, pde_entity, ret);
> +	return ret;
> +}

In theory it can happen that hw_params() is invoked multiple times with different parameters, without hw_free() invoked.
You may want to double-check if it's ok to change the sample-rate index after moving to PS_0, IOW that it's safe to call hw_params() multiple times.


> +static int tac_port_prep(struct sdw_slave *slave, struct sdw_prepare_ch *prep_ch,
> +			 enum sdw_port_prep_ops pre_ops)
> +{
> +	struct device *dev = &slave->dev;
> +	struct tac5xx2_prv *tac_dev = dev_get_drvdata(dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (pre_ops != SDW_OPS_PORT_POST_PREP)
> +		return 0;
> +
> +	if (!tac_dev->fw_dl_success)
> +		return 0;
> +
> +	ret = regmap_read(tac_dev->regmap, TAC_DSP_ALGO_STATUS, &val);
> +	if (ret) {
> +		dev_err(dev, "Failed to read algo status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (val != TAC_DSP_ALGO_STATUS_RUNNING) {
> +		dev_dbg(dev, "Algo not running (0x%02x), re-enabling\n", val);
> +		ret = regmap_write(tac_dev->regmap, TAC_DSP_ALGO_STATUS,
> +				   TAC_DSP_ALGO_STATUS_RUNNING);

this part is interesting, when the port is prepared the DSP is set as RUNNING, but I don't see when the DSP would be stopped?

Let's say that all streams are stopped and all PDEs are in PS_3, wouldn't the DSP need to be managed at some point to reach a lower power?