linux-kernel - Re: [PATCH v10] The tas2783 is a smart audio amplifier with integrated MIPI SoundWire interface (Version 1.2.1 compliant), I2C, and I2S/TDM interfaces designed for portable applications. An on-chip DSP supports Texas Instruments SmartAmp speaker protection algorithm. The integrated speaker voltage and current sense provides for real-time monitoring of loudspeakers.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 5 Mar 2024 19:18:59 +0000
From: Mark Brown <broonie@...nel.org>
To: Shenghao Ding <shenghao-ding@...com>
Cc: andriy.shevchenko@...ux.intel.com, lgirdwood@...il.com, perex@...ex.cz,
	pierre-louis.bossart@...ux.intel.com, 13916275206@....com,
	alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
	liam.r.girdwood@...el.com, bard.liao@...el.com,
	mengdong.lin@...el.com, yung-chuan.liao@...ux.intel.com,
	baojun.xu@...com, kevin-lu@...com, tiwai@...e.de, soyer@....hu,
	Baojun.Xu@....com, navada@...com
Subject: Re: [PATCH v10] The tas2783 is a smart audio amplifier with
 integrated MIPI SoundWire interface (Version 1.2.1 compliant), I2C, and
 I2S/TDM interfaces designed for portable applications. An on-chip DSP
 supports Texas Instruments SmartAmp speaker protection algorithm. The
 integrated speaker voltage and current sense provides for real-time
 monitoring of loudspeakers.

On Tue, Mar 05, 2024 at 04:43:35PM +0800, Shenghao Ding wrote:
> The ASoC component provides the majority of the functionality of the
> device, all the audio functions.

> +static const struct reg_default tas2783_reg_defaults[] = {
> +	/* Default values for ROM mode. Activated. */
> +	{ 0x8002, 0x1a },	/* AMP inactive. */
> +	{ 0x8097, 0xc8 },
> +	{ 0x80b5, 0x74 },
> +	{ 0x8099, 0x20 },
> +	{ 0xfe8d, 0x0d },
> +	{ 0xfebe, 0x4a },
> +	{ 0x8230, 0x00 },
> +	{ 0x8231, 0x00 },
> +	{ 0x8232, 0x00 },
> +	{ 0x8233, 0x01 },
> +	{ 0x8418, 0x00 },	/* Set volume to 0 dB. */
> +	{ 0x8419, 0x00 },
> +	{ 0x841a, 0x00 },
> +	{ 0x841b, 0x00 },
> +	{ 0x8428, 0x40 },	/* Unmute channel */
> +	{ 0x8429, 0x00 },
> +	{ 0x842a, 0x00 },
> +	{ 0x842b, 0x00 },
> +	{ 0x8548, 0x00 },	/* Set volume to 0 dB. */
> +	{ 0x8549, 0x00 },
> +	{ 0x854a, 0x00 },
> +	{ 0x854b, 0x00 },
> +	{ 0x8558, 0x40 },	/* Unmute channel */
> +	{ 0x8559, 0x00 },
> +	{ 0x855a, 0x00 },
> +	{ 0x855b, 0x00 },
> +	{ 0x800a, 0x3a },	/* Enable both channel */

These comments sound like this register default table is not actually
the physical default register values that the chip has...

> +static const struct regmap_config tasdevice_regmap = {
> +	.reg_bits = 32,
> +	.val_bits = 8,
> +	.readable_reg = tas2783_readable_register,
> +	.volatile_reg = tas2783_volatile_register,
> +	.max_register = 0x44ffffff,
> +	.reg_defaults = tas2783_reg_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(tas2783_reg_defaults),

..but this is set as the register defaults.  This will cause problems
with things like cache sync where we don't write values out if they're
not the defaults.  We also try to keep default settings from the silicon
except in the most obvious cases, it avoids issues with trying to work
out what to set and accomodate different use cases for different systems.

If you do need to set non-default values then either just regular writes
during probe or a regmap patch would do it.

> +	.cache_type = REGCACHE_RBTREE,
> +	.use_single_read = true,
> +	.use_single_write = true,

REGCACHE_MAPLE is generally the most sensible choice for modern devices
- it's a newer and fancier data structure underlying it and there's only
a few cases with low end devices, mostly doing block I/O which this
doesn't support, where the RBTREE cache is still better.

> +	u16 dev_info;
> +	int ret;
> +
> +	if (!tas_dev->sdw_peripheral) {
> +		dev_err(tas_dev->dev, "%s: peripheral doesn't exist.\n",
> +			__func__);
> +		return;
> +	}
> +
> +	dev_info = tas_dev->sdw_peripheral->bus->link_id |
> +		tas_dev->sdw_peripheral->id.unique_id << 16;

I'm kind of surprised dev_info works as a variable name without
something getting upset that it aliases the function of the same name.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)