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

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

> +struct tac5xx2_prv {
> +	struct snd_soc_component *component;
> +	struct sdw_slave *sdw_peripheral;
> +	struct sdca_function_data *sa_func_data;
> +	struct sdca_function_data *sm_func_data;
> +	struct sdca_function_data *uaj_func_data;
> +	struct sdca_function_data *hid_func_data;
> +	enum sdw_slave_status status;
> +	/* Lock for firmware download and PDE state transitions.
> +	 * Serializes FW caching/download and DAPM-driven power
> +	 * state changes to prevent PDE operations during firmware load.
> +	 */
> +	struct mutex pde_lock;

that's a lot of stuff that's protected with this lock. See below for one question...


> +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;
> +	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_MS));
> +	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;
> +	}
> +
> +	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;
> +		/* Detect and set jack type for UAJ path before playback.
> +		 * This is required as jack detection does not trigger interrupt
> +		 * when device is in runtime_pm suspend with bus in clock stop mode.
> +		 */

so here we have an interesting logic - or I misunderstood the comment?

If a headset is inserted when the device is in runtime_pm suspend, how would applications modify the routing and select playback on the headset, which would then ripple down to this hw_params() call?

IOW to play on a headset you first have to know there's a headset.

> +		mutex_lock(&tac_dev->uaj_lock);
> +		tac5xx2_sdca_headset_detect(tac_dev);
> +		mutex_unlock(&tac_dev->uaj_lock);
> +		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", 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;
> +	}
> +
> +	guard(mutex)(&tac_dev->pde_lock);

question I mentioned above: when you reach the hw_params phase, do you really have a potential race with firmware download? What does this specific use of pde_lock protect against?

