| Message ID | 21a183b5a08cb23b193af78d4b1114cc59419272.1701906455.git.soyer@irl.hu |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: hda/tas2781: Add tas2563 support | expand |
On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote: > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > > > And only one, because tasdevice_change_chn_book directly changes the > > > > address of i2c_client, so the unlucky one gets invalid values in its > > > > actual book from regcache_sync. > > > > The code creates the impression that writing to one tas2781 writes to > > > all of them, is that not the case? > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > > this. > > So this syncing is done in software not hardware? My understanding was > that this was a hardware thing. If you mean that the amplifier does not know that there are several programs or configurations or profiles, but only runs the current one, yes. > > > > How would the devices get their configuration restored? > > > tasdevice_tuning_switch calls tasdevice_select_tuningprm_cfg which > > checks whether the devices needs a new program or configuration. > > > the runtime_suspend and system resume set the devices cur_prog, > > cur_conf to -1. > > ... > > > The tas2781_hda_playback_hook calls the tasdevice_tuning_switch > > And there are no registers other than these programs? The tas2781-hda writes 4 things: 1. Profiles from RCA file eg. INT8866RCA2.bin has 4 profile: Music degree 0 calibration voice call earpiece spk2 bypass The profiles contain pre-power-up and pre-shutdown register+value sequences for each amplifier. 2. Programs from DSP firmware. eg. TAS2XXX3870.bin has 1 program: Tuning Mode 3. Configurations from the DSP firmware. eg. TAS2XXX3870.bin has 2 configurations: configuration_Normal_Tuning Mode_48 KHz_s2_0 calibration_Tuning Mode_48 KHz_s2_0 Programs and configurations contain blocks with addresses where they should be written. 4. Calibration data from EFI variables. R0, INV_R0, R0LOW, POWER, TLIM, Based on the chip, they should be written to 5 registers. The code restores all of these in playback_hook, runtime_resume, system_resume functions without regmap_cache_sync.
On Fri, Dec 15, 2023 at 02:17:01AM +0100, Gergo Koteles wrote: > On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote: > > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > > The code creates the impression that writing to one tas2781 writes to > > > > all of them, is that not the case? > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > > > this. > > So this syncing is done in software not hardware? My understanding was > > that this was a hardware thing. > If you mean that the amplifier does not know that there are several > programs or configurations or profiles, but only runs the current one, > yes. No, I mean that the amplifiers don't talk to each other at a hardware level and the grouping is all in software.
On Fri, 2023-12-15 at 12:55 +0000, Mark Brown wrote: > On Fri, Dec 15, 2023 at 02:17:01AM +0100, Gergo Koteles wrote: > > On Thu, 2023-12-07 at 22:39 +0000, Mark Brown wrote: > > > On Thu, Dec 07, 2023 at 10:12:13PM +0100, Gergo Koteles wrote: > > > > On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote: > > > > > > The code creates the impression that writing to one tas2781 writes to > > > > > all of them, is that not the case? > > > > > Yes, the tasdevice_* functions, but the regcache_sync doesn't know > > > > this. > > > > So this syncing is done in software not hardware? My understanding was > > > that this was a hardware thing. > > > If you mean that the amplifier does not know that there are several > > programs or configurations or profiles, but only runs the current one, > > yes. > > No, I mean that the amplifiers don't talk to each other at a hardware > level and the grouping is all in software. No, they don't talk to each other. But they have a global i2c address to speed up configuration, but the module doesn't use it yet.
On Fri, Dec 15, 2023 at 03:42:43PM +0100, Gergo Koteles wrote: > On Fri, 2023-12-15 at 12:55 +0000, Mark Brown wrote: > > No, I mean that the amplifiers don't talk to each other at a hardware > > level and the grouping is all in software. > No, they don't talk to each other. But they have a global i2c address > to speed up configuration, but the module doesn't use it yet. That's hardware level synchronisation between the devices, that makes all this a bit less horrifying though it seems like a lot of the issues would go away if the broadcast write address were actually being used more.
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 077a01521eef..d272d3d08b29 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -717,8 +717,6 @@ static int tas2781_runtime_suspend(struct device *dev) tas_priv->tasdevice[i].cur_conf = -1; } - regcache_cache_only(tas_priv->regmap, true); - regcache_mark_dirty(tas_priv->regmap); mutex_unlock(&tas_priv->codec_lock); @@ -730,20 +728,11 @@ static int tas2781_runtime_resume(struct device *dev) struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); unsigned long calib_data_sz = tas_priv->ndev * TASDEVICE_SPEAKER_CALIBRATION_SIZE; - int ret; dev_dbg(tas_priv->dev, "Runtime Resume\n"); mutex_lock(&tas_priv->codec_lock); - regcache_cache_only(tas_priv->regmap, false); - ret = regcache_sync(tas_priv->regmap); - if (ret) { - dev_err(tas_priv->dev, - "Failed to restore register cache: %d\n", ret); - goto out; - } - tasdevice_prmg_load(tas_priv, tas_priv->cur_prog); /* If calibrated data occurs error, dsp will still works with default @@ -752,10 +741,9 @@ static int tas2781_runtime_resume(struct device *dev) if (tas_priv->cali_data.total_sz > calib_data_sz) tas2781_apply_calib(tas_priv); -out: mutex_unlock(&tas_priv->codec_lock); - return ret; + return 0; } static int tas2781_system_suspend(struct device *dev) @@ -770,10 +758,7 @@ static int tas2781_system_suspend(struct device *dev) return ret; /* Shutdown chip before system suspend */ - regcache_cache_only(tas_priv->regmap, false); tasdevice_tuning_switch(tas_priv, 1); - regcache_cache_only(tas_priv->regmap, true); - regcache_mark_dirty(tas_priv->regmap); /* * Reset GPIO may be shared, so cannot reset here. diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index ffb26e4a7e2f..933cd008e9f5 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -39,7 +39,7 @@ static const struct regmap_range_cfg tasdevice_ranges[] = { static const struct regmap_config tasdevice_regmap = { .reg_bits = 8, .val_bits = 8, - .cache_type = REGCACHE_RBTREE, + .cache_type = REGCACHE_NONE, .ranges = tasdevice_ranges, .num_ranges = ARRAY_SIZE(tasdevice_ranges), .max_register = 256 * 128,
The amp has 3 level addressing (BOOK, PAGE, REG). The regcache couldn't handle it. Signed-off-by: Gergo Koteles <soyer@irl.hu> --- sound/pci/hda/tas2781_hda_i2c.c | 17 +---------------- sound/soc/codecs/tas2781-comlib.c | 2 +- 2 files changed, 2 insertions(+), 17 deletions(-)