Received-SPF: pass (google.com: domain of linux-kernel+bounces-92879-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 5 Mar 2024 19:18:59 +0000
From: Mark Brown <broonie@kernel.org>
To: Shenghao Ding <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,
	baojun.xu@ti.com, kevin-lu@ti.com, tiwai@suse.de, soyer@irl.hu,
	Baojun.Xu@fpt.com, navada@ti.com
Subject: 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.
Message-ID: <5637d141-d619-4910-bbe7-b30d3a7e7b97@sirena.org.uk>
References: <20240305084336.606-1-shenghao-ding@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="oKtf0G+iFmy3C6u7"
Content-Disposition: inline
In-Reply-To: <20240305084336.606-1-shenghao-ding@ti.com>


--oKtf0G+iFmy3C6u7
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

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.

--oKtf0G+iFmy3C6u7
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEreZoqmdXGLWf4p/qJNaLcl1Uh9AFAmXncCIACgkQJNaLcl1U
h9DlHwf/Xq+Guwgm78pQq6+8ERCajDkI2IbMJmvrL73zEB29/YvshNpE1wFd0VXZ
VVcz2IWrVhrBlaxUiDv+ByHSJp/jndWlYys6LwuczhXIXsWtV4v17ugHUYjBTXaP
W9mSGzcNFIO0IJmupeEuwjWD1ijHBwuc+fDROf2q+gS+NfVLO/S3h7M0TmyqneLE
k5GM4ZnNFPxNE2F6gDo4gs6PbQFomynEOFDDAtJGAoh1lFhc1RX13+1mDwwE8bYj
NTYBv7aMWKcgAcyxY6jIShRnm/zQgz8QmPNN6qNVdU80fc74WfOffCQea6zGs6It
As0BsfHOKypJ30EMKGyJ6f948zbXFA==
=WXwt
-----END PGP SIGNATURE-----

--oKtf0G+iFmy3C6u7--