> +	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 PS to 0: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sdca_asoc_pde_ensure_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 to PS0, err= %d\n", ret);
> +	return ret;
> +}
> +
> +static s32 tac_sdw_pcm_hw_free(struct snd_pcm_substream *substream,
> +			       struct snd_soc_dai *dai)
> +{
> +	s32 ret;
> +	struct snd_soc_component *component = dai->component;
> +	struct tac5xx2_prv *tac_dev =
> +		snd_soc_component_get_drvdata(component);
> +	struct sdw_stream_runtime *sdw_stream =
> +		snd_soc_dai_get_dma_data(dai, substream);
> +	int pde_entity, function_id;
> +
> +	sdw_stream_remove_slave(tac_dev->sdw_peripheral, sdw_stream);
> +
> +	switch (dai->id) {
> +	case TAC5XX2_DMIC:
> +		pde_entity = TAC_SDCA_ENT_PDE11;
> +		function_id = TAC_FUNCTION_ID_SM;
> +		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;
> +		break;
> +	default:
> +		function_id = TAC_FUNCTION_ID_SA;
> +		pde_entity = TAC_SDCA_ENT_PDE23;
> +		break;
> +	}
> +
> +	guard(mutex)(&tac_dev->pde_lock);

same here, do you really have a race with firmware download?

Or is this a case of dependencies between functions that requires all power state transitions to be serialized?

> +	ret = regmap_write(tac_dev->regmap,
> +			   SDW_SDCA_CTL(function_id, pde_entity, TAC_SDCA_REQUESTED_PS, 0),
> +			   SDCA_PDE_PS3);
> +	if (ret)
> +		return ret;
> +
> +	ret = sdca_asoc_pde_ensure_ps(tac_dev->dev, tac_dev->regmap,
> +				      function_id, pde_entity,
> +				      SDCA_PDE_PS0, SDCA_PDE_PS3,
> +				      NULL, 0);
> +	if (ret)
> +		dev_err(tac_dev->dev, "failed to trasition from PS0 to PS3");
> +	return ret;
> +}
> +
> +static const struct snd_soc_dai_ops tac_dai_ops = {
> +	.hw_params = tac_sdw_hw_params,
> +	.hw_free = tac_sdw_pcm_hw_free,
> +	.set_stream = tac_set_sdw_stream,
> +	.shutdown = tac_sdw_shutdown,
> +};
> +
> +static int tac5xx2_sdca_btn_type(unsigned char *buffer, struct tac5xx2_prv *tac_dev)
> +{
> +	switch (*buffer) {
> +	case 1: /* play pause */
> +		return SND_JACK_BTN_0;
> +	case 10: /* vol down */
> +		return SND_JACK_BTN_3;
> +	case 8: /* vol up */
> +		return SND_JACK_BTN_2;
> +	case 4: /* long press */
> +		return SND_JACK_BTN_1;
> +	case 2: /* next song */
> +	case 32: /* next song */
> +		return SND_JACK_BTN_4;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int tac5xx2_sdca_button_detect(struct tac5xx2_prv *tac_dev)
> +{
> +	unsigned int btn_type, offset, idx;
> +	int ret, value, owner;
> +	u8 buf[2];
> +
> +	ret = regmap_read(tac_dev->regmap,
> +			  SDW_SDCA_CTL(TAC_FUNCTION_ID_HID, TAC_SDCA_ENT_HID1,
> +				       TAC_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), &owner);
> +	if (ret) {
> +		dev_err(tac_dev->dev,
> +			"Failed to read current UMP message owner 0x%x", ret);
> +		return ret;
> +	}
> +
> +	if (owner == SDCA_UMP_OWNER_DEVICE) {
> +		dev_dbg(tac_dev->dev, "skip button detect as current owner is not host\n");
> +		return 0;
> +	}
> +
> +	ret = regmap_read(tac_dev->regmap,
> +			  SDW_SDCA_CTL(TAC_FUNCTION_ID_HID, TAC_SDCA_ENT_HID1,
> +				       TAC_SDCA_CTL_HIDTX_MESSAGE_OFFSET, 0), &offset);
> +	if (ret) {
> +		dev_err(tac_dev->dev,
> +			"Failed to read current UMP message offset: %d", ret);
> +		goto end_btn_det;
> +	}
> +
> +	dev_dbg(tac_dev->dev, "button detect: message offset = %x", offset);
> +
> +	for (idx = 0; idx < sizeof(buf); idx++) {
> +		ret = regmap_read(tac_dev->regmap,
> +				  TAC_BUF_ADDR_HID1 + offset + idx, &value);
> +		if (ret) {
> +			dev_err(tac_dev->dev,
> +				"Failed to read HID buffer: %d", ret);
> +			goto end_btn_det;
> +		}
> +		buf[idx] = value & 0xff;
> +	}
> +
> +	if (buf[0] == 0x1) {
> +		btn_type = tac5xx2_sdca_btn_type(&buf[1], tac_dev);
> +		ret = btn_type;
> +	}
> +
> +end_btn_det:
> +	regmap_write(tac_dev->regmap,
> +		     SDW_SDCA_CTL(TAC_FUNCTION_ID_HID, TAC_SDCA_ENT_HID1,
> +				  TAC_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
> +
> +	return ret;
> +}
> +
> +static int tac5xx2_sdca_headset_detect(struct tac5xx2_prv *tac_dev)
> +{
> +	int val, ret;
> +
> +	if (!tac_has_uaj_support(tac_dev))
> +		return 0;

can this really happen? usually you try to detect a headset if the device is capable of dealing with headsets, no?
Should this test be moved at a higher level before you enable low-level handling of headset stuff?

> +	ret = regmap_read(tac_dev->regmap,
> +			  SDW_SDCA_CTL(TAC_FUNCTION_ID_UAJ, TAC_SDCA_ENT_GE35,
> +				       TAC_SDCA_CTL_DET_MODE, 0), &val);
> +	if (ret) {
> +		dev_err(tac_dev->dev, "Failed to read the detect mode");
> +		return ret;
> +	}
> +
> +	switch (val) {
> +	case 4:
> +		tac_dev->jack_type = SND_JACK_MICROPHONE;
> +		break;
> +	case 5:
> +		tac_dev->jack_type = SND_JACK_HEADPHONE;
> +		break;
> +	case 6:
> +		tac_dev->jack_type = SND_JACK_HEADSET;
> +		break;
> +	case 0:
> +	default:
> +		tac_dev->jack_type = 0;
> +		break;
> +	}
> +
> +	ret = regmap_write(tac_dev->regmap,
> +			   SDW_SDCA_CTL(TAC_FUNCTION_ID_UAJ, TAC_SDCA_ENT_GE35,
> +					TAC_SDCA_CTL_SEL_MODE, 0), val);
> +	if (ret)
> +		dev_err(tac_dev->dev, "Failed to update the jack type to device");
> +
> +	return 0;
> +}
> +
> +static int tac5xx2_set_jack(struct snd_soc_component *component,
> +			    struct snd_soc_jack *hs_jack, void *data)
> +{
> +	struct tac5xx2_prv *tac_dev = snd_soc_component_get_drvdata(component);
> +	int ret;
> +
> +	if (!tac_has_uaj_support(tac_dev))
> +		return 0;

same here, shouldn't set_jack() be added to the component callbacks before probe?

> +	guard(mutex)(&tac_dev->uaj_lock);
> +	if (!hs_jack) {
> +		if (tac_dev->hs_jack) {
> +			tac_dev->hs_jack = NULL;
> +			ret = 0;
> +			goto disable_interrupts;
> +		}
> +		return 0;
> +	}
> +
> +	tac_dev->hs_jack = hs_jack;
> +	if (!tac_dev->hw_init) {
> +		dev_err(tac_dev->dev, "jack init failed, hw not initialized");
> +		return 0;
> +	}
> +
> +	ret = regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK2,
> +			   SDW_SCP_SDCA_INTMASK_SDCA_11);
> +	if (ret) {
> +		dev_warn(tac_dev->dev,
> +			 "Failed to register jack detection interrupt");
> +		goto disable_interrupts;
> +	}
> +
> +	ret = regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK3,
> +			   SDW_SCP_SDCA_INTMASK_SDCA_16);
> +	if (ret) {
> +		dev_warn(tac_dev->dev,
> +			 "Failed to register for button detect interrupt");
> +		goto disable_interrupts;
> +	}
> +
> +	return 0;
> +
> +disable_interrupts:
> +	/* ignore errors while disabling interrupts */
> +	regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK2, 0);
> +	regmap_write(tac_dev->regmap, SDW_SCP_SDCA_INTMASK3, 0);
> +
> +	return ret;
> +}

> +static const struct snd_soc_component_driver soc_codec_driver_tacdevice = {
> +	.probe = tac_component_probe,
> +	.remove = tac_component_remove,
> +	.controls = tac5xx2_snd_controls,
> +	.num_controls = ARRAY_SIZE(tac5xx2_snd_controls),
> +	.dapm_widgets = tac5xx2_common_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(tac5xx2_common_widgets),
> +	.dapm_routes = tac5xx2_common_routes,
> +	.num_dapm_routes = ARRAY_SIZE(tac5xx2_common_routes),
> +	.idle_bias_on = 0,
> +	.endianness = 1,
> +	.set_jack = tac5xx2_set_jack,

maybe make this dynamic and only populate .set_jack in tac_init() below when you can deal with a jack?

> +};
> +
> +static s32 tac_init(struct tac5xx2_prv *tac_dev)
> +{
> +	s32 ret;
> +	struct snd_soc_dai_driver *dai_drv;
> +	int num_dais;
> +
> +	dev_set_drvdata(tac_dev->dev, tac_dev);
> +
> +	switch (tac_dev->part_id) {
> +	case 0x5572:
> +		dai_drv = tac5572_dai_driver;
> +		num_dais = ARRAY_SIZE(tac5572_dai_driver);
> +		break;
> +	case 0x5672:
> +		dai_drv = tac5672_dai_driver;
> +		num_dais = ARRAY_SIZE(tac5672_dai_driver);
> +		break;
> +	case 0x5682:
> +		dai_drv = tac5682_dai_driver;
> +		num_dais = ARRAY_SIZE(tac5682_dai_driver);
> +		break;
> +	case 0x2883:
> +		dai_drv = tas2883_dai_driver;
> +		num_dais = ARRAY_SIZE(tas2883_dai_driver);
> +		break;
> +	default:
> +		dev_err(tac_dev->dev, "Unsupported device: 0x%x\n",
> +			tac_dev->part_id);
> +		return -EINVAL;
> +	}
> +
> +	ret = devm_snd_soc_register_component(tac_dev->dev,
> +					      &soc_codec_driver_tacdevice,
> +					      dai_drv, num_dais);
> +	if (ret) {
> +		dev_err(tac_dev->dev, "%s: codec register error:%d.\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}