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

[Mark Brown]

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: signature.asc
Description: PGP signature