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

Mark Brown
2026-04-03T13:26:04Z

[-- Attachment #1: Type: text/plain, Size: 2855 bytes --]

On Wed, Apr 01, 2026 at 06:51:46PM +0530, Niranjan H Y wrote:

> +static int tac_sdw_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
> +{

> + /* Detect and set jack type for UAJ path before playback.
> + * This is required as jack is not triggering interrupt
> + * when the device is in suspended mode.
> + */
> + tac5xx2_sdca_headset_detect(tac_dev);

Do we need locking in case the device is not in suspended mode and this
happens to race with an interrupt?

> + ret = regmap_write(tac_dev->regmap, SDW_SDCA_CTL(function_id, pde_entity,
> + TAC_SDCA_REQUESTED_PS, 0),
> + 0x03);

Other writes here are protected by pde_lock.

> + 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);

Is it safe to only log this error, will the PCM have any chance of
working?

> +static int tac_interrupt_callback(struct sdw_slave *slave,
> + struct sdw_slave_intr_status *status)
> +{
> + struct tac5xx2_prv *tac_dev = dev_get_drvdata(&slave->dev);
> + struct device *dev = &slave->dev;
> + int ret = 0;
> + int btn_type = 0;
> + unsigned int sdca_int3;
> +

> + ret = regmap_read(tac_dev->regmap, SDW_SCP_SDCA_INT3, &sdca_int3);
> + if (ret) {
> + dev_err(dev, "Failed to read SDCA_INT3: %d", ret);
> + return ret;
> + }

Can an interrupt be generated during runtime suspend (or race with
another thread doing a runtime suspend)?

> + snd_soc_jack_report(tac_dev->hs_jack, tac_dev->jack_type | btn_type,
> + SND_JACK_HEADSET | SND_JACK_BTN_0 |
> + SND_JACK_BTN_1 | SND_JACK_BTN_2 |
> + SND_JACK_BTN_3 | SND_JACK_BTN_4);

The detection function detects line in/out but this doesn't include them
in the report mask.

> +static s32 tac_download_fw_to_hw(struct tac5xx2_prv *tac_dev)
> +{

> + offset = tac_fw_read_hdr(buf, hdr);
> + while (offset < img_sz && num_files < TAC_MAX_FW_CHUNKS) {
> + u32 file_length;
> +

> + if (file_length > img_sz || offset > img_sz - TAC_FW_FILE_HDR ||
> + file_length > img_sz - offset - TAC_FW_FILE_HDR) {
> + dev_warn(tac_dev->dev, "File at offset %d exceeds buffer: length=%u, available=%zu\n",
> + offset, file_length, img_sz - offset - TAC_FW_FILE_HDR);
> + break;
> + }

Shouldn't this jump out of the loop to the error handling like...

> + ret = tac_fw_get_next_file(&buf[offset], img_sz - offset, &files[num_files]);
> + if (ret < 0) {
> + dev_err(tac_dev->dev, "Failed to parse file at offset %d\n", offset);
> + goto out;
> + }

...this does?

> +static void tac_remove(struct tac5xx2_prv *tac_dev)
> +{
> + snd_soc_unregister_component(tac_dev->dev);
> +}

We used devm_snd_soc_register_component() so this should result in a
double free.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]