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

[Mark Brown]

On Sat, Jul 18, 2026 at 01:34:00AM +0200, Gianluca Boiano wrote:

> The TAS2557 is a mono Class-D smart amplifier with an on-chip DSP for
> speaker protection. Audio is received over I2S/TDM (ASI) and processed
> by DSP firmware before reaching the Class-D output stage.

> +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 */

This looks awfully board specific....

> +static int tas2557_dev_bulk_write(struct tas2557_priv *tas2557, unsigned int chn,
> +				  unsigned int reg, const u8 *data, size_t len)
> +{
> +	int ret;
> +
> +	/* The chip auto-increments past reg 0x7f into the page register */
> +	if (TAS2557_PAGE_REG_ADDR(reg) + len > 128)
> +		return -EINVAL;

I didn't check but is there room for sharing code with the other
multi-book TI devices?

> +static int fw_parse_header(struct tas2557_priv *tas2557,
> +			   struct tas2557_firmware *fw,
> +			   const u8 *data, size_t size)
> +{

> +	fw->device = get_unaligned_be32(data); data += 4;
> +	if (fw->device != TAS2557_FW_DEVICE_MONO &&
> +	    fw->device != TAS2557_FW_DEVICE_STEREO) {
> +		dev_err(tas2557->dev,
> +			"unsupported device %u in firmware (expected 2=mono or 3=stereo)\n",
> +			fw->device);
> +		return -EINVAL;
> +	}

The tas2781 checks to see if there are one or two devices being
controlled by the driver and rejects a mismatch.  Also, is there some
overlap in firmware formats here?  Again I didn't actually check.

> +static int tas2557_set_program(struct tas2557_priv *tas2557,
> +			       unsigned int prog_idx, int config_idx)

> +	/* Load PLL configuration for this config (firmware-driven) */
> +	if (config->pll < fw->num_plls) {
> +		ret = tas2557_load_pll(tas2557, config->pll);
> +		if (ret < 0)
> +			return ret;
> +	}

Shouldn't an out of range value be an error?

> +static int tas2557_enable(struct tas2557_priv *tas2557, bool enable)
> +{

> +	} else if (!enable && tas2557->powered) {
> +		ret = tas2557_load_data_all(tas2557, tas2557_shutdown_data);
> +		if (ret < 0)
> +			dev_err(tas2557->dev, "shutdown failed: %d\n", ret);
> +
> +		tas2557->powered = false;
> +		tas2557->dac_muted = true;
> +		tas2557->sense_muted = true;
> +
> +		dev_dbg(tas2557->dev, "amplifier group powered off\n");
> +	}

Does sense_muted get handled properly for suspend?  The core mutes and
unmutes playback streams over suspend but not capture.

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

> +       /* Commit cached stream state now the hardware writes have succeeded */
> +       tas2557->sample_rate = rate;
> +       tas2557->wordlength = wordlength;

This won't actually take effect if the device doesn't power down - you
should probably not set use_pmdown_time to force a power down/up, though
that's annoying when the configuration is the same.

Attachment: signature.asc
Description: PGP signature