-----Original Message----- From: Mark Brown broonie@kernel.org Sent: Wednesday, March 6, 2024 3:19 AM To: Ding, Shenghao shenghao-ding@ti.com Cc: andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com; perex@perex.cz; pierre-louis.bossart@linux.intel.com; 13916275206@139.com; alsa-devel@alsa-project.org; linux- kernel@vger.kernel.org; liam.r.girdwood@intel.com; bard.liao@intel.com; mengdong.lin@intel.com; yung-chuan.liao@linux.intel.com; Xu, Baojun baojun.xu@ti.com; Lu, Kevin kevin-lu@ti.com; tiwai@suse.de; soyer@irl.hu; Baojun.Xu@fpt.com; Navada Kanyana, Mukund navada@ti.com Subject: [EXTERNAL] 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 spe...

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.

So, can I remove ".reg_defaults = tas2783_reg_defaults," and tas2783_reg_defaults from the code?

...

• .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.

Accept

...

• 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.

Accept