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.

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature