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

[Mark Brown]

On Tue, Mar 31, 2026 at 08:46:36PM +0530, Niranjan H Y wrote:
> Add codec driver for tac5xx2 family of devices.


> +static int tac_cx11_get(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component;
> +	struct tac5xx2_prv *tac_dev;
> +
> +	ucontrol->value.enumerated.item[0] = 1; /* Default to DC:1 */
> +
> +	if (!kcontrol || !kcontrol->private_data)
> +		return 0;
> +
> +	component = snd_kcontrol_chip(kcontrol);
> +	if (!component)
> +		return 0;
> +
> +	tac_dev = snd_soc_component_get_drvdata(component);
> +	if (!tac_dev)
> +		return 0;

Shouldn't these be errors?

> +
> +static int tac_cx11_put(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_value *ucontrol)
> +{

> +	ret = regmap_write(tac_dev->regmap,
> +			   SDW_SDCA_CTL(TAC_FUNCTION_ID_SM, TAC_SDCA_ENT_CX11,
> +					TAC_SDCA_CTL_CX_CLK_SEL, 0),
> +			   val);
> +	if (ret) {
> +		dev_dbg(tac_dev->dev, "cx put failed");
> +		return -EIO;
> +	}

Probably nicer to return the error code you got here, and also include
it in the log message - that'll help people figure out what's going on
if things do go wrong.

> +/* Volume controls for mic, hp and mic cap */
> +static const struct snd_kcontrol_new tac5xx2_snd_controls[] = {
> +	SOC_SINGLE_RANGE_TLV("Left Amp Volume", TAC_AMP_LVL_CFG0, 2, 0, 44, 1,
> +			     tac5xx2_amp_tlv),
> +	SOC_SINGLE_RANGE_TLV("Right Amp Volume", TAC_AMP_LVL_CFG1, 2, 0, 44, 1,
> +			     tac5xx2_amp_tlv),

What's the logic behind two mono controls rather than stereo controls?
There's a bunch of these in the driver.

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

> +	if (dai->id == TAC5XX2_DMIC) {
> +	} else if (dai->id == TAC5XX2_UAJ) {
> +	} else if (dai->id == TAC5XX2_SPK) {
> +	} else {
> +	}

This should be a switch statement for clarity and scalability.

> +	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_err(tac_dev->dev, "Unsupported sample rate: %d Hz",
> +			params_rate(params));
> +		return -EINVAL;
> +	}

This allows userspace to spam the logs doesn't it?

> +	if (function_id == TAC_FUNCTION_ID_SM) {
> +	} else if (function_id == TAC_FUNCTION_ID_UAJ) {

Again, switch statement.  There's a bunch of these in teh driver.

> +static s32 tac_fw_read_hdr(const u8 *data, struct tac_fw_hdr *hdr)
> +{
> +	hdr->size = get_unaligned_le32(data);
> +
> +	return TAC_FW_HDR_SIZE;
> +}

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

> +	buf = tac_dev->fw_data;
> +	img_sz = tac_dev->fw_size;
> +
> +	dev_dbg(tac_dev->dev, "Downloading cached firmware to HW, size=%zu\n", img_sz);
> +
> +	offset += tac_fw_read_hdr(buf, hdr);
> +	if (hdr->size != img_sz) {

We don't validate that the file has enough data to read here, there's a
check after the request_firmware() that the file is non-zero but it
might just be a single byte.

> +	while (offset < img_sz && num_files < TAC_MAX_FW_CHUNKS) {
> +		if (offset + 20 > img_sz) {
> +			dev_warn(tac_dev->dev, "Incomplete block header at offset %d\n",
> +				 offset);
> +			break;
> +		}
> +		offset += tac_fw_get_next_file(&buf[offset], &files[num_files]);
> +		num_files++;
> +	}

I can't see anything that validates that we stay within the buffer here,
we're checking if the header is in the buffer but not the payload and if
we do overflow we just exit the loop and...

> +
> +	if (num_files == 0) {
> +		dev_err(tac_dev->dev, "firmware with no files\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}

...so long as we found at least one block we'll just carry on.

Attachment: signature.asc
Description: PGP signature