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