RE: [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 sp

RE: [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 sp

[Ding, Shenghao]

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

Re: [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 sp

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Thu, Mar 07, 2024 at 03:56:40AM +0000, Ding, Shenghao wrote:

> > 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?

Yes, supplying register defaults is completely optional.  It means we
can't omit default values from cache sync and have to read values back
from the device the first time we read but those tend to be fairly minor
performance impacts.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]