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@xxxxxxxxxx>
> Sent: Wednesday, March 6, 2024 3:19 AM
> To: Ding, Shenghao <shenghao-ding@xxxxxx>
> Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; lgirdwood@xxxxxxxxx;
> perex@xxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx;
> 13916275206@xxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; liam.r.girdwood@xxxxxxxxx; bard.liao@xxxxxxxxx;
> mengdong.lin@xxxxxxxxx; yung-chuan.liao@xxxxxxxxxxxxxxx; Xu, Baojun
> <baojun.xu@xxxxxx>; Lu, Kevin <kevin-lu@xxxxxx>; tiwai@xxxxxxx;
> soyer@xxxxxx; Baojun.Xu@xxxxxxx; Navada Kanyana, Mukund
> <navada@xxxxxx>
> 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