[PATCH 00/16] ALSA: hda/tas2781: Add tas2563 support

[PATCH 00/16] ALSA: hda/tas2781: Add tas2563 support

[Gergo Koteles]

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

[PATCH 01/16] ASoC: tas2781: add support for fw version 0x0503

[Gergo Koteles]

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

[PATCH 02/16] ALSA: hda/tas2781: leave hda_component in usable state

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Gergo Koteles]

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

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Gergo Koteles]

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.

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Gergo Koteles]

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.

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Gergo Koteles]

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.

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Gergo Koteles]

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.

Re: [PATCH 03/16] ASoC: tas2781: disable regmap regcache

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 04/16] ALSA: hda/tas2781: handle missing calibration data

[Gergo Koteles]

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

[PATCH 05/16] ALSA: hda/tas2781: fix typos in comment

[Gergo Koteles]

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

[PATCH 06/16] ASoC: tas2781: add ptrs to calibration functions

[Gergo Koteles]

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

Re: [PATCH 06/16] ASoC: tas2781: add ptrs to calibration functions

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 379 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 07/16] ALSA: hda/tas2781: load_calibration just load

[Gergo Koteles]

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) {
-- 
2.43.0

[PATCH 08/16] ASoC: tas2781: add configurable global_addr

[Gergo Koteles]

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++;
-- 
2.43.0

[PATCH 09/16] ALSA: hda/tas2781: add TAS2563 support for 14ARB7

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH 10/16] ASoC: tas2781: check negative indexes

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH 11/16] ASoC: tas2781: use 0 as default prog/conf index

[Gergo Koteles]

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

Re: [PATCH 11/16] ASoC: tas2781: use 0 as default prog/conf index

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 383 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 11/16] ASoC: tas2781: use 0 as default prog/conf index

[Gergo Koteles]

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.

[PATCH 12/16] ASoC: tas2781: move set_drv_data outside tasdevice_init

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH 13/16] ALSA: hda/tas2781: remove sound controls in unbind

[Gergo Koteles]

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;
 }
-- 
2.43.0

Re: [PATCH 13/16] ALSA: hda/tas2781: remove sound controls in unbind

[kernel test robot]

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-tas2781-leave-hda_component-in-usable-state/20231207-085947
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/8f16576930682297fd08bba5e063a9a1f3150388.1701906455.git.soyer%40irl.hu
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@intel.com/config)
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@intel.com/reproduce)

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 13/16] ALSA: hda/tas2781: remove sound controls in unbind

[kernel test robot]

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-tas2781-leave-hda_component-in-usable-state/20231207-085947
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/8f16576930682297fd08bba5e063a9a1f3150388.1701906455.git.soyer%40irl.hu
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@intel.com/config)
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@intel.com/reproduce)

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 14/16] ALSA: hda/tas2781: call cleaner functions only once

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH 15/16] ALSA: hda/tas2781: reset the amp before component_add

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH 16/16] ALSA: hda/tas2781: configure the amp after firmware load

[Gergo Koteles]

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

Re: [PATCH 00/16] ALSA: hda/tas2781: Add tas2563 support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1064 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/16] ALSA: hda/tas2781: Add tas2563 support

[Takashi Iwai]

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

[PATCH v2 0/4] ALSA: hda/tas2781: Add tas2563 support

[Gergo Koteles]

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

[PATCH v2 1/4] ALSA: hda/tas2781: add ptrs to calibration functions

[Gergo Koteles]

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

[PATCH v2 2/4] ALSA: hda/tas2781: add configurable global i2c address

[Gergo Koteles]

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

[PATCH v2 3/4] ALSA: hda/tas2781: add TAS2563 support for 14ARB7

[Gergo Koteles]

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);
-- 
2.43.0

[PATCH v2 4/4] ALSA: hda/tas2781: add fixup for Lenovo 14ARB7

[Gergo Koteles]

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),
-- 
2.43.0

Re: [PATCH v2 0/4] ALSA: hda/tas2781: Add tas2563 support

[Takashi Iwai]

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

Re: [PATCH v2 0/4] ALSA: hda/tas2781: Add tas2563 support

[Gergo Koteles]

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

Re: [PATCH v2 0/4] ALSA: hda/tas2781: Add tas2563 support

[Takashi Iwai]

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

Re: [PATCH v2 0/4] ALSA: hda/tas2781: Add tas2563 support

[Takashi Iwai]

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

[PATCH 00/16] ALSA: hda/tas2781: Add tas2563 support

[Gergo Koteles]

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