Re: [PATCH v2 2/2] ASoC: codecs: add TAS2557 mono smart amplifier driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect ASI data-slot offset calculation for the right channel skips the correct audio slot.
- [Medium] Missing SNS_CTRL_REG configuration when enabling ISENSE/VSENSE ALSA controls at runtime.
- [Medium] `tas2557_failsafe_recovery` unintentionally unmutes the capture path upon fault recovery.
- [Medium] `tas2557_set_dai_fmt()` updates DAC format registers but fails to update ADC format registers.
- [Medium] `tas2557_startup_data` hardcodes ASI2 DAC format but omits ASI1 DAC format configuration.
--

--- Patch [2]: [PATCH v2 2/2] ASoC: codecs: add TAS2557 mono smart amplifier driver ---
commit d1471370278591a7f73ca18bb2ac75981e6b8a92
Author: Gianluca Boiano <morf3089@gmail.com>
Subject: ASoC: codecs: add TAS2557 mono smart amplifier driver

The TAS2557 is a mono Class-D smart amplifier with an on-chip DSP for
speaker protection. It uses I2S/TDM and requires board-specific firmware
loaded per speaker design. Amplifier power is managed through DAPM and
includes fault interrupt recovery and volume controls.

> diff --git a/sound/soc/codecs/tas2557.c b/sound/soc/codecs/tas2557.c
> new file mode 100644
> --- /dev/null
> +++ b/sound/soc/codecs/tas2557.c
[ ... ]
> +static const unsigned int tas2557_startup_data[] = {
> +	TAS2557_GPI_PIN_REG,              0x15,	/* enable DIN, MCLK, CCI */
> +	TAS2557_GPIO1_PIN_REG,            0x01,	/* enable BCLK (ASI1) */
> +	TAS2557_GPIO2_PIN_REG,            0x01,	/* enable WCLK (ASI1) */
> +	/* ASI2 GPIO wiring; arm the BCLK source (GPIO5) before its dividers */
> +	TAS2557_GPIO6_PIN_REG,            0x01,	/* GPIO6 = ASI2 WCLK input */
> +	TAS2557_GPIO8_PIN_REG,            0x02,	/* GPIO8 = ASI2 DIN */
> +	TAS2557_GPIO5_PIN_REG,            0x01,	/* GPIO5 = ASI2 BCLK input */
> +	TAS2557_ASI2_DAC_FORMAT_REG,      0x18,	/* ASI2: 32-bit I2S */

[Severity: Medium]
Is it intentional to omit the equivalent formatting register for ASI1 here?

The comment above mentions both ASI interfaces are enabled and configured
identically, but if the board wiring uses ASI1, it looks like it would be left
at its reset default word length while ASI2 is explicitly set to 32-bit.

> +	TAS2557_ASI2_BDIV_CLK_SEL_REG,   0x01,
[ ... ]
> +static int tas2557_failsafe_recovery(struct tas2557_priv *tas2557)
> +{
[ ... ]
> +	if (tas2557->powered) {
> +		ret = tas2557_load_data(tas2557, tas2557_startup_data);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = tas2557_apply_runtime_state(tas2557);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = tas2557_load_config_post_power(tas2557);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!tas2557->dac_muted) {
> +			ret = tas2557_load_data(tas2557, tas2557_unmute_data);
> +			if (ret < 0)
> +				return ret;
> +		} else {

[Severity: Medium]
Does this sequence inadvertently unmute the capture path upon fault recovery?

When the DAC is unmuted but the capture stream is muted, calling
tas2557_load_data with tas2557_unmute_data clears the mute bits for both the
DAC and ISENSE paths.

Should this check tas2557->sense_muted and re-assert the capture mutes like
it does in tas2557_enable?

> +			/* Muted: clocks not guaranteed, keep detection off */
> +			ret = tas2557_clk_err_detect(tas2557, false);
[ ... ]
> +static int tas2557_hw_params(struct snd_pcm_substream *substream,
> +			     struct snd_pcm_hw_params *params,
> +			     struct snd_soc_dai *dai)
> +{
[ ... ]
> +	if (!tas2557->tdm_configured) {
> +		width = tas2557->tdm_slot_width > 0 ?
> +				tas2557->tdm_slot_width :
> +				params_physical_width(params);
> +		offset = (tas2557->channel == 0) ? 1 : 1 + width * 2;

[Severity: High]
Can this offset calculation skip the correct audio slot for the right channel?

For a 32-bit word, 1 + width * 2 evaluates to 65 BCLKs, which falls completely
outside the standard 64-bit stereo I2S frame where the right channel is
expected at offset 33.

> +		tas2557->asi_offset = offset;
> +	}
[ ... ]
> +static int tas2557_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
> +{
[ ... ]
> +	mutex_lock(&tas2557->lock);
> +	tas2557->dai_fmt = asi_fmt;
> +	ret = tas2557_dev_update_bits(tas2557, TAS2557_ASI1_DAC_FORMAT_REG,
> +				      TAS2557_FORMAT_MASK, asi_fmt);
> +	if (ret >= 0)
> +		ret = tas2557_dev_update_bits(tas2557,
> +					      TAS2557_ASI2_DAC_FORMAT_REG,
> +					      TAS2557_FORMAT_MASK, asi_fmt);

[Severity: Medium]
Should the ADC format registers be updated here as well?

This currently updates the DAC format registers (TAS2557_ASI1_DAC_FORMAT_REG
and TAS2557_ASI2_DAC_FORMAT_REG) but leaves the corresponding ADC format
registers unmodified at their reset defaults. Could this cause the capture
stream to use the wrong ASI data format?

> +	mutex_unlock(&tas2557->lock);
[ ... ]
> +static int tas2557_isense_put(struct snd_kcontrol *kc,
> +			      struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	tas2557->isense_enabled = en;
> +
> +	if (tas2557->powered)
> +		ret = tas2557_dev_update_bits(tas2557, TAS2557_POWER_CTRL2_REG,
> +					      TAS2557_ISENSE_ENABLE,
> +					      en ? TAS2557_ISENSE_ENABLE : 0);

[Severity: Medium]
If the amplifier powers up with ISENSE and VSENSE disabled, does enabling
them later via this control miss updating TAS2557_SNS_CTRL_REG?

In tas2557_apply_runtime_state, the slot routing is only written if at least
one sense line is already enabled. If the ALSA switches are turned on later,
the hardware might transmit on the default slot instead of the DT-configured
slot.

> +
> +	mutex_unlock(&tas2557->lock);
[ ... ]
> +static int tas2557_vsense_put(struct snd_kcontrol *kc,
> +			      struct snd_ctl_elem_value *ucontrol)
> +{
[ ... ]
> +	tas2557->vsense_enabled = en;
> +
> +	if (tas2557->powered)
> +		ret = tas2557_dev_update_bits(tas2557, TAS2557_POWER_CTRL2_REG,
> +					      TAS2557_VSENSE_ENABLE,
> +					      en ? TAS2557_VSENSE_ENABLE : 0);

[Severity: Medium]
Similarly, does toggling VSENSE power at runtime need to configure the
SNS_CTRL_REG routing if it wasn't done during startup?

> +
> +	mutex_unlock(&tas2557->lock);
[ ... ]
> +static int tas2557_i2c_probe(struct i2c_client *client)
> +{
[ ... ]
> +	/*
> +	 * Seed the cached ASI data-slot offset before any hw_params() or
> +	 * set_tdm_slot() call arrives; 32 is the fixed ASI word length
> +	 * used by tas2557_startup_data.
> +	 */
> +	tas2557->asi_offset = (tas2557->channel == 0) ? 1 : 1 + 32 * 2;

[Severity: High]
Does this seed the same incorrect right channel offset mentioned earlier in
tas2557_hw_params?

> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(tas2557_supplies),
> +					     tas2557_supplies);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711145722.188556-1-morf3089@gmail.com?part=2