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

Holalu Yogendra, Niranjan
2026-04-20T16:18:48Z

> 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