Re: [PATCH v3 5/8] ASoC: cs35l36: Implement set_tdm_slot to program RX and TX slots

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ASoC: cs35l36: I2S bus contention from unmapped TDM channels
- [High] ASoC: cs35l36: DAPM kcontrol corrupts TDM slot configuration
- [Medium] ASoC: cs35l36: missing error propagation for invalid TDM mask
- [Low] ASoC: cs35l36: inverted TDM mask perspective in comment
--

commit caaab40c1180fd182ef9f5dce19a698193666d09
Author: David Heidelberg <david@ixit.cz>

ASoC: cs35l36: Implement set_tdm_slot to program RX and TX slots

This commit introduces the set_tdm_slot callback for the CS35L36 codec to
allow machine drivers to dynamically program ASP RX and TX slot assignments.
The new configuration maps each bit set in the provided TDM masks to
consecutive hardware channels, enforcing maximum channel limits.

> diff --git a/sound/soc/codecs/cs35l36.c b/sound/soc/codecs/cs35l36.c
> index 89645327945f1..4d35ca6291548 100644
> --- a/sound/soc/codecs/cs35l36.c
> +++ b/sound/soc/codecs/cs35l36.c
> @@ -947,10 +947,54 @@ static const struct cs35l36_pll_config *cs35l36_get_clk_config(
>  	return NULL;
>  }
>  
> +static void cs35l36_mask_to_slots(struct cs35l36_private *cs35l36,
> +				  unsigned long mask, unsigned int base_reg,
> +				  unsigned int nchan)
> +{
> +	unsigned int chan = 0, shift;
> +	int slot;
> +
> +	/* Two 6-bit slot fields per register, at bits 0 and 16 */
> +	for_each_set_bit(slot, &mask, BITS_PER_TYPE(mask)) {
> +		if (chan == nchan) {
> +			dev_warn(cs35l36->dev,
> +				 "Too many slots in TDM mask: %lx\n", mask);
> +			return;

[Severity: Medium]
Will this error state be lost since this function returns void?

If a machine driver passes a mask with more bits set than supported channels,
this warning prints, but cs35l36_set_tdm_slot() continues and unconditionally
returns 0.

Should this configuration failure be propagated back to the sound framework
by returning a negative error code instead?

> +		}
> +
> +		shift = (chan % 2) * CS35L36_ASP_TX2_SLOT_SHIFT;
> +		regmap_update_bits(cs35l36->regmap, base_reg + (chan / 2) * 4,
> +				   CS35L36_ASP_RX1_SLOT_MASK << shift,
> +				   slot << shift);
> +		chan++;
> +	}

[Severity: High]
Does this leave remaining channels unmapped on their default slots if the
provided mask specifies fewer slots than the total channels?

In a multi-codec setup, the CPU DAI typically opens the stream with the total
number of channels on the bus. DAPM then powers on all asptx widgets up to
that channel count.

If we only map a subset of slots here, the remaining powered-on channels
will retain their hardware default slots and actively transmit on them.

Could this lead to hardware contention on the I2S bus and data corruption by
colliding with properly mapped channels or other codecs?

> +}
> +
> +static int cs35l36_set_tdm_slot(struct snd_soc_dai *dai,
> +				unsigned int tx_mask, unsigned int rx_mask,
> +				int slots, int slot_width)
> +{
> +	struct cs35l36_private *cs35l36 =
> +			snd_soc_component_get_drvdata(dai->component);
> +
> +	/* Note: rx/tx is from point of view of the CPU end */

[Severity: Low]
Is this comment accurate? The implementation below maps rx_mask to the
codec's rx register and tx_mask to the codec's tx register.

If it were from the CPU's point of view, the CPU's rx mask would map to the
codec's tx. The code correctly follows ASoC semantics (from the codec's
perspective), which makes the comment appear inverted.

> +	if (!slots || !rx_mask)
> +		rx_mask = BIT(0);		/* ASPRX1 in slot 0 */
> +
> +	if (!slots || !tx_mask)
> +		tx_mask = GENMASK(7, 0);	/* ASPTX1..8 in slots 0..7 */
> +
> +	cs35l36_mask_to_slots(cs35l36, rx_mask, CS35L36_ASP_RX1_SLOT, 1);

[Severity: High]
Could programming CS35L36_ASP_RX1_SLOT here conflict with the existing
DAPM kcontrol?

There appears to be a legacy DAPM kcontrol (cs35l36_chan_mux / chansel_enum)
that operates on bit 0 of this exact same register to toggle between
rx1 and rx2.

If a machine driver sets the rx slot to a value like 2 (binary 000010) here,
and userspace later restores the Input Mux kcontrol, could the framework
forcefully toggle bit 0 and inadvertently change the slot configuration?

> +	cs35l36_mask_to_slots(cs35l36, tx_mask, CS35L36_ASP_TX1_TX2_SLOT, 8);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-pixel3-audio-v3-0-5d06460fc8a9@ixit.cz?part=5