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

[Holalu Yogendra, Niranjan]

> On 15:40-20260420, Pierre-Louis Bossart wrote:
> Subject: Re: [PATCH v9 2/4] ASoC: tac5xx2-sdw: add soundwire based codec driver

> > +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,
> > +
....

> > +		/* 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.

This is limitation in the current HW revision. So keeping this as temporary workaround
to make uaj playback work.

> 
> > +		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?

answer below ...
> > +static s32 tac_sdw_pcm_hw_free(struct snd_pcm_substream *substream,
> > +			       struct snd_soc_dai *dai)
> > +
> > +	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?

... during my testing, I have noted when the playback starts (from clock stop mode),
device gets "detached" first, tries to resume, .hw_params is called, then gets "attached".
I am checking slave's initialization completion variable in .hw_params to make sure that the
firmware download is complete before .hw_params proceeds.
On the "master" side, looks like the slave status is handled via workqueues.
So, still I wanted to keep one more protection, just in case, because of the above sequence, for serialization.

> > +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?

This is currently there because of workaround in .hw_params as it is called for all devices.

> > +
> > +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?

Yes, currently it  is. ( .set_jack = tac5xx2_set_jack )


> > +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?

Let me test this case. Thanks for the suggestion.

Regards
Niranjan