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

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 for Android: free password hash cracker in your pocket

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

Message-ID: <dff90323b41a4e3a8e0e8540fbb32154@ti.com>
Date: Thu, 7 Mar 2024 03:56:40 +0000
From: "Ding, Shenghao" <shenghao-ding@...com>
To: Mark Brown <broonie@...nel.org>
CC: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>,
        "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "perex@...ex.cz"
	<perex@...ex.cz>,
        "pierre-louis.bossart@...ux.intel.com"
	<pierre-louis.bossart@...ux.intel.com>,
        "13916275206@....com"
	<13916275206@....com>,
        "alsa-devel@...a-project.org"
	<alsa-devel@...a-project.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "liam.r.girdwood@...el.com"
	<liam.r.girdwood@...el.com>,
        "bard.liao@...el.com" <bard.liao@...el.com>,
        "mengdong.lin@...el.com" <mengdong.lin@...el.com>,
        "yung-chuan.liao@...ux.intel.com" <yung-chuan.liao@...ux.intel.com>,
        "Xu,
 Baojun" <baojun.xu@...com>, "Lu, Kevin" <kevin-lu@...com>,
        "tiwai@...e.de"
	<tiwai@...e.de>, "soyer@....hu" <soyer@....hu>,
        "Baojun.Xu@....com"
	<Baojun.Xu@....com>,
        "Navada Kanyana, Mukund" <navada@...com>
Subject: 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



> -----Original Message-----
> From: Mark Brown <broonie@...nel.org>
> Sent: Wednesday, March 6, 2024 3:19 AM
> To: Ding, Shenghao <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; Xu, Baojun
> <baojun.xu@...com>; Lu, Kevin <kevin-lu@...com>; tiwai@...e.de;
> soyer@....hu; Baojun.Xu@....com; Navada Kanyana, Mukund
> <navada@...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