[PATCH 00/16] ALSA: hda/tas2781: Add tas2563 support
The tas2781-hda driver can be modified to support tas2563 as well. Before knowing this information, I created another series for a new driver. https://lore.kernel.org/lkml/cover.1701733441.git.soyer@irl.hu/
This series now extends tas2781-hda, addresses differences and fixes various bugs.
The tas2563 is a smart amplifier. Similar to tas2562 but with DSP. Some Lenovo laptops have it to drive the bass speakers. By default, it is in software shutdown state.
To make the DSP work it needs a firmware and some calibration data. The latter can be read from the EFI in Lenovo laptops.
For the correct configuration it needs additional register data. It captured after running the Windows driver.
The firmware can be extracted as TAS2563Firmware.bin from the Windows driver with innoextract. https://download.lenovo.com/consumer/mobiles/h5yd037fbfyy7kd0.exe
The driver will search for it as TAS2XXX3870.bin with the 14ARB7. The captured registers extracted with TI's regtool: https://github.com/soyersoyer/tas2563rca/raw/main/INT8866RCA2.bin
Gergo Koteles (16): ASoC: tas2781: add support for fw version 0x0503 ALSA: hda/tas2781: leave hda_component in usable state ASoC: tas2781: disable regmap regcache ALSA: hda/tas2781: handle missing calibration data ALSA: hda/tas2781: fix typos in comment ASoC: tas2781: add ptrs to calibration functions ALSA: hda/tas2781: load_calibration just load ASoC: tas2781: add configurable global_addr ALSA: hda/tas2781: add TAS2563 support for 14ARB7 ASoC: tas2781: check negative indexes ASoC: tas2781: use 0 as default prog/conf index ASoC: tas2781: move set_drv_data outside tasdevice_init ALSA: hda/tas2781: remove sound controls in unbind ALSA: hda/tas2781: call cleaner functions only once ALSA: hda/tas2781: reset the amp before component_add ALSA: hda/tas2781: configure the amp after firmware load
include/sound/tas2781.h | 8 + sound/pci/hda/tas2781_hda_i2c.c | 364 +++++++++++++++++++----------- sound/soc/codecs/tas2781-comlib.c | 23 +- sound/soc/codecs/tas2781-fmwlib.c | 11 +- sound/soc/codecs/tas2781-i2c.c | 2 + 5 files changed, 270 insertions(+), 138 deletions(-)
base-commit: ffc253263a1375a65fa6c9f62a893e9767fbebfa
It is compatible with the fw version 0x0502. Already supported by TI's tas2781-linux-driver tree. https://git.ti.com/cgit/tas2781-linux-drivers/tas2781-linux-driver
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/soc/codecs/tas2781-fmwlib.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c index eb55abae0d7b..20dc2df034e9 100644 --- a/sound/soc/codecs/tas2781-fmwlib.c +++ b/sound/soc/codecs/tas2781-fmwlib.c @@ -2012,6 +2012,7 @@ static int tasdevice_dspfw_ready(const struct firmware *fmw, case 0x301: case 0x302: case 0x502: + case 0x503: tas_priv->fw_parse_variable_header = fw_parse_variable_header_kernel; tas_priv->fw_parse_program_data =
Unloading then loading the module causes a panic. Set only previously modified fields to null.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index fb802802939e..077a01521eef 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -613,8 +613,11 @@ static void tas2781_hda_unbind(struct device *dev, struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); struct hda_component *comps = master_data;
- if (comps[tas_priv->index].dev == dev) - memset(&comps[tas_priv->index], 0, sizeof(*comps)); + if (comps[tas_priv->index].dev == dev) { + comps[tas_priv->index].dev = NULL; + strscpy(comps->name, "", sizeof(comps->name)); + comps[tas_priv->index].playback_hook = NULL; + }
tasdevice_config_info_remove(tas_priv); tasdevice_dsp_remove(tas_priv);
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(-)
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,
On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:
The amp has 3 level addressing (BOOK, PAGE, REG). The regcache couldn't handle it.
So the books aren't currently used so the driver actually works?
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.
How can this work over system suspend? This just removes the cache with no replacement so if the device looses power over suspend (which seems likely) then all the register state will be lost. A similar issue may potentially exist over runtime suspend on an ACPI system with sufficiently heavily optimised power management.
On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote:
On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:
The amp has 3 level addressing (BOOK, PAGE, REG). The regcache couldn't handle it.
So the books aren't currently used so the driver actually works?
It writes to the book 0 and 8c. The initialization works with regcache, because it writes also the i2c devices.
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.
How can this work over system suspend? This just removes the cache with no replacement so if the device looses power over suspend (which seems likely) then all the register state will be lost. A similar issue may potentially exist over runtime suspend on an ACPI system with sufficiently heavily optimised power management.
In runtime_resume, only one of the two amplifiers goes back. The runtime_suspend sets the current book/prog/conf to -1 on all devices, and tas2781_hda_playback_hook will restore the program/configuration/profile with tasdevice_tuning_switch.
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.
system_restore doesn't work at all, because regcache_cache_only stays true since system_suspend.
It works without the regcache functions.
On Thu, Dec 07, 2023 at 09:19:34PM +0100, Gergo Koteles wrote:
On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote:
On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:
The amp has 3 level addressing (BOOK, PAGE, REG). The regcache couldn't handle it.
So the books aren't currently used so the driver actually works?
It writes to the book 0 and 8c. The initialization works with regcache, because it writes also the i2c devices.
I can't see any references to 0x8c in the driver?
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);
How can this work over system suspend? This just removes the cache with no replacement so if the device looses power over suspend (which seems likely) then all the register state will be lost. A similar issue may potentially exist over runtime suspend on an ACPI system with sufficiently heavily optimised power management.
In runtime_resume, only one of the two amplifiers goes back. The runtime_suspend sets the current book/prog/conf to -1 on all devices, and tas2781_hda_playback_hook will restore the program/configuration/profile with tasdevice_tuning_switch.
What does "go back" mean?
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?
system_restore doesn't work at all, because regcache_cache_only stays true since system_suspend.
Presumably the next runtime resume would make the device writable again?
It works without the regcache functions.
How would the devices get their configuration restored?
This sounds very much like a case of something working for your specific system in your specific test through some external factor rather than working by design, whatever problems might exist it seems fairly obvious to inspection that this patch would make things worse for other systems.
At a minimum this patch needs a much clearer changelog (all the patches I looked at could use clearer changelogs) which explains what's going on here, I would really expect to see something that replaces the use of the cache sync to restore the device state for example.
On Thu, 2023-12-07 at 20:36 +0000, Mark Brown wrote:
On Thu, Dec 07, 2023 at 09:19:34PM +0100, Gergo Koteles wrote:
On Thu, 2023-12-07 at 18:20 +0000, Mark Brown wrote:
On Thu, Dec 07, 2023 at 12:59:44AM +0100, Gergo Koteles wrote:
The amp has 3 level addressing (BOOK, PAGE, REG). The regcache couldn't handle it.
So the books aren't currently used so the driver actually works?
It writes to the book 0 and 8c. The initialization works with regcache, because it writes also the i2c devices.
I can't see any references to 0x8c in the driver?
The firmware has different programs. The programs have blocks, that the driver writes to the amplifier. The address comes from the blocks.
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);
How can this work over system suspend? This just removes the cache with no replacement so if the device looses power over suspend (which seems likely) then all the register state will be lost. A similar issue may potentially exist over runtime suspend on an ACPI system with sufficiently heavily optimised power management.
In runtime_resume, only one of the two amplifiers goes back. The runtime_suspend sets the current book/prog/conf to -1 on all devices, and tas2781_hda_playback_hook will restore the program/configuration/profile with tasdevice_tuning_switch.
What does "go back" mean?
Sorry for imprecise wording. The speaker is silent, I didn't checked why, maybe the amp is in error state or something.
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.
system_restore doesn't work at all, because regcache_cache_only stays true since system_suspend.
Presumably the next runtime resume would make the device writable again?
Yes, then one of the speakers works.
It works without the regcache functions.
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.
for (i = 0; i < tas_hda->priv->ndev; i++) { tas_hda->priv->tasdevice[i].cur_book = -1; tas_hda->priv->tasdevice[i].cur_prog = -1; tas_hda->priv->tasdevice[i].cur_conf = -1; }
And the tasdevice_select_tuningprm_cfg checks with if (tas_priv->tasdevice[i].cur_prog != prm_no ...
If needed, it writes the new program/configuration to the device.
The tas2781_hda_playback_hook calls the tasdevice_tuning_switch
This sounds very much like a case of something working for your specific system in your specific test through some external factor rather than working by design, whatever problems might exist it seems fairly obvious to inspection that this patch would make things worse for other systems.
At a minimum this patch needs a much clearer changelog (all the patches I looked at could use clearer changelogs) which explains what's going on here, I would really expect to see something that replaces the use of the cache sync to restore the device state for example.
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.
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?
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.
Also check the return value of the first get_variable call.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index d272d3d08b29..7c286802b273 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -455,9 +455,9 @@ static int tas2781_save_calibration(struct tasdevice_priv *tas_priv) status = efi.get_variable(efi_name, &efi_guid, &attr, &tas_priv->cali_data.total_sz, tas_priv->cali_data.data); - if (status != EFI_SUCCESS) - return -EINVAL; } + if (status != EFI_SUCCESS) + return -EINVAL;
tmp_val = (unsigned int *)tas_priv->cali_data.data;
Correct typos.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 7c286802b273..70085177230e 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -421,9 +421,9 @@ static void tas2781_apply_calib(struct tasdevice_priv *tas_priv) } }
-/* Update the calibrate data, including speaker impedance, f0, etc, into algo. +/* Update the calibration data, including speaker impedance, f0, etc, into algo. * Calibrate data is done by manufacturer in the factory. These data are used - * by Algo for calucating the speaker temperature, speaker membrance excursion + * by Algo for calcucating the speaker temperature, speaker membrane excursion * and f0 in real time during playback. */ static int tas2781_save_calibration(struct tasdevice_priv *tas_priv)
rename save_calibration to load_calibration
Signed-off-by: Gergo Koteles soyer@irl.hu --- include/sound/tas2781.h | 5 +++++ sound/pci/hda/tas2781_hda_i2c.c | 28 ++++++++++++---------------- sound/soc/codecs/tas2781-comlib.c | 15 +++++++++++++++ 3 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index a6c808b22318..1d3c71d7e68d 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -131,6 +131,9 @@ struct tasdevice_priv { const struct firmware *fmw, int offset); int (*tasdevice_load_block)(struct tasdevice_priv *tas_priv, struct tasdev_blk *block); + + int (*load_calibration)(struct tasdevice_priv *tas_priv); + void (*apply_calibration)(struct tasdevice_priv *tas_priv); };
void tas2781_reset(struct tasdevice_priv *tas_dev); @@ -139,6 +142,8 @@ int tascodec_init(struct tasdevice_priv *tas_priv, void *codec, struct tasdevice_priv *tasdevice_kzalloc(struct i2c_client *i2c); int tasdevice_init(struct tasdevice_priv *tas_priv); void tasdevice_remove(struct tasdevice_priv *tas_priv); +int tasdevice_load_calibration(struct tasdevice_priv *tas_priv); +void tasdevice_apply_calibration(struct tasdevice_priv *tas_priv); int tasdevice_dev_read(struct tasdevice_priv *tas_priv, unsigned short chn, unsigned int reg, unsigned int *value); int tasdevice_dev_write(struct tasdevice_priv *tas_priv, diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 70085177230e..2b5031e4dda3 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -395,7 +395,7 @@ static const struct snd_kcontrol_new tas2781_dsp_conf_ctrl = { .put = tasdevice_config_put, };
-static void tas2781_apply_calib(struct tasdevice_priv *tas_priv) +static void tas2781_apply_calibration(struct tasdevice_priv *tas_priv) { static const unsigned char page_array[CALIB_MAX] = { 0x17, 0x18, 0x18, 0x0d, 0x18 @@ -426,7 +426,7 @@ static void tas2781_apply_calib(struct tasdevice_priv *tas_priv) * by Algo for calcucating the speaker temperature, speaker membrane excursion * and f0 in real time during playback. */ -static int tas2781_save_calibration(struct tasdevice_priv *tas_priv) +static int load_calibration_efi_1(struct tasdevice_priv *tas_priv) { efi_guid_t efi_guid = EFI_GUID(0x02f9af02, 0x7734, 0x4233, 0xb4, 0x3d, 0x93, 0xfe, 0x5a, 0xa3, 0x5d, 0xb3); @@ -547,7 +547,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) /* If calibrated data occurs error, dsp will still works with default * calibrated data inside algo. */ - tas2781_save_calibration(tas_priv); + tasdevice_load_calibration(tas_priv);
out: if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) { @@ -650,15 +650,17 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) const char *device_name; int ret;
- if (strstr(dev_name(&clt->dev), "TIAS2781")) - device_name = "TIAS2781"; - else - return -ENODEV; - tas_priv = tasdevice_kzalloc(clt); if (!tas_priv) return -ENOMEM;
+ if (strstr(dev_name(&clt->dev), "TIAS2781")) { + device_name = "TIAS2781"; + tas_priv->load_calibration = load_calibration_efi_1; + tas_priv->apply_calibration = tas2781_apply_calibration; + } else + return -ENODEV; + tas_priv->irq_info.irq = clt->irq; ret = tas2781_read_acpi(tas_priv, device_name); if (ret) @@ -726,8 +728,6 @@ static int tas2781_runtime_suspend(struct device *dev) 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;
dev_dbg(tas_priv->dev, "Runtime Resume\n");
@@ -738,8 +738,7 @@ static int tas2781_runtime_resume(struct device *dev) /* If calibrated data occurs error, dsp will still works with default * calibrated data inside algo. */ - if (tas_priv->cali_data.total_sz > calib_data_sz) - tas2781_apply_calib(tas_priv); + tasdevice_apply_calibration(tas_priv);
mutex_unlock(&tas_priv->codec_lock);
@@ -770,8 +769,6 @@ static int tas2781_system_suspend(struct device *dev) static int tas2781_system_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 i, ret;
dev_dbg(tas_priv->dev, "System Resume\n"); @@ -793,8 +790,7 @@ static int tas2781_system_resume(struct device *dev) /* If calibrated data occurs error, dsp will still work with default * calibrated data inside algo. */ - if (tas_priv->cali_data.total_sz > calib_data_sz) - tas2781_apply_calib(tas_priv); + tasdevice_apply_calibration(tas_priv); mutex_unlock(&tas_priv->codec_lock);
return 0; diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index 933cd008e9f5..f914123c7284 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -414,6 +414,21 @@ void tasdevice_remove(struct tasdevice_priv *tas_priv) } EXPORT_SYMBOL_GPL(tasdevice_remove);
+int tasdevice_load_calibration(struct tasdevice_priv *tas_priv) +{ + if (tas_priv->load_calibration) + return tas_priv->load_calibration(tas_priv); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(tasdevice_load_calibration); + +void tasdevice_apply_calibration(struct tasdevice_priv *tas_priv) +{ + if (tas_priv->apply_calibration && tas_priv->cali_data.total_sz) + tas_priv->apply_calibration(tas_priv); +} +EXPORT_SYMBOL_GPL(tasdevice_apply_calibration); + static int tasdevice_clamp(int val, int max, unsigned int invert) { if (val > max)
On Thu, Dec 07, 2023 at 12:59:47AM +0100, Gergo Koteles wrote:
rename save_calibration to load_calibration
The subject line said we were adding pointers, the commit message says something different and neither explains why we're making these changes. This should probably be multiple patches, the pointer refactoring split from the renames and all explained somehow.
Move the apply_calibration outside to make it clearer.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 2b5031e4dda3..c82ed2413fcb 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -470,7 +470,6 @@ static int load_calibration_efi_1(struct tasdevice_priv *tas_priv) dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n", tm->tm_year, tm->tm_mon, tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec); - tas2781_apply_calib(tas_priv); } else tas_priv->cali_data.total_sz = 0;
@@ -548,6 +547,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) * calibrated data inside algo. */ tasdevice_load_calibration(tas_priv); + tasdevice_apply_calibration(tas_priv);
out: if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) {
Add ability to handle global_addr of different chip versions.
Signed-off-by: Gergo Koteles soyer@irl.hu --- include/sound/tas2781.h | 2 ++ sound/pci/hda/tas2781_hda_i2c.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index 1d3c71d7e68d..5f364e4d8995 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -121,6 +121,8 @@ struct tasdevice_priv { bool force_fwload_status; bool playback_started; bool isacpi; + unsigned int global_addr; + int (*fw_parse_variable_header)(struct tasdevice_priv *tas_priv, const struct firmware *fmw, int offset); int (*fw_parse_program_data)(struct tasdevice_priv *tas_priv, diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index c82ed2413fcb..7ebf5d7e4aac 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -72,7 +72,7 @@ static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)
if (i2c_acpi_get_i2c_resource(ares, &sb)) { if (tas_priv->ndev < TASDEVICE_MAX_CHANNELS && - sb->slave_address != TAS2781_GLOBAL_ADDR) { + sb->slave_address != tas_priv->global_addr) { tas_priv->tasdevice[tas_priv->ndev].dev_addr = (unsigned int)sb->slave_address; tas_priv->ndev++;
The INT8866 belongs to the Lenovo Yoga 7 Gen 7 AMD 14ARB7 laptop. It has two TAS2563 amplifier. Add the ACPI UID and calibration functions to handle them.
Signed-off-by: Gergo Koteles soyer@irl.hu --- include/sound/tas2781.h | 1 + sound/pci/hda/tas2781_hda_i2c.c | 87 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index 5f364e4d8995..afe9584daa65 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -22,6 +22,7 @@ #define TAS2781_DRV_VER 1 #define SMARTAMP_MODULE_NAME "tas2781" #define TAS2781_GLOBAL_ADDR 0x40 +#define TAS2563_GLOBAL_ADDR 0x48 #define TASDEVICE_RATES (SNDRV_PCM_RATE_44100 |\ SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |\ SNDRV_PCM_RATE_88200) diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 7ebf5d7e4aac..f28383597ca8 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -65,6 +65,24 @@ enum calib_data { CALIB_MAX };
+#define TAS2563_MAX_CHANNELS 4 + +#define TAS2563_CAL_POWER TASDEVICE_REG(0, 0x0d, 0x3c) +#define TAS2563_CAL_R0 TASDEVICE_REG(0, 0x0f, 0x34) +#define TAS2563_CAL_INVR0 TASDEVICE_REG(0, 0x0f, 0x40) +#define TAS2563_CAL_R0_LOW TASDEVICE_REG(0, 0x0f, 0x48) +#define TAS2563_CAL_TLIM TASDEVICE_REG(0, 0x10, 0x14) +#define TAS2563_CAL_N 5 +#define TAS2563_CAL_DATA_SIZE 4 +#define TAS2563_CAL_CH_SIZE 20 +#define TAS2563_CAL_ARRAY_SIZE 80 + +static unsigned int cal_regs[TAS2563_CAL_N] = { + TAS2563_CAL_POWER, TAS2563_CAL_R0, TAS2563_CAL_INVR0, + TAS2563_CAL_R0_LOW, TAS2563_CAL_TLIM, +}; + + static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data) { struct tasdevice_priv *tas_priv = data; @@ -395,6 +413,68 @@ static const struct snd_kcontrol_new tas2781_dsp_conf_ctrl = { .put = tasdevice_config_put, };
+static void tas2563_apply_calibration(struct tasdevice_priv *tas_priv) +{ + unsigned int data; + int offset = 0; + int ret; + + for (int i = 0; i < tas_priv->ndev; i++) { + for (int j = 0; j < TAS2563_CAL_N; ++j) { + data = cpu_to_be32( + *(uint32_t *)&tas_priv->cali_data.data[offset]); + ret = tasdevice_dev_bulk_write(tas_priv, i, cal_regs[j], + (unsigned char *)&data, TAS2563_CAL_DATA_SIZE); + if (ret) + dev_err(tas_priv->dev, + "Error writing calib regs\n"); + offset += TAS2563_CAL_DATA_SIZE; + } + } +} + +static int load_calibration_efi_2(struct tasdevice_priv *tas_priv) +{ + static efi_guid_t efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, + 0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92); + + static efi_char16_t *efi_vars[TAS2563_MAX_CHANNELS][TAS2563_CAL_N] = { + { L"Power_1", L"R0_1", L"InvR0_1", L"R0_Low_1", L"TLim_1" }, + { L"Power_2", L"R0_2", L"InvR0_2", L"R0_Low_2", L"TLim_2" }, + { L"Power_3", L"R0_3", L"InvR0_3", L"R0_Low_3", L"TLim_3" }, + { L"Power_4", L"R0_4", L"InvR0_4", L"R0_Low_4", L"TLim_4" }, + }; + + unsigned long max_size = TAS2563_CAL_DATA_SIZE; + unsigned int offset = 0; + efi_status_t status; + unsigned int attr; + + tas_priv->cali_data.data = devm_kzalloc(tas_priv->dev, + TAS2563_CAL_ARRAY_SIZE, GFP_KERNEL); + if (!tas_priv->cali_data.data) + return -ENOMEM; + + for (int i = 0; i < tas_priv->ndev; ++i) { + for (int j = 0; j < TAS2563_CAL_N; ++j) { + status = efi.get_variable(efi_vars[i][j], + &efi_guid, &attr, &max_size, + &tas_priv->cali_data.data[offset]); + if (status != EFI_SUCCESS || + max_size != TAS2563_CAL_DATA_SIZE) { + dev_warn(tas_priv->dev, + "Calibration data read failed %ld\n", status); + return -EINVAL; + } + offset += TAS2563_CAL_DATA_SIZE; + } + } + + tas_priv->cali_data.total_sz = offset; + + return 0; +} + static void tas2781_apply_calibration(struct tasdevice_priv *tas_priv) { static const unsigned char page_array[CALIB_MAX] = { @@ -658,6 +738,12 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) device_name = "TIAS2781"; tas_priv->load_calibration = load_calibration_efi_1; tas_priv->apply_calibration = tas2781_apply_calibration; + tas_priv->global_addr = TAS2781_GLOBAL_ADDR; + } else if (strstr(dev_name(&clt->dev), "INT8866")) { + device_name = "INT8866"; + tas_priv->load_calibration = load_calibration_efi_2; + tas_priv->apply_calibration = tas2563_apply_calibration; + tas_priv->global_addr = TAS2563_GLOBAL_ADDR; } else return -ENODEV;
@@ -808,6 +894,7 @@ static const struct i2c_device_id tas2781_hda_i2c_id[] = {
static const struct acpi_device_id tas2781_acpi_hda_match[] = { {"TIAS2781", 0 }, + {"INT8866", 0 }, {} }; MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
Negative indexes are not valid here.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/soc/codecs/tas2781-fmwlib.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/tas2781-fmwlib.c b/sound/soc/codecs/tas2781-fmwlib.c index 20dc2df034e9..aa5f56f9ad33 100644 --- a/sound/soc/codecs/tas2781-fmwlib.c +++ b/sound/soc/codecs/tas2781-fmwlib.c @@ -2198,21 +2198,21 @@ int tasdevice_select_tuningprm_cfg(void *context, int prm_no, goto out; }
- if (cfg_no >= tas_fmw->nr_configurations) { + if (cfg_no < 0 || cfg_no >= tas_fmw->nr_configurations) { dev_err(tas_priv->dev, "%s: cfg(%d) is not in range of conf %u\n", __func__, cfg_no, tas_fmw->nr_configurations); goto out; }
- if (prm_no >= tas_fmw->nr_programs) { + if (prm_no < 0 || prm_no >= tas_fmw->nr_programs) { dev_err(tas_priv->dev, "%s: prm(%d) is not in range of Programs %u\n", __func__, prm_no, tas_fmw->nr_programs); goto out; }
- if (rca_conf_no >= rca->ncfgs || rca_conf_no < 0 || + if (rca_conf_no < 0 || rca_conf_no >= rca->ncfgs || !cfg_info) { dev_err(tas_priv->dev, "conf_no:%d should be in range from 0 to %u\n", @@ -2304,7 +2304,7 @@ int tasdevice_prmg_load(void *context, int prm_no) goto out; }
- if (prm_no >= tas_fmw->nr_programs) { + if (prm_no < 0 || prm_no >= tas_fmw->nr_programs) { dev_err(tas_priv->dev, "%s: prm(%d) is not in range of Programs %u\n", __func__, prm_no, tas_fmw->nr_programs); @@ -2349,7 +2349,7 @@ int tasdevice_prmg_calibdata_load(void *context, int prm_no) goto out; }
- if (prm_no >= tas_fmw->nr_programs) { + if (prm_no < 0 || prm_no >= tas_fmw->nr_programs) { dev_err(tas_priv->dev, "%s: prm(%d) is not in range of Programs %u\n", __func__, prm_no, tas_fmw->nr_programs);
Invalid indexes are not the best default values.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/soc/codecs/tas2781-comlib.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index f914123c7284..635f97db033b 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -307,8 +307,8 @@ int tasdevice_init(struct tasdevice_priv *tas_priv) goto out; }
- tas_priv->cur_prog = -1; - tas_priv->cur_conf = -1; + tas_priv->cur_prog = 0; + tas_priv->cur_conf = 0;
for (i = 0; i < tas_priv->ndev; i++) { tas_priv->tasdevice[i].cur_book = -1;
On Thu, Dec 07, 2023 at 01:04:27AM +0100, Gergo Koteles wrote:
Invalid indexes are not the best default values.
I'm guessing this is just fallout from the previous (not really explained patch)? Is there perhaps some bootstrapping issue here with ensuring that the program and configuration get written to the device if the user doesn't explicitly select something in a control?
On Thu, 2023-12-07 at 18:28 +0000, Mark Brown wrote:
On Thu, Dec 07, 2023 at 01:04:27AM +0100, Gergo Koteles wrote:
Invalid indexes are not the best default values.
I'm guessing this is just fallout from the previous (not really explained patch)? Is there perhaps some bootstrapping issue here with ensuring that the program and configuration get written to the device if the user doesn't explicitly select something in a control?
I added the >0 checks because I encountered a NULL pointer dereference.
Because tasdevice_init sets tas_priv->cur_prog = -1 tas_priv->cur_conf = -1
tasdev_fw_ready calls tasdevice_prmg_load(tas_priv, 0) which sets tasdevice[i]->prg_no = 0
Then the playback hook calls tasdevice_tuning_switch(tas_hda->priv, 0) tasdevice_select_tuningprm_cfg(tas_priv, cur_prog (-1), cur_conf (-1), profile_cfg_id (0));
tasdevice_select_tuningprm_cfg checks if (tas_priv->tasdevice[i].cur_prog (0) != prm_no (-1) and tries to load the program from program = &(tas_fmw->programs[prm_no (-1)]); tasdevice_load_data(tas_priv, &(program->dev_data));
I think the intention was to load the first program, first configuration, first profile. And that's the safe thing to do. So I expressed that with this commit.
Yes, I need to write much better commit messages.
allow driver specific driver data in tas2781-hda-ic2c and tas2781-i2c
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 2 ++ sound/soc/codecs/tas2781-comlib.c | 2 -- sound/soc/codecs/tas2781-i2c.c | 2 ++ 3 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index f28383597ca8..290c41194139 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -734,6 +734,8 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) if (!tas_priv) return -ENOMEM;
+ dev_set_drvdata(&clt->dev, tas_priv); + if (strstr(dev_name(&clt->dev), "TIAS2781")) { device_name = "TIAS2781"; tas_priv->load_calibration = load_calibration_efi_1; diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index 635f97db033b..0c04735dd575 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -316,8 +316,6 @@ int tasdevice_init(struct tasdevice_priv *tas_priv) tas_priv->tasdevice[i].cur_conf = -1; }
- dev_set_drvdata(tas_priv->dev, tas_priv); - mutex_init(&tas_priv->codec_lock);
out: diff --git a/sound/soc/codecs/tas2781-i2c.c b/sound/soc/codecs/tas2781-i2c.c index 55cd5e3c23a5..917b1c15f71d 100644 --- a/sound/soc/codecs/tas2781-i2c.c +++ b/sound/soc/codecs/tas2781-i2c.c @@ -689,6 +689,8 @@ static int tasdevice_i2c_probe(struct i2c_client *i2c) if (!tas_priv) return -ENOMEM;
+ dev_set_drvdata(&i2c->dev, tas_priv); + if (ACPI_HANDLE(&i2c->dev)) { acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table, &i2c->dev);
Remove sound controls in hda_unbind to make module loadable after module unload.
Add a driver specific struct (tas2781_hda) to store the controls.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 234 +++++++++++++++++++------------- 1 file changed, 136 insertions(+), 98 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 290c41194139..0972345c15e5 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -82,6 +82,14 @@ static unsigned int cal_regs[TAS2563_CAL_N] = { TAS2563_CAL_R0_LOW, TAS2563_CAL_TLIM, };
+struct tas2781_hda { + struct device *dev; + struct tasdevice_priv *priv; + struct snd_kcontrol *dsp_prog_ctl; + struct snd_kcontrol *dsp_conf_ctl; + struct snd_kcontrol *prof_ctl; + struct snd_kcontrol *snd_ctls[3]; +};
static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data) { @@ -143,26 +151,26 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
static void tas2781_hda_playback_hook(struct device *dev, int action) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
- dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action); + dev_dbg(tas_hda->dev, "%s: action = %d\n", __func__, action); switch (action) { case HDA_GEN_PCM_ACT_OPEN: pm_runtime_get_sync(dev); - mutex_lock(&tas_priv->codec_lock); - tasdevice_tuning_switch(tas_priv, 0); - mutex_unlock(&tas_priv->codec_lock); + mutex_lock(&tas_hda->priv->codec_lock); + tasdevice_tuning_switch(tas_hda->priv, 0); + mutex_unlock(&tas_hda->priv->codec_lock); break; case HDA_GEN_PCM_ACT_CLOSE: - mutex_lock(&tas_priv->codec_lock); - tasdevice_tuning_switch(tas_priv, 1); - mutex_unlock(&tas_priv->codec_lock); + mutex_lock(&tas_hda->priv->codec_lock); + tasdevice_tuning_switch(tas_hda->priv, 1); + mutex_unlock(&tas_hda->priv->codec_lock);
pm_runtime_mark_last_busy(dev); pm_runtime_put_autosuspend(dev); break; default: - dev_dbg(tas_priv->dev, "Playback action not supported: %d\n", + dev_dbg(tas_hda->dev, "Playback action not supported: %d\n", action); break; } @@ -556,9 +564,28 @@ static int load_calibration_efi_1(struct tasdevice_priv *tas_priv) return 0; }
+static void tas2781_hda_remove_controls(struct tas2781_hda *tas_hda) +{ + struct hda_codec *codec = tas_hda->priv->codec; + + if (tas_hda->dsp_prog_ctl) + snd_ctl_remove(codec->card, tas_hda->dsp_prog_ctl); + + if (tas_hda->dsp_conf_ctl) + snd_ctl_remove(codec->card, tas_hda->dsp_conf_ctl); + + for (int i = ARRAY_SIZE(tas_hda->snd_ctls) - 1; i >= 0; i--) + if (tas_hda->snd_ctls[i]) + snd_ctl_remove(codec->card, tas_hda->snd_ctls[i]); + + if (tas_hda->prof_ctl) + snd_ctl_remove(codec->card, tas_hda->prof_ctl); +} + static void tasdev_fw_ready(const struct firmware *fmw, void *context) { struct tasdevice_priv *tas_priv = context; + struct tas2781_hda *tas_hda = dev_get_drvdata(tas_priv->dev); struct hda_codec *codec = tas_priv->codec; int i, ret;
@@ -569,8 +596,8 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) if (ret) goto out;
- ret = snd_ctl_add(codec->card, - snd_ctl_new1(&tas2781_prof_ctrl, tas_priv)); + tas_hda->prof_ctl = snd_ctl_new1(&tas2781_prof_ctrl, tas_priv); + ret = snd_ctl_add(codec->card, tas_hda->prof_ctl); if (ret) { dev_err(tas_priv->dev, "Failed to add KControl %s = %d\n", @@ -579,8 +606,9 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) }
for (i = 0; i < ARRAY_SIZE(tas2781_snd_controls); i++) { - ret = snd_ctl_add(codec->card, - snd_ctl_new1(&tas2781_snd_controls[i], tas_priv)); + tas_hda->snd_ctls[i] = snd_ctl_new1(&tas2781_snd_controls[i], + tas_priv); + ret = snd_ctl_add(codec->card, tas_hda->snd_ctls[i]); if (ret) { dev_err(tas_priv->dev, "Failed to add KControl %s = %d\n", @@ -602,8 +630,9 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) goto out; }
- ret = snd_ctl_add(codec->card, - snd_ctl_new1(&tas2781_dsp_prog_ctrl, tas_priv)); + tas_hda->dsp_prog_ctl = snd_ctl_new1(&tas2781_dsp_prog_ctrl, + tas_priv); + ret = snd_ctl_add(codec->card, tas_hda->dsp_prog_ctl); if (ret) { dev_err(tas_priv->dev, "Failed to add KControl %s = %d\n", @@ -611,8 +640,9 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) goto out; }
- ret = snd_ctl_add(codec->card, - snd_ctl_new1(&tas2781_dsp_conf_ctrl, tas_priv)); + tas_hda->dsp_conf_ctl = snd_ctl_new1(&tas2781_dsp_conf_ctrl, + tas_priv); + ret = snd_ctl_add(codec->card, tas_hda->dsp_conf_ctl); if (ret) { dev_err(tas_priv->dev, "Failed to add KControl %s = %d\n", @@ -635,27 +665,27 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) tasdevice_config_info_remove(tas_priv); tasdevice_dsp_remove(tas_priv); } - mutex_unlock(&tas_priv->codec_lock); + mutex_unlock(&tas_hda->priv->codec_lock); if (fmw) release_firmware(fmw); - pm_runtime_mark_last_busy(tas_priv->dev); - pm_runtime_put_autosuspend(tas_priv->dev); + pm_runtime_mark_last_busy(tas_hda->dev); + pm_runtime_put_autosuspend(tas_hda->dev); }
static int tas2781_hda_bind(struct device *dev, struct device *master, void *master_data) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev); struct hda_component *comps = master_data; struct hda_codec *codec; unsigned int subid; int ret;
- if (!comps || tas_priv->index < 0 || - tas_priv->index >= HDA_MAX_COMPONENTS) + if (!comps || tas_hda->priv->index < 0 || + tas_hda->priv->index >= HDA_MAX_COMPONENTS) return -EINVAL;
- comps = &comps[tas_priv->index]; + comps = &comps[tas_hda->priv->index]; if (comps->dev) return -EBUSY;
@@ -664,10 +694,10 @@ static int tas2781_hda_bind(struct device *dev, struct device *master,
switch (subid) { case 0x17aa: - tas_priv->catlog_id = LENOVO; + tas_hda->priv->catlog_id = LENOVO; break; default: - tas_priv->catlog_id = OTHERS; + tas_hda->priv->catlog_id = OTHERS; break; }
@@ -677,7 +707,7 @@ static int tas2781_hda_bind(struct device *dev, struct device *master,
strscpy(comps->name, dev_name(dev), sizeof(comps->name));
- ret = tascodec_init(tas_priv, codec, tasdev_fw_ready); + ret = tascodec_init(tas_hda->priv, codec, tasdev_fw_ready); if (!ret) comps->playback_hook = tas2781_hda_playback_hook;
@@ -690,19 +720,21 @@ static int tas2781_hda_bind(struct device *dev, struct device *master, static void tas2781_hda_unbind(struct device *dev, struct device *master, void *master_data) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev); struct hda_component *comps = master_data;
- if (comps[tas_priv->index].dev == dev) { - comps[tas_priv->index].dev = NULL; + if (comps[tas_hda->priv->index].dev == dev) { + comps[tas_hda->priv->index].dev = NULL; strscpy(comps->name, "", sizeof(comps->name)); - comps[tas_priv->index].playback_hook = NULL; + comps[tas_hda->priv->index].playback_hook = NULL; }
- tasdevice_config_info_remove(tas_priv); - tasdevice_dsp_remove(tas_priv); + tas2781_hda_remove_controls(tas_hda);
- tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING; + tasdevice_config_info_remove(tas_hda->priv); + tasdevice_dsp_remove(tas_hda->priv); + + tas_hda->priv->fw_state = TASDEVICE_DSP_FW_PENDING; }
static const struct component_ops tas2781_hda_comp_ops = { @@ -712,70 +744,75 @@ static const struct component_ops tas2781_hda_comp_ops = {
static void tas2781_hda_remove(struct device *dev) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
- pm_runtime_get_sync(tas_priv->dev); - pm_runtime_disable(tas_priv->dev); + pm_runtime_get_sync(tas_hda->dev); + pm_runtime_disable(tas_hda->dev);
- component_del(tas_priv->dev, &tas2781_hda_comp_ops); + component_del(tas_hda->dev, &tas2781_hda_comp_ops);
- pm_runtime_put_noidle(tas_priv->dev); + pm_runtime_put_noidle(tas_hda->dev);
- tasdevice_remove(tas_priv); + tasdevice_remove(tas_hda->priv); }
static int tas2781_hda_i2c_probe(struct i2c_client *clt) { - struct tasdevice_priv *tas_priv; + struct tas2781_hda *tas_hda; const char *device_name; int ret;
- tas_priv = tasdevice_kzalloc(clt); - if (!tas_priv) + tas_hda = devm_kzalloc(&clt->dev, sizeof(*tas_hda), GFP_KERNEL); + if (!tas_hda) return -ENOMEM;
- dev_set_drvdata(&clt->dev, tas_priv); + dev_set_drvdata(&clt->dev, tas_hda); + tas_hda->dev = &clt->dev; + + tas_hda->priv = tasdevice_kzalloc(clt); + if (!tas_hda->priv) + return -ENOMEM;
- if (strstr(dev_name(&clt->dev), "TIAS2781")) { + if (strstr(dev_name(tas_hda->dev), "TIAS2781")) { device_name = "TIAS2781"; - tas_priv->load_calibration = load_calibration_efi_1; - tas_priv->apply_calibration = tas2781_apply_calibration; - tas_priv->global_addr = TAS2781_GLOBAL_ADDR; - } else if (strstr(dev_name(&clt->dev), "INT8866")) { + tas_hda->priv->load_calibration = load_calibration_efi_1; + tas_hda->priv->apply_calibration = tas2781_apply_calibration; + tas_hda->priv->global_addr = TAS2781_GLOBAL_ADDR; + } else if (strstr(dev_name(tas_hda->dev), "INT8866")) { device_name = "INT8866"; - tas_priv->load_calibration = load_calibration_efi_2; - tas_priv->apply_calibration = tas2563_apply_calibration; - tas_priv->global_addr = TAS2563_GLOBAL_ADDR; + tas_hda->priv->load_calibration = load_calibration_efi_2; + tas_hda->priv->apply_calibration = tas2563_apply_calibration; + tas_hda->priv->global_addr = TAS2563_GLOBAL_ADDR; } else return -ENODEV;
- tas_priv->irq_info.irq = clt->irq; - ret = tas2781_read_acpi(tas_priv, device_name); + tas_hda->priv->irq_info.irq = clt->irq; + ret = tas2781_read_acpi(tas_hda->priv, device_name); if (ret) - return dev_err_probe(tas_priv->dev, ret, + return dev_err_probe(tas_hda->dev, ret, "Platform not supported\n");
- ret = tasdevice_init(tas_priv); + ret = tasdevice_init(tas_hda->priv); if (ret) goto err;
- pm_runtime_set_autosuspend_delay(tas_priv->dev, 3000); - pm_runtime_use_autosuspend(tas_priv->dev); - pm_runtime_mark_last_busy(tas_priv->dev); - pm_runtime_set_active(tas_priv->dev); - pm_runtime_get_noresume(tas_priv->dev); - pm_runtime_enable(tas_priv->dev); + pm_runtime_set_autosuspend_delay(tas_hda->dev, 3000); + pm_runtime_use_autosuspend(tas_hda->dev); + pm_runtime_mark_last_busy(tas_hda->dev); + pm_runtime_set_active(tas_hda->dev); + pm_runtime_get_noresume(tas_hda->dev); + pm_runtime_enable(tas_hda->dev);
- pm_runtime_put_autosuspend(tas_priv->dev); + pm_runtime_put_autosuspend(tas_hda->dev);
- ret = component_add(tas_priv->dev, &tas2781_hda_comp_ops); + ret = component_add(tas_hda->dev, &tas2781_hda_comp_ops); if (ret) { - dev_err(tas_priv->dev, "Register component failed: %d\n", ret); - pm_runtime_disable(tas_priv->dev); + dev_err(tas_hda->dev, "Register component failed: %d\n", ret); + pm_runtime_disable(tas_hda->dev); goto err; }
- tas2781_reset(tas_priv); + tas2781_reset(tas_hda->priv); err: if (ret) tas2781_hda_remove(&clt->dev); @@ -789,63 +826,64 @@ static void tas2781_hda_i2c_remove(struct i2c_client *clt)
static int tas2781_runtime_suspend(struct device *dev) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev); int i;
- dev_dbg(tas_priv->dev, "Runtime Suspend\n"); + dev_info(tas_hda->dev, "Runtime Suspend\n");
- mutex_lock(&tas_priv->codec_lock); + mutex_lock(&tas_hda->priv->codec_lock);
- if (tas_priv->playback_started) { - tasdevice_tuning_switch(tas_priv, 1); - tas_priv->playback_started = false; + if (tas_hda->priv->playback_started) { + tasdevice_tuning_switch(tas_hda->priv, 1); + tas_hda->priv->playback_started = false; }
- for (i = 0; i < tas_priv->ndev; i++) { - tas_priv->tasdevice[i].cur_book = -1; - tas_priv->tasdevice[i].cur_prog = -1; - tas_priv->tasdevice[i].cur_conf = -1; + for (i = 0; i < tas_hda->priv->ndev; i++) { + tas_hda->priv->tasdevice[i].cur_book = -1; + tas_hda->priv->tasdevice[i].cur_prog = -1; + tas_hda->priv->tasdevice[i].cur_conf = -1; }
- mutex_unlock(&tas_priv->codec_lock); + mutex_unlock(&tas_hda->priv->codec_lock);
return 0; }
static int tas2781_runtime_resume(struct device *dev) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev);
- dev_dbg(tas_priv->dev, "Runtime Resume\n"); + dev_dbg(tas_hda->dev, "Runtime Resume\n");
- mutex_lock(&tas_priv->codec_lock); + mutex_lock(&tas_hda->priv->codec_lock);
- tasdevice_prmg_load(tas_priv, tas_priv->cur_prog); + tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog);
/* If calibrated data occurs error, dsp will still works with default * calibrated data inside algo. */ - tasdevice_apply_calibration(tas_priv); + tasdevice_apply_calibration(tas_hda->priv);
- mutex_unlock(&tas_priv->codec_lock); +out: + mutex_unlock(&tas_hda->priv->codec_lock);
return 0; }
static int tas2781_system_suspend(struct device *dev) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev); int ret;
- dev_dbg(tas_priv->dev, "System Suspend\n"); + dev_dbg(tas_hda->priv->dev, "System Suspend\n");
ret = pm_runtime_force_suspend(dev); if (ret) return ret;
/* Shutdown chip before system suspend */ - tasdevice_tuning_switch(tas_priv, 1); + tasdevice_tuning_switch(tas_hda->priv, 1);
/* * Reset GPIO may be shared, so cannot reset here. @@ -856,30 +894,30 @@ static int tas2781_system_suspend(struct device *dev)
static int tas2781_system_resume(struct device *dev) { - struct tasdevice_priv *tas_priv = dev_get_drvdata(dev); + struct tas2781_hda *tas_hda = dev_get_drvdata(dev); int i, ret;
- dev_dbg(tas_priv->dev, "System Resume\n"); + dev_info(tas_hda->priv->dev, "System Resume\n");
ret = pm_runtime_force_resume(dev); if (ret) return ret;
- mutex_lock(&tas_priv->codec_lock); + mutex_lock(&tas_hda->priv->codec_lock);
- for (i = 0; i < tas_priv->ndev; i++) { - tas_priv->tasdevice[i].cur_book = -1; - tas_priv->tasdevice[i].cur_prog = -1; - tas_priv->tasdevice[i].cur_conf = -1; + for (i = 0; i < tas_hda->priv->ndev; i++) { + tas_hda->priv->tasdevice[i].cur_book = -1; + tas_hda->priv->tasdevice[i].cur_prog = -1; + tas_hda->priv->tasdevice[i].cur_conf = -1; } - tas2781_reset(tas_priv); - tasdevice_prmg_load(tas_priv, tas_priv->cur_prog); + tas2781_reset(tas_hda->priv); + tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog);
/* If calibrated data occurs error, dsp will still work with default * calibrated data inside algo. */ - tasdevice_apply_calibration(tas_priv); - mutex_unlock(&tas_priv->codec_lock); + tasdevice_apply_calibration(tas_hda->priv); + mutex_unlock(&tas_hda->priv->codec_lock);
return 0; }
Hi Gergo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tiwai-sound/for-next] [also build test WARNING on tiwai-sound/for-linus broonie-sound/for-next linus/master v6.7-rc4 next-20231207] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gergo-Koteles/ALSA-hda-tas278... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next patch link: https://lore.kernel.org/r/8f16576930682297fd08bba5e063a9a1f3150388.170190645... patch subject: [PATCH 13/16] ALSA: hda/tas2781: remove sound controls in unbind config: x86_64-randconfig-013-20231207 (https://download.01.org/0day-ci/archive/20231207/202312072037.CYK5reOb-lkp@i...) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231207/202312072037.CYK5reOb-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312072037.CYK5reOb-lkp@intel.com/
All warnings (new ones prefixed by >>):
sound/pci/hda/tas2781_hda_i2c.c: In function 'tas2781_runtime_resume':
sound/pci/hda/tas2781_hda_i2c.c:868:1: warning: label 'out' defined but not used [-Wunused-label]
868 | out: | ^~~
vim +/out +868 sound/pci/hda/tas2781_hda_i2c.c
852 853 static int tas2781_runtime_resume(struct device *dev) 854 { 855 struct tas2781_hda *tas_hda = dev_get_drvdata(dev); 856 857 dev_dbg(tas_hda->dev, "Runtime Resume\n"); 858 859 mutex_lock(&tas_hda->priv->codec_lock); 860 861 tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog); 862 863 /* If calibrated data occurs error, dsp will still works with default 864 * calibrated data inside algo. 865 */ 866 tasdevice_apply_calibration(tas_hda->priv); 867
868 out:
869 mutex_unlock(&tas_hda->priv->codec_lock); 870 871 return 0; 872 } 873
Hi Gergo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tiwai-sound/for-next] [also build test WARNING on tiwai-sound/for-linus broonie-sound/for-next linus/master v6.7-rc4 next-20231207] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gergo-Koteles/ALSA-hda-tas278... base: https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next patch link: https://lore.kernel.org/r/8f16576930682297fd08bba5e063a9a1f3150388.170190645... patch subject: [PATCH 13/16] ALSA: hda/tas2781: remove sound controls in unbind config: x86_64-randconfig-001-20231207 (https://download.01.org/0day-ci/archive/20231207/202312072318.9KUgvYeb-lkp@i...) compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231207/202312072318.9KUgvYeb-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202312072318.9KUgvYeb-lkp@intel.com/
All warnings (new ones prefixed by >>):
sound/pci/hda/tas2781_hda_i2c.c:868:1: warning: unused label 'out' [-Wunused-label]
out: ^~~~ 1 warning generated.
vim +/out +868 sound/pci/hda/tas2781_hda_i2c.c
852 853 static int tas2781_runtime_resume(struct device *dev) 854 { 855 struct tas2781_hda *tas_hda = dev_get_drvdata(dev); 856 857 dev_dbg(tas_hda->dev, "Runtime Resume\n"); 858 859 mutex_lock(&tas_hda->priv->codec_lock); 860 861 tasdevice_prmg_load(tas_hda->priv, tas_hda->priv->cur_prog); 862 863 /* If calibrated data occurs error, dsp will still works with default 864 * calibrated data inside algo. 865 */ 866 tasdevice_apply_calibration(tas_hda->priv); 867
868 out:
869 mutex_unlock(&tas_hda->priv->codec_lock); 870 871 return 0; 872 } 873
Prevent double free if only the DSP firmware is missing
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 5 ----- 1 file changed, 5 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 0972345c15e5..cb8872e15bb0 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -660,11 +660,6 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) tasdevice_apply_calibration(tas_priv);
out: - if (tas_priv->fw_state == TASDEVICE_DSP_FW_FAIL) { - /*If DSP FW fail, kcontrol won't be created */ - tasdevice_config_info_remove(tas_priv); - tasdevice_dsp_remove(tas_priv); - } mutex_unlock(&tas_hda->priv->codec_lock); if (fmw) release_firmware(fmw);
Prevent race to make the init reliable.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index cb8872e15bb0..f72e0beab1fb 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -800,14 +800,14 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
pm_runtime_put_autosuspend(tas_hda->dev);
+ tas2781_reset(tas_hda->priv); + ret = component_add(tas_hda->dev, &tas2781_hda_comp_ops); if (ret) { dev_err(tas_hda->dev, "Register component failed: %d\n", ret); pm_runtime_disable(tas_hda->dev); goto err; } - - tas2781_reset(tas_hda->priv); err: if (ret) tas2781_hda_remove(&clt->dev);
Make the amp available immediately after a module load to avoid having to wait for a PCM hook action. (eg. unloading & loading the module while listening music)
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/tas2781_hda_i2c.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index f72e0beab1fb..07bdff87c962 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -659,6 +659,8 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) tasdevice_load_calibration(tas_priv); tasdevice_apply_calibration(tas_priv);
+ tasdevice_tuning_switch(tas_hda->priv, 0); + out: mutex_unlock(&tas_hda->priv->codec_lock); if (fmw)
On Thu, Dec 07, 2023 at 01:58:22AM +0100, Gergo Koteles wrote:
Gergo Koteles (16): ASoC: tas2781: add support for fw version 0x0503 ALSA: hda/tas2781: leave hda_component in usable state ASoC: tas2781: disable regmap regcache ALSA: hda/tas2781: handle missing calibration data ALSA: hda/tas2781: fix typos in comment ASoC: tas2781: add ptrs to calibration functions ALSA: hda/tas2781: load_calibration just load ASoC: tas2781: add configurable global_addr ALSA: hda/tas2781: add TAS2563 support for 14ARB7 ASoC: tas2781: check negative indexes ASoC: tas2781: use 0 as default prog/conf index ASoC: tas2781: move set_drv_data outside tasdevice_init ALSA: hda/tas2781: remove sound controls in unbind ALSA: hda/tas2781: call cleaner functions only once ALSA: hda/tas2781: reset the amp before component_add ALSA: hda/tas2781: configure the amp after firmware load
Please don't randomly interleave ASoC and ALSA patches like this without some strong need, it just makes everything harder to manage.
On Thu, 07 Dec 2023 14:55:25 +0100, Mark Brown wrote:
On Thu, Dec 07, 2023 at 01:58:22AM +0100, Gergo Koteles wrote:
Gergo Koteles (16): ASoC: tas2781: add support for fw version 0x0503 ALSA: hda/tas2781: leave hda_component in usable state ASoC: tas2781: disable regmap regcache ALSA: hda/tas2781: handle missing calibration data ALSA: hda/tas2781: fix typos in comment ASoC: tas2781: add ptrs to calibration functions ALSA: hda/tas2781: load_calibration just load ASoC: tas2781: add configurable global_addr ALSA: hda/tas2781: add TAS2563 support for 14ARB7 ASoC: tas2781: check negative indexes ASoC: tas2781: use 0 as default prog/conf index ASoC: tas2781: move set_drv_data outside tasdevice_init ALSA: hda/tas2781: remove sound controls in unbind ALSA: hda/tas2781: call cleaner functions only once ALSA: hda/tas2781: reset the amp before component_add ALSA: hda/tas2781: configure the amp after firmware load
Please don't randomly interleave ASoC and ALSA patches like this without some strong need, it just makes everything harder to manage.
And, some look really like rather individual fixes; they deserve for Fixes tag and Cc-to-stable, at least.
thanks,
Takashi
The tas2781-hda driver can be modified to support tas2563 as well. Before knowing this information, I created another series for a new driver. Link: https://lore.kernel.org/lkml/cover.1701733441.git.soyer@irl.hu/
This series now extends tas2781-hda.
The tas2563 is a smart amplifier. Similar to tas2562 but with DSP. Some Lenovo laptops have it to drive the bass speakers. By default, it is in software shutdown state.
To make the DSP work it needs a firmware and some calibration data. The latter can be read from the EFI in Lenovo laptops.
For the correct configuration it needs additional register data. It captured after running the Windows driver.
The firmware can be extracted as TAS2563Firmware.bin from the Windows driver with innoextract. https://download.lenovo.com/consumer/mobiles/h5yd037fbfyy7kd0.exe
The driver will search for it as TAS2XXX3870.bin with the Lenovo Yoga 7 14ARB7.
The captured registers extracted with TI's regtool: https://github.com/soyersoyer/tas2563rca/raw/main/INT8866RCA2.bin
Changes since v1: - fixes were sent as individual patches - rebased onto for-next - adding the missed fixup
Gergo Koteles (4): ALSA: hda/tas2781: add ptrs to calibration functions ALSA: hda/tas2781: add configurable global i2c address ALSA: hda/tas2781: add TAS2563 support for 14ARB7 ALSA: hda/tas2781: add fixup for Lenovo 14ARB7
include/sound/tas2781.h | 8 +++ sound/pci/hda/patch_realtek.c | 14 ++++ sound/pci/hda/tas2781_hda_i2c.c | 115 ++++++++++++++++++++++++++---- sound/soc/codecs/tas2781-comlib.c | 15 ++++ 4 files changed, 137 insertions(+), 15 deletions(-)
base-commit: 64bf8dec54cfe57f416884a6b3d54c7f4259e93f
On Sat, 30 Dec 2023 01:09:41 +0100, Gergo Koteles wrote:
The tas2781-hda driver can be modified to support tas2563 as well. Before knowing this information, I created another series for a new driver. Link: https://lore.kernel.org/lkml/cover.1701733441.git.soyer@irl.hu/
This series now extends tas2781-hda.
The tas2563 is a smart amplifier. Similar to tas2562 but with DSP. Some Lenovo laptops have it to drive the bass speakers. By default, it is in software shutdown state.
To make the DSP work it needs a firmware and some calibration data. The latter can be read from the EFI in Lenovo laptops.
For the correct configuration it needs additional register data. It captured after running the Windows driver.
The firmware can be extracted as TAS2563Firmware.bin from the Windows driver with innoextract. https://download.lenovo.com/consumer/mobiles/h5yd037fbfyy7kd0.exe
The driver will search for it as TAS2XXX3870.bin with the Lenovo Yoga 7 14ARB7.
The captured registers extracted with TI's regtool: https://github.com/soyersoyer/tas2563rca/raw/main/INT8866RCA2.bin
Changes since v1:
- fixes were sent as individual patches
- rebased onto for-next
- adding the missed fixup
Gergo Koteles (4): ALSA: hda/tas2781: add ptrs to calibration functions ALSA: hda/tas2781: add configurable global i2c address ALSA: hda/tas2781: add TAS2563 support for 14ARB7 ALSA: hda/tas2781: add fixup for Lenovo 14ARB7
Applies all patches to for-next branch now.
thanks,
Takashi
Make calibration functions configurable to support different calibration data storage modes.
Signed-off-by: Gergo Koteles soyer@irl.hu --- include/sound/tas2781.h | 5 +++++ sound/pci/hda/tas2781_hda_i2c.c | 25 +++++++++++-------------- sound/soc/codecs/tas2781-comlib.c | 15 +++++++++++++++ 3 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index a6c808b22318..e17ceab4fead 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -131,6 +131,9 @@ struct tasdevice_priv { const struct firmware *fmw, int offset); int (*tasdevice_load_block)(struct tasdevice_priv *tas_priv, struct tasdev_blk *block); + + int (*save_calibration)(struct tasdevice_priv *tas_priv); + void (*apply_calibration)(struct tasdevice_priv *tas_priv); };
void tas2781_reset(struct tasdevice_priv *tas_dev); @@ -139,6 +142,8 @@ int tascodec_init(struct tasdevice_priv *tas_priv, void *codec, struct tasdevice_priv *tasdevice_kzalloc(struct i2c_client *i2c); int tasdevice_init(struct tasdevice_priv *tas_priv); void tasdevice_remove(struct tasdevice_priv *tas_priv); +int tasdevice_save_calibration(struct tasdevice_priv *tas_priv); +void tasdevice_apply_calibration(struct tasdevice_priv *tas_priv); int tasdevice_dev_read(struct tasdevice_priv *tas_priv, unsigned short chn, unsigned int reg, unsigned int *value); int tasdevice_dev_write(struct tasdevice_priv *tas_priv, diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index dfe281b57aa6..0f8d5f947f54 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -479,7 +479,7 @@ static int tas2781_save_calibration(struct tasdevice_priv *tas_priv) dev_dbg(tas_priv->dev, "%4ld-%2d-%2d, %2d:%2d:%2d\n", tm->tm_year, tm->tm_mon, tm->tm_mday, tm->tm_hour, tm->tm_min, tm->tm_sec); - tas2781_apply_calib(tas_priv); + tasdevice_apply_calibration(tas_priv); } else tas_priv->cali_data.total_sz = 0;
@@ -582,7 +582,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context) /* If calibrated data occurs error, dsp will still works with default * calibrated data inside algo. */ - tas2781_save_calibration(tas_priv); + tasdevice_save_calibration(tas_priv);
out: mutex_unlock(&tas_hda->priv->codec_lock); @@ -683,10 +683,6 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) const char *device_name; int ret;
- if (strstr(dev_name(&clt->dev), "TIAS2781")) - device_name = "TIAS2781"; - else - return -ENODEV;
tas_hda = devm_kzalloc(&clt->dev, sizeof(*tas_hda), GFP_KERNEL); if (!tas_hda) @@ -699,6 +695,13 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) if (!tas_hda->priv) return -ENOMEM;
+ if (strstr(dev_name(&clt->dev), "TIAS2781")) { + device_name = "TIAS2781"; + tas_hda->priv->save_calibration = tas2781_save_calibration; + tas_hda->priv->apply_calibration = tas2781_apply_calib; + } else + return -ENODEV; + tas_hda->priv->irq_info.irq = clt->irq; ret = tas2781_read_acpi(tas_hda->priv, device_name); if (ret) @@ -765,8 +768,6 @@ static int tas2781_runtime_suspend(struct device *dev) static int tas2781_runtime_resume(struct device *dev) { struct tas2781_hda *tas_hda = dev_get_drvdata(dev); - unsigned long calib_data_sz = - tas_hda->priv->ndev * TASDEVICE_SPEAKER_CALIBRATION_SIZE;
dev_dbg(tas_hda->dev, "Runtime Resume\n");
@@ -777,8 +778,7 @@ static int tas2781_runtime_resume(struct device *dev) /* If calibrated data occurs error, dsp will still works with default * calibrated data inside algo. */ - if (tas_hda->priv->cali_data.total_sz > calib_data_sz) - tas2781_apply_calib(tas_hda->priv); + tasdevice_apply_calibration(tas_hda->priv);
mutex_unlock(&tas_hda->priv->codec_lock);
@@ -809,8 +809,6 @@ static int tas2781_system_suspend(struct device *dev) static int tas2781_system_resume(struct device *dev) { struct tas2781_hda *tas_hda = dev_get_drvdata(dev); - unsigned long calib_data_sz = - tas_hda->priv->ndev * TASDEVICE_SPEAKER_CALIBRATION_SIZE; int i, ret;
dev_info(tas_hda->priv->dev, "System Resume\n"); @@ -832,8 +830,7 @@ static int tas2781_system_resume(struct device *dev) /* If calibrated data occurs error, dsp will still work with default * calibrated data inside algo. */ - if (tas_hda->priv->cali_data.total_sz > calib_data_sz) - tas2781_apply_calib(tas_hda->priv); + tasdevice_apply_calibration(tas_hda->priv); mutex_unlock(&tas_hda->priv->codec_lock);
return 0; diff --git a/sound/soc/codecs/tas2781-comlib.c b/sound/soc/codecs/tas2781-comlib.c index 00e35169ae49..b7e56ceb1acf 100644 --- a/sound/soc/codecs/tas2781-comlib.c +++ b/sound/soc/codecs/tas2781-comlib.c @@ -412,6 +412,21 @@ void tasdevice_remove(struct tasdevice_priv *tas_priv) } EXPORT_SYMBOL_GPL(tasdevice_remove);
+int tasdevice_save_calibration(struct tasdevice_priv *tas_priv) +{ + if (tas_priv->save_calibration) + return tas_priv->save_calibration(tas_priv); + return -EINVAL; +} +EXPORT_SYMBOL_GPL(tasdevice_save_calibration); + +void tasdevice_apply_calibration(struct tasdevice_priv *tas_priv) +{ + if (tas_priv->apply_calibration && tas_priv->cali_data.total_sz) + tas_priv->apply_calibration(tas_priv); +} +EXPORT_SYMBOL_GPL(tasdevice_apply_calibration); + static int tasdevice_clamp(int val, int max, unsigned int invert) { if (val > max)
Make the global i2c address configurable to support compatible amplifiers with different global i2c address.
Signed-off-by: Gergo Koteles soyer@irl.hu --- include/sound/tas2781.h | 2 ++ sound/pci/hda/tas2781_hda_i2c.c | 3 ++- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index e17ceab4fead..dde9f8120d4c 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -121,6 +121,8 @@ struct tasdevice_priv { bool force_fwload_status; bool playback_started; bool isacpi; + unsigned int global_addr; + int (*fw_parse_variable_header)(struct tasdevice_priv *tas_priv, const struct firmware *fmw, int offset); int (*fw_parse_program_data)(struct tasdevice_priv *tas_priv, diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 0f8d5f947f54..49477d17b07c 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -81,7 +81,7 @@ static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)
if (i2c_acpi_get_i2c_resource(ares, &sb)) { if (tas_priv->ndev < TASDEVICE_MAX_CHANNELS && - sb->slave_address != TAS2781_GLOBAL_ADDR) { + sb->slave_address != tas_priv->global_addr) { tas_priv->tasdevice[tas_priv->ndev].dev_addr = (unsigned int)sb->slave_address; tas_priv->ndev++; @@ -699,6 +699,7 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) device_name = "TIAS2781"; tas_hda->priv->save_calibration = tas2781_save_calibration; tas_hda->priv->apply_calibration = tas2781_apply_calib; + tas_hda->priv->global_addr = TAS2781_GLOBAL_ADDR; } else return -ENODEV;
The INT8866 belongs to the Lenovo Yoga 7 Gen 7 AMD 14ARB7 laptop. It has two TAS2563 amplifier. Add the PNP ID and calibration functions to handle them.
ACPI excerpt:
Scope (_SB.I2CD) { Device (TAS) { Name (_HID, "INT8866") // _HID: Hardware ID Name (_UID, Zero) // _UID: Unique ID Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings { Name (RBUF, ResourceTemplate () { I2cSerialBusV2 (0x004C, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\_SB.I2CD", 0x00, ResourceConsumer, , Exclusive, ) I2cSerialBusV2 (0x004D, ControllerInitiated, 0x00061A80, AddressingMode7Bit, "\_SB.I2CD", 0x00, ResourceConsumer, , Exclusive, ) GpioInt (Edge, ActiveLow, SharedAndWake, PullNone, 0x0000, "\_SB.GPIO", 0x00, ResourceConsumer, , ) { // Pin list 0x0020 } }) Return (RBUF) /* _SB_.I2CD.TAS_._CRS.RBUF */ }
Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) } } }
Signed-off-by: Gergo Koteles soyer@irl.hu --- include/sound/tas2781.h | 1 + sound/pci/hda/tas2781_hda_i2c.c | 87 +++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+)
diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h index dde9f8120d4c..0a86ab8d47b9 100644 --- a/include/sound/tas2781.h +++ b/include/sound/tas2781.h @@ -22,6 +22,7 @@ #define TAS2781_DRV_VER 1 #define SMARTAMP_MODULE_NAME "tas2781" #define TAS2781_GLOBAL_ADDR 0x40 +#define TAS2563_GLOBAL_ADDR 0x48 #define TASDEVICE_RATES (SNDRV_PCM_RATE_44100 |\ SNDRV_PCM_RATE_48000 | SNDRV_PCM_RATE_96000 |\ SNDRV_PCM_RATE_88200) diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c index 49477d17b07c..057688ca29bf 100644 --- a/sound/pci/hda/tas2781_hda_i2c.c +++ b/sound/pci/hda/tas2781_hda_i2c.c @@ -65,6 +65,24 @@ enum calib_data { CALIB_MAX };
+#define TAS2563_MAX_CHANNELS 4 + +#define TAS2563_CAL_POWER TASDEVICE_REG(0, 0x0d, 0x3c) +#define TAS2563_CAL_R0 TASDEVICE_REG(0, 0x0f, 0x34) +#define TAS2563_CAL_INVR0 TASDEVICE_REG(0, 0x0f, 0x40) +#define TAS2563_CAL_R0_LOW TASDEVICE_REG(0, 0x0f, 0x48) +#define TAS2563_CAL_TLIM TASDEVICE_REG(0, 0x10, 0x14) +#define TAS2563_CAL_N 5 +#define TAS2563_CAL_DATA_SIZE 4 +#define TAS2563_CAL_CH_SIZE 20 +#define TAS2563_CAL_ARRAY_SIZE 80 + +static unsigned int cal_regs[TAS2563_CAL_N] = { + TAS2563_CAL_POWER, TAS2563_CAL_R0, TAS2563_CAL_INVR0, + TAS2563_CAL_R0_LOW, TAS2563_CAL_TLIM, +}; + + struct tas2781_hda { struct device *dev; struct tasdevice_priv *priv; @@ -404,6 +422,69 @@ static const struct snd_kcontrol_new tas2781_dsp_conf_ctrl = { .put = tasdevice_config_put, };
+static void tas2563_apply_calib(struct tasdevice_priv *tas_priv) +{ + unsigned int data; + int offset = 0; + int ret; + + for (int i = 0; i < tas_priv->ndev; i++) { + for (int j = 0; j < TAS2563_CAL_N; ++j) { + data = cpu_to_be32( + *(uint32_t *)&tas_priv->cali_data.data[offset]); + ret = tasdevice_dev_bulk_write(tas_priv, i, cal_regs[j], + (unsigned char *)&data, TAS2563_CAL_DATA_SIZE); + if (ret) + dev_err(tas_priv->dev, + "Error writing calib regs\n"); + offset += TAS2563_CAL_DATA_SIZE; + } + } +} + +static int tas2563_save_calibration(struct tasdevice_priv *tas_priv) +{ + static efi_guid_t efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, + 0x09, 0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92); + + static efi_char16_t *efi_vars[TAS2563_MAX_CHANNELS][TAS2563_CAL_N] = { + { L"Power_1", L"R0_1", L"InvR0_1", L"R0_Low_1", L"TLim_1" }, + { L"Power_2", L"R0_2", L"InvR0_2", L"R0_Low_2", L"TLim_2" }, + { L"Power_3", L"R0_3", L"InvR0_3", L"R0_Low_3", L"TLim_3" }, + { L"Power_4", L"R0_4", L"InvR0_4", L"R0_Low_4", L"TLim_4" }, + }; + + unsigned long max_size = TAS2563_CAL_DATA_SIZE; + unsigned int offset = 0; + efi_status_t status; + unsigned int attr; + + tas_priv->cali_data.data = devm_kzalloc(tas_priv->dev, + TAS2563_CAL_ARRAY_SIZE, GFP_KERNEL); + if (!tas_priv->cali_data.data) + return -ENOMEM; + + for (int i = 0; i < tas_priv->ndev; ++i) { + for (int j = 0; j < TAS2563_CAL_N; ++j) { + status = efi.get_variable(efi_vars[i][j], + &efi_guid, &attr, &max_size, + &tas_priv->cali_data.data[offset]); + if (status != EFI_SUCCESS || + max_size != TAS2563_CAL_DATA_SIZE) { + dev_warn(tas_priv->dev, + "Calibration data read failed %ld\n", status); + return -EINVAL; + } + offset += TAS2563_CAL_DATA_SIZE; + } + } + + tas_priv->cali_data.total_sz = offset; + tasdevice_apply_calibration(tas_priv); + + return 0; +} + static void tas2781_apply_calib(struct tasdevice_priv *tas_priv) { static const unsigned char page_array[CALIB_MAX] = { @@ -700,6 +781,11 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt) tas_hda->priv->save_calibration = tas2781_save_calibration; tas_hda->priv->apply_calibration = tas2781_apply_calib; tas_hda->priv->global_addr = TAS2781_GLOBAL_ADDR; + } else if (strstr(dev_name(&clt->dev), "INT8866")) { + device_name = "INT8866"; + tas_hda->priv->save_calibration = tas2563_save_calibration; + tas_hda->priv->apply_calibration = tas2563_apply_calib; + tas_hda->priv->global_addr = TAS2563_GLOBAL_ADDR; } else return -ENODEV;
@@ -849,6 +935,7 @@ static const struct i2c_device_id tas2781_hda_i2c_id[] = {
static const struct acpi_device_id tas2781_acpi_hda_match[] = { {"TIAS2781", 0 }, + {"INT8866", 0 }, {} }; MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
The 14ARB7 has two tas2563 amplifier on i2c. Connect it to the tas2781 driver.
Signed-off-by: Gergo Koteles soyer@irl.hu --- sound/pci/hda/patch_realtek.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index 19040887ff67..67468f7d1c90 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6989,6 +6989,12 @@ static void tas2781_fixup_i2c(struct hda_codec *cdc, tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781"); }
+static void yoga7_14arb7_fixup_i2c(struct hda_codec *cdc, + const struct hda_fixup *fix, int action) +{ + tas2781_generic_fixup(cdc, action, "i2c", "INT8866"); +} + /* for alc295_fixup_hp_top_speakers */ #include "hp_x360_helper.c"
@@ -7460,6 +7466,7 @@ enum { ALC236_FIXUP_DELL_DUAL_CODECS, ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI, ALC287_FIXUP_TAS2781_I2C, + ALC287_FIXUP_YOGA7_14ARB7_I2C, ALC245_FIXUP_HP_MUTE_LED_COEFBIT, ALC245_FIXUP_HP_X360_MUTE_LEDS, ALC287_FIXUP_THINKPAD_I2S_SPK, @@ -9578,6 +9585,12 @@ static const struct hda_fixup alc269_fixups[] = { .chained = true, .chain_id = ALC269_FIXUP_THINKPAD_ACPI, }, + [ALC287_FIXUP_YOGA7_14ARB7_I2C] = { + .type = HDA_FIXUP_FUNC, + .v.func = yoga7_14arb7_fixup_i2c, + .chained = true, + .chain_id = ALC285_FIXUP_THINKPAD_HEADSET_JACK, + }, [ALC245_FIXUP_HP_MUTE_LED_COEFBIT] = { .type = HDA_FIXUP_FUNC, .v.func = alc245_fixup_hp_mute_led_coefbit, @@ -10231,6 +10244,7 @@ static const struct snd_pci_quirk alc269_fixup_tbl[] = { SND_PCI_QUIRK(0x17aa, 0x3853, "Lenovo Yoga 7 15ITL5", ALC287_FIXUP_YOGA7_14ITL_SPEAKERS), SND_PCI_QUIRK(0x17aa, 0x3855, "Legion 7 16ITHG6", ALC287_FIXUP_LEGION_16ITHG6), SND_PCI_QUIRK(0x17aa, 0x3869, "Lenovo Yoga7 14IAL7", ALC287_FIXUP_YOGA9_14IAP7_BASS_SPK_PIN), + SND_PCI_QUIRK(0x17aa, 0x3870, "Lenovo Yoga 7 14ARB7", ALC287_FIXUP_YOGA7_14ARB7_I2C), SND_PCI_QUIRK(0x17aa, 0x387d, "Yoga S780-16 pro Quad AAC", ALC287_FIXUP_TAS2781_I2C), SND_PCI_QUIRK(0x17aa, 0x387e, "Yoga S780-16 pro Quad YC", ALC287_FIXUP_TAS2781_I2C), SND_PCI_QUIRK(0x17aa, 0x3881, "YB9 dual power mode2 YC", ALC287_FIXUP_TAS2781_I2C),
On Sat, 30 Dec 2023 01:09:41 +0100, Gergo Koteles wrote:
The tas2781-hda driver can be modified to support tas2563 as well. Before knowing this information, I created another series for a new driver. Link: https://lore.kernel.org/lkml/cover.1701733441.git.soyer@irl.hu/
This series now extends tas2781-hda.
The tas2563 is a smart amplifier. Similar to tas2562 but with DSP. Some Lenovo laptops have it to drive the bass speakers. By default, it is in software shutdown state.
To make the DSP work it needs a firmware and some calibration data. The latter can be read from the EFI in Lenovo laptops.
For the correct configuration it needs additional register data. It captured after running the Windows driver.
The firmware can be extracted as TAS2563Firmware.bin from the Windows driver with innoextract. https://download.lenovo.com/consumer/mobiles/h5yd037fbfyy7kd0.exe
The driver will search for it as TAS2XXX3870.bin with the Lenovo Yoga 7 14ARB7.
The captured registers extracted with TI's regtool: https://github.com/soyersoyer/tas2563rca/raw/main/INT8866RCA2.bin
Changes since v1:
- fixes were sent as individual patches
- rebased onto for-next
- adding the missed fixup
Gergo Koteles (4): ALSA: hda/tas2781: add ptrs to calibration functions ALSA: hda/tas2781: add configurable global i2c address ALSA: hda/tas2781: add TAS2563 support for 14ARB7 ALSA: hda/tas2781: add fixup for Lenovo 14ARB7
Thanks, I guess I'll take this series later for 6.8 unless any objection is raised from reviewers.
But, I'd like to hear clarifications of some points beforehand:
- Did we get consensus about the ACPI HID? I didn't follow the previous thread completely.
Since those models have been already in the market for quite some time, we'd have to accept "INT8866", I'm afraid. But it's still very important to know whether a similar problem can be avoided in future.
- Will be the firmware files upstreamed to linux-firmware tree later? Otherwise users will have significant difficulties.
Takashi
Hi Takashi,
On Sat, 2023-12-30 at 17:59 +0100, Takashi Iwai wrote:
Thanks, I guess I'll take this series later for 6.8 unless any objection is raised from reviewers.
But, I'd like to hear clarifications of some points beforehand:
- Did we get consensus about the ACPI HID? I didn't follow the previous thread completely.
The INT8866 is a (wrong) PNP ID, that should only be used by the owner "Interphase Corporation". Intel has also mistakenly used the INT PNP prefix in the past, and now TI/leNovo.
Since those models have been already in the market for quite some time, we'd have to accept "INT8866", I'm afraid. But it's still very important to know whether a similar problem can be avoided in future.
- Will be the firmware files upstreamed to linux-firmware tree later? Otherwise users will have significant difficulties.
Shenghao sent the two files to linux-firmware@kernel.org a few days ago, but I think the "Allegedly GPLv2+ ... Found in hex form in kernel source." Licence needs to be fixed before acceptance.
But even if it is not included in the linux-firmware package, it is easier for users to put two files in place per OS installation than patching the kernel.
Regards, Gergo
On Sat, 30 Dec 2023 21:18:06 +0100, Gergo Koteles wrote:
Hi Takashi,
On Sat, 2023-12-30 at 17:59 +0100, Takashi Iwai wrote:
Thanks, I guess I'll take this series later for 6.8 unless any objection is raised from reviewers.
But, I'd like to hear clarifications of some points beforehand:
- Did we get consensus about the ACPI HID? I didn't follow the previous thread completely.
The INT8866 is a (wrong) PNP ID, that should only be used by the owner "Interphase Corporation". Intel has also mistakenly used the INT PNP prefix in the past, and now TI/leNovo.
Yeah, and the question is whether TI / Lenovo recognize the problem and will avoid such a failure in future again.
Since those models have been already in the market for quite some time, we'd have to accept "INT8866", I'm afraid. But it's still very important to know whether a similar problem can be avoided in future.
- Will be the firmware files upstreamed to linux-firmware tree later? Otherwise users will have significant difficulties.
Shenghao sent the two files to linux-firmware@kernel.org a few days ago, but I think the "Allegedly GPLv2+ ... Found in hex form in kernel source." Licence needs to be fixed before acceptance.
OK, that sounds promising.
But even if it is not included in the linux-firmware package, it is easier for users to put two files in place per OS installation than patching the kernel.
Sure.
thanks,
Takashi
participants (4)
-
Gergo Koteles -
kernel test robot -
Mark Brown -
Takashi Iwai