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

Pierre-Louis Bossart
2026-05-04T17:08:29Z

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?