[PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Shenghao Ding]

Create tas2781 side codec HDA driver for Lenovo Laptops. The quantity
of the speakers has been define in ACPI. All of the tas2781s in the
laptop will be aggregated as one audio speaker. The code supports
realtek codec as the primary codec. Code offers several controls for
digtial/analog gain setting during playback, and other for eq params
setting in case of different audio profiles, such as music, voice,
movie, etc.

Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
Changes in v3:
 - oprimize the indentation
 - fix the ambiguous error message
 - The check of the given value to kcontrol's put func is in the sound/tas2781-comlib.c
 - In tasdevice_info_programs, corrected max value
 - Seperate snd_kcontrol_new tas2781_dsp_controls into two standalone kcontrol.
 - All the controls set as const static
 - Add static for page_array & rgno_array
 - Remove uncessary blank lines
 - Add descriptions for tas2781_save_calibration
 - remove global addr handling in the code
 - checking subid in switch statement in function tas2781_hda_bind
 - add force firmware load Kcontrol
 - rename the kcontrol name to be more undertandable
 - remove Superfluous cast in tasdevice_fw_ready
 - correct weird line break in function tas2781_acpi_get_i2c_resource
 - correct Referencing adev after acpi_dev_put() in tas2781_hda_read_acpi
 - As to checking the given value in tasdevice_set_profile_id, it seems done
   by the tasdevice_info_profile
 - replace strcpy with strscpy in tas2781_hda_read_acpi
 - rewrite the subid judgement
 - Add tiwai@suse.de into Cc list
 - remove the cast in tas2781_acpi_get_i2c_resource
 - remove else in tas2781_acpi_get_i2c_resource
 - fix the return value in tasdevice_set_profile_id
 - remove unneeded NL in tasdevice_config_get
 - Unifiy the comment style
 - remove ret = 0 in tasdevice_fw_ready
 - remove ret in tas2781_save_calibration
 - remove unused ret in tas2781_hda_playback
 - add force firmware load Kcontrol
---
 sound/pci/hda/Kconfig           |  15 +
 sound/pci/hda/Makefile          |   2 +
 sound/pci/hda/tas2781_hda_i2c.c | 858 ++++++++++++++++++++++++++++++++
 3 files changed, 875 insertions(+)
 create mode 100644 sound/pci/hda/tas2781_hda_i2c.c

diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 886255a03e8b..e66257277492 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -130,6 +130,21 @@ config SND_HDA_SCODEC_CS35L41_SPI
 comment "Set to Y if you want auto-loading the side codec driver"
 	depends on SND_HDA=y && SND_HDA_SCODEC_CS35L41_SPI=m
 
+config SND_HDA_SCODEC_TAS2781_I2C
+	tristate "Build TAS2781 HD-audio side codec support for I2C Bus"
+	depends on I2C
+	depends on ACPI
+	depends on SND_SOC
+	select SND_SOC_TAS2781_COMLIB
+	select SND_SOC_TAS2781_FMWLIB
+	select CRC32_SARWATE
+	help
+	  Say Y or M here to include TAS2781 I2C HD-audio side codec support
+	  in snd-hda-intel driver, such as ALC287.
+
+comment "Set to Y if you want auto-loading the side codec driver"
+	depends on SND_HDA=y && SND_HDA_SCODEC_TAS2781_I2C=m
+
 config SND_HDA_CODEC_REALTEK
 	tristate "Build Realtek HD-audio codec support"
 	select SND_HDA_GENERIC
diff --git a/sound/pci/hda/Makefile b/sound/pci/hda/Makefile
index 00d306104484..1c76609690d6 100644
--- a/sound/pci/hda/Makefile
+++ b/sound/pci/hda/Makefile
@@ -32,6 +32,7 @@ snd-hda-scodec-cs35l41-objs :=		cs35l41_hda.o
 snd-hda-scodec-cs35l41-i2c-objs :=	cs35l41_hda_i2c.o
 snd-hda-scodec-cs35l41-spi-objs :=	cs35l41_hda_spi.o
 snd-hda-cs-dsp-ctls-objs :=		hda_cs_dsp_ctl.o
+snd-hda-scodec-tas2781-i2c-objs :=	tas2781_hda_i2c.o
 
 # common driver
 obj-$(CONFIG_SND_HDA) := snd-hda-codec.o
@@ -56,6 +57,7 @@ obj-$(CONFIG_SND_HDA_SCODEC_CS35L41) += snd-hda-scodec-cs35l41.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_I2C) += snd-hda-scodec-cs35l41-i2c.o
 obj-$(CONFIG_SND_HDA_SCODEC_CS35L41_SPI) += snd-hda-scodec-cs35l41-spi.o
 obj-$(CONFIG_SND_HDA_CS_DSP_CONTROLS) += snd-hda-cs-dsp-ctls.o
+obj-$(CONFIG_SND_HDA_SCODEC_TAS2781_I2C) += snd-hda-scodec-tas2781-i2c.o
 
 # this must be the last entry after codec drivers;
 # otherwise the codec patches won't be hooked before the PCI probe
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
new file mode 100644
index 000000000000..4ccbe12ad451
--- /dev/null
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -0,0 +1,858 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// TAS2781 HDA I2C driver
+//
+// Copyright 2023 Texas Instruments, Inc.
+//
+// Author: Shenghao Ding <shenghao-ding@ti.com>
+
+#include <linux/acpi.h>
+#include <linux/crc8.h>
+#include <linux/crc32.h>
+#include <linux/efi.h>
+#include <linux/firmware.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <sound/hda_codec.h>
+#include <sound/soc.h>
+#include <sound/tas2781.h>
+#include <sound/tlv.h>
+#include <sound/tas2781-tlv.h>
+
+#include "hda_local.h"
+#include "hda_auto_parser.h"
+#include "hda_component.h"
+#include "hda_jack.h"
+#include "hda_generic.h"
+
+#define TASDEVICE_SPEAKER_CALIBRATION_SIZE	20
+
+/* No standard control callbacks for SNDRV_CTL_ELEM_IFACE_CARD
+ * Define two controls, one is Volume control callbacks, the other is
+ * flag setting control callbacks.
+ */
+
+/* Volume control callbacks for tas2781 */
+#define ACARD_SINGLE_RANGE_EXT_TLV(xname, xreg, xshift, xmin, xmax, xinvert, \
+	xhandler_get, xhandler_put, tlv_array) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = (xname),\
+	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
+		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
+	.tlv.p = (tlv_array), \
+	.info = snd_soc_info_volsw_range, \
+	.get = xhandler_get, .put = xhandler_put, \
+	.private_value = (unsigned long)&(struct soc_mixer_control) \
+		{.reg = xreg, .rreg = xreg, .shift = xshift, \
+		 .rshift = xshift, .min = xmin, .max = xmax, \
+		 .invert = xinvert} }
+
+/* Flag control callbacks for tas2781 */
+#define ACARD_SINGLE_BOOL_EXT(xname, xdata, xhandler_get, xhandler_put) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = xname, \
+	.info = snd_ctl_boolean_mono_info, \
+	.get = xhandler_get, .put = xhandler_put, \
+	.private_value = xdata }
+
+enum calib_data {
+	R0_VAL = 0,
+	INV_R0,
+	R0LOW,
+	POWER,
+	TLIM,
+	CALIB_MAX
+};
+
+static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)
+{
+	struct tasdevice_priv *tas_priv = data;
+	struct acpi_resource_i2c_serialbus *sb;
+
+	if (i2c_acpi_get_i2c_resource(ares, &sb)) {
+		if (tas_priv->ndev < TASDEVICE_MAX_CHANNELS &&
+			sb->slave_address != TAS2781_GLOBAL_ADDR) {
+			tas_priv->tasdevice[tas_priv->ndev].dev_addr =
+				(unsigned int)sb->slave_address;
+			tas_priv->ndev++;
+		}
+	}
+	return 1;
+}
+
+static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
+{
+	struct acpi_device *adev;
+	struct device *physdev;
+	LIST_HEAD(resources);
+	const char *sub;
+	int ret;
+
+	adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
+	if (!adev) {
+		dev_err(p->dev,
+			"Failed to find an ACPI device for %s\n", hid);
+		return -ENODEV;
+	}
+
+	ret = acpi_dev_get_resources(adev, &resources, tas2781_get_i2c_res, p);
+	if (ret < 0)
+		goto err;
+
+	acpi_dev_free_resource_list(&resources);
+	strscpy(p->dev_name, hid, sizeof(p->dev_name));
+	physdev = get_device(acpi_get_first_physical_node(adev));
+	acpi_dev_put(adev);
+
+	/* No side-effect to the playback even if subsystem_id is NULL*/
+	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
+	if (IS_ERR(sub))
+		sub = NULL;
+
+	p->acpi_subsystem_id = sub;
+
+	put_device(physdev);
+
+	return 0;
+
+err:
+	dev_err(p->dev, "read acpi error, ret: %d\n", ret);
+	put_device(physdev);
+
+	return ret;
+}
+
+static void tas2781_hda_playback_hook(struct device *dev, int action)
+{
+	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
+
+	dev_dbg(tas_priv->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);
+		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);
+
+		pm_runtime_mark_last_busy(dev);
+		pm_runtime_put_autosuspend(dev);
+		break;
+	default:
+		dev_dbg(tas_priv->dev, "Playback action not supported: %d\n",
+			action);
+		break;
+	}
+}
+
+static int tasdevice_info_profile(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_info *uinfo)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = tas_priv->rcabin.ncfgs - 1;
+
+	return 0;
+}
+
+static int tasdevice_get_profile_id(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = tas_priv->rcabin.profile_cfg_id;
+
+	return 0;
+}
+
+static int tasdevice_hda_clamp(int val, int max)
+{
+	if (val > max)
+		val = max;
+
+	if (val < 0)
+		val = 0;
+	return val;
+}
+
+static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	int nr_profile = ucontrol->value.integer.value[0];
+	int max = tas_priv->rcabin.ncfgs - 1;
+	int val, ret = 0;
+
+	val = tasdevice_hda_clamp(nr_profile, max);
+
+	if (tas_priv->rcabin.profile_cfg_id != nr_profile) {
+		tas_priv->rcabin.profile_cfg_id = nr_profile;
+		ret = 1;
+	}
+
+	return ret;
+}
+
+static int tasdevice_info_programs(struct snd_kcontrol *kcontrol,
+			struct snd_ctl_elem_info *uinfo)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct tasdevice_fw *tas_fw = tas_priv->fmw;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = tas_fw->nr_programs - 1;
+
+	return 0;
+}
+
+static int tasdevice_info_config(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_info *uinfo)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct tasdevice_fw *tas_fw = tas_priv->fmw;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = tas_fw->nr_configurations - 1;
+
+	return 0;
+}
+
+static int tasdevice_program_get(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = tas_priv->cur_prog;
+
+	return 0;
+}
+
+static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct tasdevice_fw *tas_fw = tas_priv->fmw;
+	int nr_program = ucontrol->value.integer.value[0];
+	int max = tas_fw->nr_programs - 1;
+	int val, ret = 0;
+
+	val = tasdevice_hda_clamp(nr_program, max);
+
+	if (tas_priv->cur_prog != nr_program) {
+		tas_priv->cur_prog = nr_program;
+		ret = 1;
+	}
+
+	return ret;
+}
+
+static int tasdevice_config_get(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = tas_priv->cur_conf;
+
+	return 0;
+}
+
+static int tasdevice_config_put(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct tasdevice_fw *tas_fw = tas_priv->fmw;
+	int nr_config = ucontrol->value.integer.value[0];
+	int max = tas_fw->nr_configurations - 1;
+	int val, ret = 0;
+
+	val = tasdevice_hda_clamp(nr_config, max);
+
+	if (tas_priv->cur_conf != nr_config) {
+		tas_priv->cur_conf = nr_config;
+		ret = 1;
+	}
+
+	return ret;
+}
+
+/*
+ * tas2781_digital_getvol - get the volum control
+ * @kcontrol: control pointer
+ * @ucontrol: User data
+ * Customer Kcontrol for tas2781 is primarily for regmap booking, paging
+ * depends on internal regmap mechanism.
+ * tas2781 contains book and page two-level register map, especially
+ * book switching will set the register BXXP00R7F, after switching to the
+ * correct book, then leverage the mechanism for paging to access the
+ * register.
+ */
+static int tas2781_digital_getvol(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+
+	return tasdevice_digital_getvol(tas_priv, ucontrol, mc);
+}
+
+static int tas2781_amp_getvol(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+
+	return tasdevice_amp_getvol(tas_priv, ucontrol, mc);
+}
+
+static int tas2781_digital_putvol(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+
+	/* The check of the given value is in tasdevice_digital_putvol. */
+	return tasdevice_digital_putvol(tas_priv, ucontrol, mc);
+}
+
+static int tas2781_amp_putvol(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+
+	/* The check of the given value is in tasdevice_amp_putvol. */
+	return tasdevice_amp_putvol(tas_priv, ucontrol, mc);
+}
+
+static int tas2781_force_fwload_get(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = (int)tas_priv->force_fwload_status;
+	dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__,
+			tas_priv->force_fwload_status ? "ON" : "OFF");
+
+	return 0;
+}
+
+static int tas2781_force_fwload_put(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	bool change, val = (bool)ucontrol->value.integer.value[0];
+
+	if (tas_priv->force_fwload_status == val)
+		change = false;
+	else {
+		change = true;
+		tas_priv->force_fwload_status = val;
+	}
+	dev_dbg(tas_priv->dev, "%s : Force FWload %s\n", __func__,
+		tas_priv->force_fwload_status ? "ON" : "OFF");
+
+	return change;
+}
+
+static const struct snd_kcontrol_new tas2781_snd_controls[] = {
+	ACARD_SINGLE_RANGE_EXT_TLV("Speaker Analog Gain", TAS2781_AMP_LEVEL,
+		1, 0, 20, 0, tas2781_amp_getvol,
+		tas2781_amp_putvol, amp_vol_tlv),
+	ACARD_SINGLE_RANGE_EXT_TLV("Speaker Digital Gain", TAS2781_DVC_LVL,
+		0, 0, 200, 1, tas2781_digital_getvol,
+		tas2781_digital_putvol, dvc_tlv),
+	ACARD_SINGLE_BOOL_EXT("Speaker Force Firmware Load", 0,
+		tas2781_force_fwload_get, tas2781_force_fwload_put),
+};
+
+static const struct snd_kcontrol_new tas2781_prof_ctrl = {
+	.name = "Speaker Profile Id",
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.info = tasdevice_info_profile,
+	.get = tasdevice_get_profile_id,
+	.put = tasdevice_set_profile_id,
+};
+
+static const struct snd_kcontrol_new tas2781_dsp_prog_ctrl = {
+	.name = "Speaker Program Id",
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.info = tasdevice_info_programs,
+	.get = tasdevice_program_get,
+	.put = tasdevice_program_put,
+};
+
+static const struct snd_kcontrol_new tas2781_dsp_conf_ctrl = {
+	.name = "Speaker Config Id",
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.info = tasdevice_info_config,
+	.get = tasdevice_config_get,
+	.put = tasdevice_config_put,
+};
+
+static void tas2781_apply_calib(struct tasdevice_priv *tas_priv)
+{
+	static const unsigned char page_array[CALIB_MAX] = {
+		0x17, 0x18, 0x18, 0x0d, 0x18
+	};
+	static const unsigned char rgno_array[CALIB_MAX] = {
+		0x74, 0x0c, 0x14, 0x3c, 0x7c
+	};
+	unsigned char *data;
+	int i, j, rc;
+
+	for (i = 0; i < tas_priv->ndev; i++) {
+		data = tas_priv->cali_data.data +
+			i * TASDEVICE_SPEAKER_CALIBRATION_SIZE;
+		for (j = 0; j < CALIB_MAX; j++) {
+			rc = tasdevice_dev_bulk_write(tas_priv, i,
+				TASDEVICE_REG(0, page_array[j], rgno_array[j]),
+				&(data[4 * j]), 4);
+			if (rc < 0)
+				dev_err(tas_priv->dev,
+					"chn %d calib %d bulk_wr err = %d\n",
+					i, j, rc);
+		}
+	}
+}
+
+/* Update the calibrate 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
+ * and f0 in real time during playback.
+ */
+static int tas2781_save_calibration(struct tasdevice_priv *tas_priv)
+{
+	efi_guid_t efi_guid = EFI_GUID(0x02f9af02, 0x7734, 0x4233, 0xb4, 0x3d,
+		0x93, 0xfe, 0x5a, 0xa3, 0x5d, 0xb3);
+	static efi_char16_t efi_name[] = L"CALI_DATA";
+	struct tm *tm = &tas_priv->tm;
+	unsigned int attr, crc;
+	unsigned int *tmp_val;
+	efi_status_t status;
+
+	/* Lenovo devices */
+	if (tas_priv->catlog_id == LENOVO)
+		efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, 0x09,
+			0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92);
+
+	tas_priv->cali_data.total_sz = 0;
+	/* Get real size of UEFI variable */
+	status = efi.get_variable(efi_name, &efi_guid, &attr,
+		&tas_priv->cali_data.total_sz, tas_priv->cali_data.data);
+	if (status == EFI_BUFFER_TOO_SMALL) {
+		/* Allocate data buffer of data_size bytes */
+		tas_priv->cali_data.data = devm_kzalloc(tas_priv->dev,
+			tas_priv->cali_data.total_sz, GFP_KERNEL);
+		if (!tas_priv->cali_data.data)
+			return -ENOMEM;
+		/* Get variable contents into buffer */
+		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;
+	}
+
+	tmp_val = (unsigned int *)tas_priv->cali_data.data;
+
+	crc = crc32(~0, tas_priv->cali_data.data, 84) ^ ~0;
+	dev_dbg(tas_priv->dev, "cali crc 0x%08x PK tmp_val 0x%08x\n",
+		crc, tmp_val[21]);
+
+	if (crc == tmp_val[21]) {
+		time64_to_tm(tmp_val[20], 0, tm);
+		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;
+
+	return 0;
+}
+
+static void tasdev_fw_ready(const struct firmware *fmw, void *context)
+{
+	struct tasdevice_priv *tas_priv = context;
+	struct hda_codec *codec = tas_priv->codec;
+	int i, ret;
+
+	pm_runtime_get_sync(tas_priv->dev);
+	mutex_lock(&tas_priv->codec_lock);
+
+	ret = tasdevice_rca_parser(tas_priv, fmw);
+	if (ret)
+		goto out;
+
+	ret = snd_ctl_add(codec->card,
+		snd_ctl_new1(&tas2781_prof_ctrl, tas_priv));
+	if (ret) {
+		dev_err(tas_priv->dev,
+			"Failed to add KControl %s = %d\n",
+			tas2781_prof_ctrl.name, ret);
+		goto out;
+	}
+
+	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));
+		if (ret) {
+			dev_err(tas_priv->dev,
+				"Failed to add KControl %s = %d\n",
+				tas2781_snd_controls[i].name, ret);
+			goto out;
+		}
+	}
+
+	tasdevice_dsp_remove(tas_priv);
+
+	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
+	scnprintf(tas_priv->coef_binaryname, 64, "TAS2XXX%04X.bin",
+		codec->core.subsystem_id & 0xffff);
+	ret = tasdevice_dsp_parser(tas_priv);
+	if (ret) {
+		dev_err(tas_priv->dev, "dspfw load %s error\n",
+			tas_priv->coef_binaryname);
+		tas_priv->fw_state = TASDEVICE_DSP_FW_FAIL;
+		goto out;
+	}
+
+	ret = snd_ctl_add(codec->card,
+		snd_ctl_new1(&tas2781_dsp_prog_ctrl, tas_priv));
+	if (ret) {
+		dev_err(tas_priv->dev,
+			"Failed to add KControl %s = %d\n",
+			tas2781_dsp_prog_ctrl.name, ret);
+		goto out;
+	}
+
+	ret = snd_ctl_add(codec->card,
+		snd_ctl_new1(&tas2781_dsp_conf_ctrl, tas_priv));
+	if (ret) {
+		dev_err(tas_priv->dev,
+			"Failed to add KControl %s = %d\n",
+			tas2781_dsp_conf_ctrl.name, ret);
+		goto out;
+	}
+
+	tas_priv->fw_state = TASDEVICE_DSP_FW_ALL_OK;
+	tasdevice_prmg_load(tas_priv, 0);
+
+	/* If calibrated data occurs error, dsp will still works with default
+	 * calibrated data inside algo.
+	 */
+	tas2781_save_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_priv->codec_lock);
+	if (fmw)
+		release_firmware(fmw);
+	pm_runtime_mark_last_busy(tas_priv->dev);
+	pm_runtime_put_autosuspend(tas_priv->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 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)
+		return -EINVAL;
+
+	comps = &comps[tas_priv->index];
+	if (comps->dev)
+		return -EBUSY;
+
+	codec = comps->codec;
+	subid = codec->core.subsystem_id >> 16;
+
+	switch (subid) {
+	case 0x17aa:
+		tas_priv->catlog_id = LENOVO;
+		break;
+	default:
+		tas_priv->catlog_id = OTHERS;
+		break;
+	}
+
+	pm_runtime_get_sync(dev);
+
+	comps->dev = dev;
+
+	strscpy(comps->name, dev_name(dev), sizeof(comps->name));
+
+	ret = tascodec_init(tas_priv, codec, tasdev_fw_ready);
+	if (ret)
+		return ret;
+
+	comps->playback_hook = tas2781_hda_playback_hook;
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
+static void tas2781_hda_unbind(struct device *dev,
+	struct device *master, void *master_data)
+{
+	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));
+
+	tasdevice_config_info_remove(tas_priv);
+	tasdevice_dsp_remove(tas_priv);
+
+	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
+}
+
+static const struct component_ops tas2781_hda_comp_ops = {
+	.bind = tas2781_hda_bind,
+	.unbind = tas2781_hda_unbind,
+};
+
+static void tas2781_hda_remove(struct device *dev)
+{
+	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
+
+	pm_runtime_get_sync(tas_priv->dev);
+	pm_runtime_disable(tas_priv->dev);
+
+	component_del(tas_priv->dev, &tas2781_hda_comp_ops);
+
+	pm_runtime_put_noidle(tas_priv->dev);
+
+	tasdevice_remove(tas_priv);
+}
+
+static int tas2781_hda_i2c_probe(struct i2c_client *clt)
+{
+	struct tasdevice_priv *tas_priv;
+	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;
+
+	tas_priv->irq_info.irq = clt->irq;
+	ret = tas2781_read_acpi(tas_priv, device_name);
+	if (ret)
+		return dev_err_probe(tas_priv->dev, ret,
+			"Platform not supported\n");
+
+	ret = tasdevice_init(tas_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_put_autosuspend(tas_priv->dev);
+
+	ret = component_add(tas_priv->dev, &tas2781_hda_comp_ops);
+	if (ret) {
+		dev_err(tas_priv->dev, "Register component failed: %d\n", ret);
+		pm_runtime_disable(tas_priv->dev);
+		goto err;
+	}
+
+	tas2781_reset(tas_priv);
+err:
+	if (ret)
+		tas2781_hda_remove(&clt->dev);
+	return ret;
+}
+
+static void tas2781_hda_i2c_remove(struct i2c_client *clt)
+{
+	tas2781_hda_remove(&clt->dev);
+}
+
+static int tas2781_runtime_suspend(struct device *dev)
+{
+	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
+	int i;
+
+	dev_dbg(tas_priv->dev, "Runtime Suspend\n");
+
+	mutex_lock(&tas_priv->codec_lock);
+
+	if (tas_priv->playback_started) {
+		tasdevice_tuning_switch(tas_priv, 1);
+		tas_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;
+	}
+
+	regcache_cache_only(tas_priv->regmap, true);
+	regcache_mark_dirty(tas_priv->regmap);
+
+	mutex_unlock(&tas_priv->codec_lock);
+
+	return 0;
+}
+
+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
+	 * calibrated data inside algo.
+	 */
+	if (tas_priv->cali_data.total_sz > calib_data_sz)
+		tas2781_apply_calib(tas_priv);
+
+out:
+	mutex_unlock(&tas_priv->codec_lock);
+
+	return ret;
+}
+
+static int tas2781_system_suspend(struct device *dev)
+{
+	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
+	int ret;
+
+	dev_dbg(tas_priv->dev, "System Suspend\n");
+
+	ret = pm_runtime_force_suspend(dev);
+	if (ret)
+		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.
+	 * However beyond this point, amps may be powered down.
+	 */
+	return 0;
+}
+
+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");
+
+	ret = pm_runtime_force_resume(dev);
+	if (ret)
+		return ret;
+
+	mutex_lock(&tas_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;
+	}
+	tas2781_reset(tas_priv);
+	tasdevice_prmg_load(tas_priv, tas_priv->cur_prog);
+
+	/* 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);
+	mutex_unlock(&tas_priv->codec_lock);
+
+	return 0;
+}
+
+static const struct dev_pm_ops tas2781_hda_pm_ops = {
+	RUNTIME_PM_OPS(tas2781_runtime_suspend, tas2781_runtime_resume, NULL)
+	SYSTEM_SLEEP_PM_OPS(tas2781_system_suspend, tas2781_system_resume)
+};
+
+static const struct i2c_device_id tas2781_hda_i2c_id[] = {
+	{ "tas2781-hda", 0 },
+	{}
+};
+
+static const struct acpi_device_id tas2781_acpi_hda_match[] = {
+	{"TIAS2781", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
+
+static struct i2c_driver tas2781_hda_i2c_driver = {
+	.driver = {
+		.name		= "tas2781-hda",
+		.acpi_match_table = tas2781_acpi_hda_match,
+		.pm		= &tas2781_hda_pm_ops,
+	},
+	.id_table	= tas2781_hda_i2c_id,
+	.probe_new	= tas2781_hda_i2c_probe,
+	.remove		= tas2781_hda_i2c_remove,
+};
+module_i2c_driver(tas2781_hda_i2c_driver);
+
+MODULE_DESCRIPTION("TAS2781 HDA Driver");
+MODULE_AUTHOR("Shenghao Ding, TI, <shenghao-ding@ti.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(SND_SOC_TAS2781_FMWLIB);
-- 
2.34.1

[PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Shenghao Ding]

Integrate tas2781 configs for Lenovo Laptops. All of the tas2781s in the
laptop will be aggregated as one audio device. The code support realtek
as the primary codec. Rename "struct cs35l41_dev_name" to
"struct scodec_dev_name" for all other side codecs instead of the certain
one.

Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
Changes in v3:
 - remove workaround code for 0x17aa38be, laptop vendor will fix it in acpi.
 - rename comp_match_tas2781_dev_name to avoid indentation
 - simplify the check of vendor id with Lenovo
 - ThinkPad is one of Lenovo's brands, I was suggested me to use
   ALC269_FIXUP_THINKPAD_ACPI.
 - Add comments on ACARD_SINGLE_RANGE_EXT_TLV
 - Add the range check for tas_priv->tasdevice[] in tas2781_acpi_get_i2c_res.
 - remove acpi_subsystem_id
 - Issue in Laptop 0x17aa38be ACPI talbe caused codec->bus->pci->subsystem_device
   is not equal to (codec->core.subsystem_id & 0xffff) in snd_hda_pick_fixup.
   The former is 0x3802 and the latter is 0x38be leads to getting the wrong
   fixup_id and enter into the wrong entry. Although, this issue has been raised
   to the laptop manufacturer, but the ACPI table is locked, cannot be changed
   any more. Correct the wrong entry in the code.
 - Rename "struct cs35l41_dev_name" to "struct scodec_dev_name" for all
   other side codecs instead of one certain codec.
 - Ignore the checkpatch complaints in alc269_fixup_tbl
 - Drop the hunk which is irrelevant with my code in
   alc_fixup_headset_mode_alc255_no_hp_mic
 - Add tiwai@suse.de into Cc list
 - remove useless index
 - combine ALC287_FIXUP_TAS2781_I2C_2 and ALC287_FIXUP_TAS2781_I2C_4 together as
   ALC287_FIXUP_TAS2781_I2C, The code view all the tas2781s in the laptop as one instance.
 - delete the white space at the end of the line in alc_fixup_headset_mode_alc255_no_hp_mic
---
 sound/pci/hda/patch_realtek.c | 88 +++++++++++++++++++++++++++++++++--
 1 file changed, 85 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 44fccfb93cff..ba1b02ed184a 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6721,7 +6721,7 @@ static void comp_generic_playback_hook(struct hda_pcm_stream *hinfo, struct hda_
 	}
 }
 
-struct cs35l41_dev_name {
+struct scodec_dev_name {
 	const char *bus;
 	const char *hid;
 	int index;
@@ -6730,7 +6730,7 @@ struct cs35l41_dev_name {
 /* match the device name in a slightly relaxed manner */
 static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
 {
-	struct cs35l41_dev_name *p = data;
+	struct scodec_dev_name *p = data;
 	const char *d = dev_name(dev);
 	int n = strlen(p->bus);
 	char tmp[32];
@@ -6746,12 +6746,32 @@ static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
 	return !strcmp(d + n, tmp);
 }
 
+static int comp_match_tas2781_dev_name(struct device *dev,
+	void *data)
+{
+	struct scodec_dev_name *p = data;
+	const char *d = dev_name(dev);
+	int n = strlen(p->bus);
+	char tmp[32];
+
+	/* check the bus name */
+	if (strncmp(d, p->bus, n))
+		return 0;
+	/* skip the bus number */
+	if (isdigit(d[n]))
+		n++;
+	/* the rest must be exact matching */
+	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
+
+	return !strcmp(d + n, tmp);
+}
+
 static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char *bus,
 				  const char *hid, int count)
 {
 	struct device *dev = hda_codec_dev(cdc);
 	struct alc_spec *spec = cdc->spec;
-	struct cs35l41_dev_name *rec;
+	struct scodec_dev_name *rec;
 	int ret, i;
 
 	switch (action) {
@@ -6779,6 +6799,41 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char
 	}
 }
 
+static void tas2781_generic_fixup(struct hda_codec *cdc, int action,
+	const char *bus, const char *hid)
+{
+	struct device *dev = hda_codec_dev(cdc);
+	struct alc_spec *spec = cdc->spec;
+	struct scodec_dev_name *rec;
+	int ret;
+
+	switch (action) {
+	case HDA_FIXUP_ACT_PRE_PROBE:
+		rec = devm_kmalloc(dev, sizeof(*rec), GFP_KERNEL);
+		if (!rec)
+			return;
+		rec->bus = bus;
+		rec->hid = hid;
+		rec->index = 0;
+		spec->comps[0].codec = cdc;
+		component_match_add(dev, &spec->match,
+			comp_match_tas2781_dev_name, rec);
+		ret = component_master_add_with_match(dev, &comp_master_ops,
+			spec->match);
+		if (ret)
+			codec_err(cdc,
+				"Fail to register component aggregator %d\n",
+				ret);
+		else
+			spec->gen.pcm_playback_hook =
+				comp_generic_playback_hook;
+		break;
+	case HDA_FIXUP_ACT_FREE:
+		component_master_del(dev, &comp_master_ops);
+		break;
+	}
+}
+
 static void cs35l41_fixup_i2c_two(struct hda_codec *cdc, const struct hda_fixup *fix, int action)
 {
 	cs35l41_generic_fixup(cdc, action, "i2c", "CSC3551", 2);
@@ -6806,6 +6861,12 @@ static void alc287_fixup_legion_16ithg6_speakers(struct hda_codec *cdc, const st
 	cs35l41_generic_fixup(cdc, action, "i2c", "CLSA0101", 2);
 }
 
+static void tas2781_fixup_i2c(struct hda_codec *cdc,
+	const struct hda_fixup *fix, int action)
+{
+	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
+}
+
 /* for alc295_fixup_hp_top_speakers */
 #include "hp_x360_helper.c"
 
@@ -7231,6 +7292,7 @@ enum {
 	ALC295_FIXUP_DELL_INSPIRON_TOP_SPEAKERS,
 	ALC236_FIXUP_DELL_DUAL_CODECS,
 	ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI,
+	ALC287_FIXUP_TAS2781_I2C,
 };
 
 /* A special fixup for Lenovo C940 and Yoga Duet 7;
@@ -9309,6 +9371,12 @@ static const struct hda_fixup alc269_fixups[] = {
 		.chained = true,
 		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
 	},
+	[ALC287_FIXUP_TAS2781_I2C] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = tas2781_fixup_i2c,
+		.chained = true,
+		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
+	},
 };
 
 static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -9884,6 +9952,20 @@ 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, 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 powe mode2 YC", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x3884, "Y780 YG DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x3886, "Y780 VECO DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38a7, "Y780P AMD YG dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38a8, "Y780P AMD VECO dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38ba, "Yoga S780-14.5 Air AMD quad YC", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38bb, "Yoga S780-14.5 Air AMD quad AAC", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38be, "Yoga S980-14.5 proX YC Dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38bf, "Yoga S980-14.5 proX LX Dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38c3, "Y980 DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38cb, "Y790 YG DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38cd, "Y790 VECO DUAL", ALC287_FIXUP_TAS2781_I2C),
 	SND_PCI_QUIRK(0x17aa, 0x3902, "Lenovo E50-80", ALC269_FIXUP_DMIC_THINKPAD_ACPI),
 	SND_PCI_QUIRK(0x17aa, 0x3977, "IdeaPad S210", ALC283_FIXUP_INT_MIC),
 	SND_PCI_QUIRK(0x17aa, 0x3978, "Lenovo B50-70", ALC269_FIXUP_DMIC_THINKPAD_ACPI),
-- 
2.34.1

Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Pierre-Louis Bossart]

The first patch in this series has the same commit title as the second
one, can this be updated with a more meaningful description of the two
patches?


> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 44fccfb93cff..ba1b02ed184a 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6721,7 +6721,7 @@ static void comp_generic_playback_hook(struct hda_pcm_stream *hinfo, struct hda_
>  	}
>  }
>  
> -struct cs35l41_dev_name {
> +struct scodec_dev_name {

you are changing things in patch_realtek.c that are completely unrelated
to the tas2781, usually the recommendation is that the changes have to
be part of a different patch so that the real addition of tas2781 parts
are easier to review.

>  	const char *bus;
>  	const char *hid;
>  	int index;
> @@ -6730,7 +6730,7 @@ struct cs35l41_dev_name {
>  /* match the device name in a slightly relaxed manner */
>  static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
>  {
> -	struct cs35l41_dev_name *p = data;
> +	struct scodec_dev_name *p = data;
>  	const char *d = dev_name(dev);
>  	int n = strlen(p->bus);
>  	char tmp[32];
> @@ -6746,12 +6746,32 @@ static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
>  	return !strcmp(d + n, tmp);
>  }
>  
> +static int comp_match_tas2781_dev_name(struct device *dev,
> +	void *data)
> +{
> +	struct scodec_dev_name *p = data;
> +	const char *d = dev_name(dev);
> +	int n = strlen(p->bus);
> +	char tmp[32];
> +
> +	/* check the bus name */
> +	if (strncmp(d, p->bus, n))
> +		return 0;
> +	/* skip the bus number */
> +	if (isdigit(d[n]))
> +		n++;
> +	/* the rest must be exact matching */
> +	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);

ACPI can sometimes add :01 suffixes, this looks like the re-invention of
an ACPI helper?

Adding Andy for the ACPI review.

> +
> +	return !strcmp(d + n, tmp);
> +}
> +
>  static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char *bus,
>  				  const char *hid, int count)
>  {
>  	struct device *dev = hda_codec_dev(cdc);
>  	struct alc_spec *spec = cdc->spec;
> -	struct cs35l41_dev_name *rec;
> +	struct scodec_dev_name *rec;
>  	int ret, i;
>  
>  	switch (action) {
> @@ -6779,6 +6799,41 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char
>  	}
>  }
>  
> +static void tas2781_generic_fixup(struct hda_codec *cdc, int action,
> +	const char *bus, const char *hid)
> +{
> +	struct device *dev = hda_codec_dev(cdc);
> +	struct alc_spec *spec = cdc->spec;
> +	struct scodec_dev_name *rec;
> +	int ret;
> +
> +	switch (action) {
> +	case HDA_FIXUP_ACT_PRE_PROBE:
> +		rec = devm_kmalloc(dev, sizeof(*rec), GFP_KERNEL);
> +		if (!rec)
> +			return;
> +		rec->bus = bus;
> +		rec->hid = hid;
> +		rec->index = 0;
> +		spec->comps[0].codec = cdc;
> +		component_match_add(dev, &spec->match,
> +			comp_match_tas2781_dev_name, rec);
> +		ret = component_master_add_with_match(dev, &comp_master_ops,
> +			spec->match);
> +		if (ret)
> +			codec_err(cdc,
> +				"Fail to register component aggregator %d\n",
> +				ret);
> +		else
> +			spec->gen.pcm_playback_hook =
> +				comp_generic_playback_hook;
> +		break;
> +	case HDA_FIXUP_ACT_FREE:

This is the first use of FIXUP_ACT_FREE in this function, is this
required/intentional?

> +		component_master_del(dev, &comp_master_ops);

Also is there a need to test that the PRE_PROBE actually worked?

> +		break;
> +	}
> +}
> +
>  static void cs35l41_fixup_i2c_two(struct hda_codec *cdc, const struct hda_fixup *fix, int action)
>  {
>  	cs35l41_generic_fixup(cdc, action, "i2c", "CSC3551", 2);
> @@ -6806,6 +6861,12 @@ static void alc287_fixup_legion_16ithg6_speakers(struct hda_codec *cdc, const st
>  	cs35l41_generic_fixup(cdc, action, "i2c", "CLSA0101", 2);
>  }
>  
> +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> +	const struct hda_fixup *fix, int action)
> +{
> +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");

TI ACPI ID is TXNW

https://uefi.org/ACPI_ID_List?search=TEXAS

There's also a PNP ID PXN

https://uefi.org/PNP_ID_List?search=TEXAS

"TIAS" looks like an invented identifier. It's not uncommon but should
be recorded with a comment if I am not mistaken.


> +}
> +
>  /* for alc295_fixup_hp_top_speakers */
>  #include "hp_x360_helper.c"
>  
> @@ -7231,6 +7292,7 @@ enum {
>  	ALC295_FIXUP_DELL_INSPIRON_TOP_SPEAKERS,
>  	ALC236_FIXUP_DELL_DUAL_CODECS,
>  	ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI,
> +	ALC287_FIXUP_TAS2781_I2C,
>  };
>  
>  /* A special fixup for Lenovo C940 and Yoga Duet 7;
> @@ -9309,6 +9371,12 @@ static const struct hda_fixup alc269_fixups[] = {
>  		.chained = true,
>  		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
>  	},
> +	[ALC287_FIXUP_TAS2781_I2C] = {
> +		.type = HDA_FIXUP_FUNC,
> +		.v.func = tas2781_fixup_i2c,
> +		.chained = true,
> +		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,

If this is part of the THINKPAD chain, should this fixup name also refer
to THINKPAD, as e.g. ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI

Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Pierre-Louis Bossart]

The code doesn't look too bad but needs a bit more work. There are quite
a few error handling issues, pm_runtime needs to be revisited and
ACPI/EFI as well.

> +enum calib_data {

tas2781_calib_data?

> +	R0_VAL = 0,
> +	INV_R0,
> +	R0LOW,
> +	POWER,
> +	TLIM,
> +	CALIB_MAX
> +};
> +
> +static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)
> +{
> +	struct tasdevice_priv *tas_priv = data;
> +	struct acpi_resource_i2c_serialbus *sb;
> +
> +	if (i2c_acpi_get_i2c_resource(ares, &sb)) {
> +		if (tas_priv->ndev < TASDEVICE_MAX_CHANNELS &&
> +			sb->slave_address != TAS2781_GLOBAL_ADDR) {
> +			tas_priv->tasdevice[tas_priv->ndev].dev_addr =
> +				(unsigned int)sb->slave_address;
> +			tas_priv->ndev++;
> +		}
> +	}
> +	return 1;
> +}
> +
> +static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
> +{
> +	struct acpi_device *adev;
> +	struct device *physdev;
> +	LIST_HEAD(resources);
> +	const char *sub;
> +	int ret;
> +
> +	adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
> +	if (!adev) {
> +		dev_err(p->dev,
> +			"Failed to find an ACPI device for %s\n", hid);
> +		return -ENODEV;
> +	}

[1] need to take care of a resource leak here

> +	ret = acpi_dev_get_resources(adev, &resources, tas2781_get_i2c_res, p);
> +	if (ret < 0)
> +		goto err;

you return without doing acpi_dev_put(adev), and you are also doing a
put_device(physdev) which is not initialized yet.

NAK, this needs to be reworked since a simple...


> +
> +	acpi_dev_free_resource_list(&resources);
> +	strscpy(p->dev_name, hid, sizeof(p->dev_name));
> +	physdev = get_device(acpi_get_first_physical_node(adev));
> +	acpi_dev_put(adev);

... move ong those last two lines to [1]

> +
> +	/* No side-effect to the playback even if subsystem_id is NULL*/
> +	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
> +	if (IS_ERR(sub))
> +		sub = NULL;
> +
> +	p->acpi_subsystem_id = sub;
> +
> +	put_device(physdev);
> +
> +	return 0;
> +
> +err:
> +	dev_err(p->dev, "read acpi error, ret: %d\n", ret);
> +	put_device(physdev);
> +
> +	return ret;
> +}
> +
> +static void tas2781_hda_playback_hook(struct device *dev, int action)
> +{
> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +
> +	dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
> +	switch (action) {
> +	case HDA_GEN_PCM_ACT_OPEN:
> +		pm_runtime_get_sync(dev);

test if this actually works?

> +		mutex_lock(&tas_priv->codec_lock);
> +		tasdevice_tuning_switch(tas_priv, 0);
> +		mutex_unlock(&tas_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);

how useful is this codec_lock, is the 'action' not protected at a higher
level?

> +
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +		break;
> +	default:
> +		dev_dbg(tas_priv->dev, "Playback action not supported: %d\n",
> +			action);
> +		break;
> +	}
> +}

> +static int tasdevice_hda_clamp(int val, int max)
> +{
> +	if (val > max)
> +		val = max;
> +
> +	if (val < 0)
> +		val = 0;
> +	return val;
> +}

I've seen that macro in the TAS2783 code as well, that sounds like a
good helper function to share?

> +
> +static int tasdevice_set_profile_id(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
> +	int nr_profile = ucontrol->value.integer.value[0];
> +	int max = tas_priv->rcabin.ncfgs - 1;
> +	int val, ret = 0;
> +
> +	val = tasdevice_hda_clamp(nr_profile, max);
> +
> +	if (tas_priv->rcabin.profile_cfg_id != nr_profile) {
> +		tas_priv->rcabin.profile_cfg_id = nr_profile;
> +		ret = 1;

return 1;
> +	}
> +
> +	return ret;

return 0;

you don't really need a variable here. same comment for other usages.


> +static void tas2781_apply_calib(struct tasdevice_priv *tas_priv)
> +{
> +	static const unsigned char page_array[CALIB_MAX] = {
> +		0x17, 0x18, 0x18, 0x0d, 0x18
> +	};
> +	static const unsigned char rgno_array[CALIB_MAX] = {
> +		0x74, 0x0c, 0x14, 0x3c, 0x7c
> +	};
> +	unsigned char *data;
> +	int i, j, rc;
> +
> +	for (i = 0; i < tas_priv->ndev; i++) {
> +		data = tas_priv->cali_data.data +
> +			i * TASDEVICE_SPEAKER_CALIBRATION_SIZE;
> +		for (j = 0; j < CALIB_MAX; j++) {
> +			rc = tasdevice_dev_bulk_write(tas_priv, i,
> +				TASDEVICE_REG(0, page_array[j], rgno_array[j]),
> +				&(data[4 * j]), 4);
> +			if (rc < 0)
> +				dev_err(tas_priv->dev,
> +					"chn %d calib %d bulk_wr err = %d\n",
> +					i, j, rc);

do you want to keep going or just stop on the first error?

> +		}
> +	}
> +}
> +
> +/* Update the calibrate 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

typos, use a spell checker.

calculation
membrane

> + * and f0 in real time during playback.
> + */
> +static int tas2781_save_calibration(struct tasdevice_priv *tas_priv)
> +{
> +	efi_guid_t efi_guid = EFI_GUID(0x02f9af02, 0x7734, 0x4233, 0xb4, 0x3d,
> +		0x93, 0xfe, 0x5a, 0xa3, 0x5d, 0xb3);

I've seen an EFI use for SoundWire TAS2783, is this the same thing?
It looks very very similar except that this one checks the BUFFER_TOO_SMALL.

If yes, can this be a helper function? If this is the same sort of
calibration we should not have duplicated code really, it's easier to
maintain if there's one set of helpers shared between TI drivers.

> +	static efi_char16_t efi_name[] = L"CALI_DATA";
> +	struct tm *tm = &tas_priv->tm;
> +	unsigned int attr, crc;
> +	unsigned int *tmp_val;
> +	efi_status_t status;
> +
> +	/* Lenovo devices */
> +	if (tas_priv->catlog_id == LENOVO)
> +		efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, 0x09,
> +			0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92);
> +
> +	tas_priv->cali_data.total_sz = 0;
> +	/* Get real size of UEFI variable */
> +	status = efi.get_variable(efi_name, &efi_guid, &attr,
> +		&tas_priv->cali_data.total_sz, tas_priv->cali_data.data);
> +	if (status == EFI_BUFFER_TOO_SMALL) {
> +		/* Allocate data buffer of data_size bytes */
> +		tas_priv->cali_data.data = devm_kzalloc(tas_priv->dev,
> +			tas_priv->cali_data.total_sz, GFP_KERNEL);
> +		if (!tas_priv->cali_data.data)
> +			return -ENOMEM;
> +		/* Get variable contents into buffer */
> +		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;
> +	}
> +
> +	tmp_val = (unsigned int *)tas_priv->cali_data.data;
> +
> +	crc = crc32(~0, tas_priv->cali_data.data, 84) ^ ~0;
> +	dev_dbg(tas_priv->dev, "cali crc 0x%08x PK tmp_val 0x%08x\n",
> +		crc, tmp_val[21]);
> +
> +	if (crc == tmp_val[21]) {
> +		time64_to_tm(tmp_val[20], 0, tm);
> +		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;
> +
> +	return 0;
> +}
> +
> +static void tasdev_fw_ready(const struct firmware *fmw, void *context)
> +{
> +	struct tasdevice_priv *tas_priv = context;
> +	struct hda_codec *codec = tas_priv->codec;
> +	int i, ret;
> +
> +	pm_runtime_get_sync(tas_priv->dev);

test that it worked?

> +	mutex_lock(&tas_priv->codec_lock);

...

> +static int tas2781_hda_bind(struct device *dev, struct device *master,
> +	void *master_data)
> +{
> +	struct tasdevice_priv *tas_priv = 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)
> +		return -EINVAL;
> +
> +	comps = &comps[tas_priv->index];
> +	if (comps->dev)
> +		return -EBUSY;
> +
> +	codec = comps->codec;
> +	subid = codec->core.subsystem_id >> 16;
> +
> +	switch (subid) {
> +	case 0x17aa:

magic number should be a define somewhere...

> +		tas_priv->catlog_id = LENOVO;
> +		break;
> +	default:
> +		tas_priv->catlog_id = OTHERS;
> +		break;
> +	}
> +
> +	pm_runtime_get_sync(dev);

test that it worked?

> +
> +	comps->dev = dev;
> +
> +	strscpy(comps->name, dev_name(dev), sizeof(comps->name));
> +
> +	ret = tascodec_init(tas_priv, codec, tasdev_fw_ready);
> +	if (ret)
> +		return ret;

need to do a put_autosuspend below, this is leaking a refcount.

> +
> +	comps->playback_hook = tas2781_hda_playback_hook;
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +}
> +
> +static void tas2781_hda_unbind(struct device *dev,
> +	struct device *master, void *master_data)
> +{
> +	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));
> +
> +	tasdevice_config_info_remove(tas_priv);
> +	tasdevice_dsp_remove(tas_priv);
> +
> +	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
> +}
> +
> +static const struct component_ops tas2781_hda_comp_ops = {
> +	.bind = tas2781_hda_bind,
> +	.unbind = tas2781_hda_unbind,
> +};
> +
> +static void tas2781_hda_remove(struct device *dev)
> +{
> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +
> +	pm_runtime_get_sync(tas_priv->dev);
> +	pm_runtime_disable(tas_priv->dev);

I don't think this sequence makes any sense.

> +	component_del(tas_priv->dev, &tas2781_hda_comp_ops);
> +
> +	pm_runtime_put_noidle(tas_priv->dev);
> +
> +	tasdevice_remove(tas_priv);
> +}
> +
> +static int tas2781_hda_i2c_probe(struct i2c_client *clt)
> +{
> +	struct tasdevice_priv *tas_priv;
> +	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;
> +
> +	tas_priv->irq_info.irq = clt->irq;
> +	ret = tas2781_read_acpi(tas_priv, device_name);
> +	if (ret)
> +		return dev_err_probe(tas_priv->dev, ret,
> +			"Platform not supported\n");
> +
> +	ret = tasdevice_init(tas_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);

this ..

> +	pm_runtime_enable(tas_priv->dev);
> +
> +	pm_runtime_put_autosuspend(tas_priv->dev);

and this should be removed IMHO. it makes no sense to me.

> +
> +	ret = component_add(tas_priv->dev, &tas2781_hda_comp_ops);
> +	if (ret) {
> +		dev_err(tas_priv->dev, "Register component failed: %d\n", ret);
> +		pm_runtime_disable(tas_priv->dev);
> +		goto err;
> +	}
> +
> +	tas2781_reset(tas_priv);
> +err:
> +	if (ret)
> +		tas2781_hda_remove(&clt->dev);
> +	return ret;
> +}
> +
> +static void tas2781_hda_i2c_remove(struct i2c_client *clt)
> +{
> +	tas2781_hda_remove(&clt->dev);

so for symmetry that's where pm_runtime needs to be disabled.


> +static int tas2781_system_suspend(struct device *dev)
> +{
> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	dev_dbg(tas_priv->dev, "System Suspend\n");
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		return ret;

that's usually the other way around, for system suspend you either want
the device to be pm_runtime active, or if it's already suspended do nothing.

This is very odd to me.

> +
> +	/* 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.
> +	 * However beyond this point, amps may be powered down.
> +	 */
> +	return 0;
> +}
> +
> +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");
> +
> +	ret = pm_runtime_force_resume(dev);
> +	if (ret)
> +		return ret;

that's also not quite right IMHO, this doesn't follow the recommended
sequences for power management.

> +
> +	mutex_lock(&tas_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;
> +	}
> +	tas2781_reset(tas_priv);
> +	tasdevice_prmg_load(tas_priv, tas_priv->cur_prog);
> +
> +	/* 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);
> +	mutex_unlock(&tas_priv->codec_lock);
> +
> +	return 0;
> +}

> +MODULE_IMPORT_NS(SND_SOC_TAS2781_FMWLIB);

Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Andy Shevchenko]

On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:

...

> > +static int comp_match_tas2781_dev_name(struct device *dev,
> > +	void *data)
> > +{
> > +	struct scodec_dev_name *p = data;
> > +	const char *d = dev_name(dev);
> > +	int n = strlen(p->bus);
> > +	char tmp[32];
> > +
> > +	/* check the bus name */
> > +	if (strncmp(d, p->bus, n))
> > +		return 0;

> > +	/* skip the bus number */
> > +	if (isdigit(d[n]))
> > +		n++;

Why do you think it can't be two or more digits?

> > +	/* the rest must be exact matching */
> > +	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
> 
> ACPI can sometimes add :01 suffixes, this looks like the re-invention of
> an ACPI helper?
> 
> Adding Andy for the ACPI review.
> 
> > +	return !strcmp(d + n, tmp);
> > +}

Yes, this looks like reinventing a wheel.
Just compare dev_name() against what is in p->....

...

> > +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> > +	const struct hda_fixup *fix, int action)
> > +{
> > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> 
> TI ACPI ID is TXNW
> 
> https://uefi.org/ACPI_ID_List?search=TEXAS
> 
> There's also a PNP ID PXN
> 
> https://uefi.org/PNP_ID_List?search=TEXAS
> 
> "TIAS" looks like an invented identifier. It's not uncommon but should
> be recorded with a comment if I am not mistaken.
> 
> > +}

Thank you, but actually it's a strong NAK to this even with the comment.
We have to teach people to follow the specification (may be even hard way).

So where did you get the ill-formed ACPI ID?
Is Texas Instrument aware of this?
Can we have a confirmation letter from TI for this ID, please?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

On Fri, 18 Aug 2023 19:01:16 +0200,
Andy Shevchenko wrote:
> 
> On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:
> 
> ...
> 
> > > +static int comp_match_tas2781_dev_name(struct device *dev,
> > > +	void *data)
> > > +{
> > > +	struct scodec_dev_name *p = data;
> > > +	const char *d = dev_name(dev);
> > > +	int n = strlen(p->bus);
> > > +	char tmp[32];
> > > +
> > > +	/* check the bus name */
> > > +	if (strncmp(d, p->bus, n))
> > > +		return 0;
> 
> > > +	/* skip the bus number */
> > > +	if (isdigit(d[n]))
> > > +		n++;
> 
> Why do you think it can't be two or more digits?
> 
> > > +	/* the rest must be exact matching */
> > > +	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
> > 
> > ACPI can sometimes add :01 suffixes, this looks like the re-invention of
> > an ACPI helper?
> > 
> > Adding Andy for the ACPI review.
> > 
> > > +	return !strcmp(d + n, tmp);
> > > +}
> 
> Yes, this looks like reinventing a wheel.
> Just compare dev_name() against what is in p->....

Note that comp_match_tas7281_dev_name() is a copy of
comp_patch_cs35l41_dev_name() and it was implemented in a hackish way
to be applicable to both I2C and SPI device names that have slightly
different naming rules.

> ...
> 
> > > +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> > > +	const struct hda_fixup *fix, int action)
> > > +{
> > > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> > 
> > TI ACPI ID is TXNW
> > 
> > https://uefi.org/ACPI_ID_List?search=TEXAS
> > 
> > There's also a PNP ID PXN
> > 
> > https://uefi.org/PNP_ID_List?search=TEXAS
> > 
> > "TIAS" looks like an invented identifier. It's not uncommon but should
> > be recorded with a comment if I am not mistaken.
> > 
> > > +}
> 
> Thank you, but actually it's a strong NAK to this even with the comment.
> We have to teach people to follow the specification (may be even hard way).
> 
> So where did you get the ill-formed ACPI ID?
> Is Texas Instrument aware of this?
> Can we have a confirmation letter from TI for this ID, please?

This is used already for products that have been long in the market,
so it's way too late to correct it, I'm afraid.

What we can do is to get the confirmation from TI, complain it, and
some verbose comment in the code, indeed.


thanks,

Takashi

Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

On Fri, 18 Aug 2023 18:30:42 +0200,
Pierre-Louis Bossart wrote:
> 
> The code doesn't look too bad but needs a bit more work. There are quite
> a few error handling issues, pm_runtime needs to be revisited and
> ACPI/EFI as well.
> 
> > +enum calib_data {
> 
> tas2781_calib_data?

Well, as long as it's a local stuff, a suffix isn't really needed.
If it makes thing too confusing, it should be named properly, of
course, though.

> > +static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
> > +{
> > +	struct acpi_device *adev;
> > +	struct device *physdev;
> > +	LIST_HEAD(resources);
> > +	const char *sub;
> > +	int ret;
> > +
> > +	adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
> > +	if (!adev) {
> > +		dev_err(p->dev,
> > +			"Failed to find an ACPI device for %s\n", hid);
> > +		return -ENODEV;
> > +	}
> 
> [1] need to take care of a resource leak here

Right, and that's rather a typo at the end of the function...

> > +err:
> > +	dev_err(p->dev, "read acpi error, ret: %d\n", ret);
> > +	put_device(physdev);

... this must be put_device(adev) instead physdev.

> > +static void tas2781_hda_playback_hook(struct device *dev, int action)
> > +{
> > +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> > +
> > +	dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
> > +	switch (action) {
> > +	case HDA_GEN_PCM_ACT_OPEN:
> > +		pm_runtime_get_sync(dev);
> 
> test if this actually works?

To be fair, most of driver codes don't check it, including the
HD-audio core.  (Actually, over 900 of 1300 calls have no check in the
whole tree.)

It implies that forcing the check in each place is moot; rather the
helper needs to be coded not to fail, IMO.

> > +static int tasdevice_hda_clamp(int val, int max)
> > +{
> > +	if (val > max)
> > +		val = max;
> > +
> > +	if (val < 0)
> > +		val = 0;
> > +	return val;
> > +}
> 
> I've seen that macro in the TAS2783 code as well, that sounds like a
> good helper function to share?

There is already clamp() macro, and I guess it can be replaced with
clamp(val, 0, max).

> > +
> > +	comps->dev = dev;
> > +
> > +	strscpy(comps->name, dev_name(dev), sizeof(comps->name));
> > +
> > +	ret = tascodec_init(tas_priv, codec, tasdev_fw_ready);
> > +	if (ret)
> > +		return ret;
> 
> need to do a put_autosuspend below, this is leaking a refcount.

Right, that needs an obvious leak.  Let's fix it.

> > +static int tas2781_system_suspend(struct device *dev)
> > +{
> > +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	dev_dbg(tas_priv->dev, "System Suspend\n");
> > +
> > +	ret = pm_runtime_force_suspend(dev);
> > +	if (ret)
> > +		return ret;
> 
> that's usually the other way around, for system suspend you either want
> the device to be pm_runtime active, or if it's already suspended do nothing.
> 
> This is very odd to me.

This is a normal procedure, as stated in pm_runtime_force_suspend()
definition:

/**
 * pm_runtime_force_suspend - Force a device into suspend state if needed.
....
 * Typically this function may be invoked from a system suspend callback to make
 * sure the device is put into low power state and it should only be used during
 * system-wide PM transitions to sleep states.  It assumes that the analogous
 * pm_runtime_force_resume() will be used to resume the device.


thanks,

Takashi

Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Andy Shevchenko]

On Sun, Aug 20, 2023 at 11:16:08AM +0200, Takashi Iwai wrote:
> On Fri, 18 Aug 2023 19:01:16 +0200,
> Andy Shevchenko wrote:
> > On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:

...

> > > > +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> > > > +	const struct hda_fixup *fix, int action)
> > > > +{
> > > > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> > > 
> > > TI ACPI ID is TXNW
> > > 
> > > https://uefi.org/ACPI_ID_List?search=TEXAS
> > > 
> > > There's also a PNP ID PXN
> > > 
> > > https://uefi.org/PNP_ID_List?search=TEXAS
> > > 
> > > "TIAS" looks like an invented identifier. It's not uncommon but should
> > > be recorded with a comment if I am not mistaken.
> > > 
> > > > +}
> > 
> > Thank you, but actually it's a strong NAK to this even with the comment.
> > We have to teach people to follow the specification (may be even hard way).
> > 
> > So where did you get the ill-formed ACPI ID?
> > Is Texas Instrument aware of this?
> > Can we have a confirmation letter from TI for this ID, please?
> 
> This is used already for products that have been long in the market,
> so it's way too late to correct it, I'm afraid.
> 
> What we can do is to get the confirmation from TI, complain it, and
> some verbose comment in the code, indeed.

Oh, no! Who made that ID, I really want to point that at their faces.
Look at the Coreboot (successful) case, they created something, but
in time asked and then actually fixed the ill-formed ID (that was for
one of RTC chips).

For this, please make sure that commit message has that summary, explaining that
- states that ID is ill-formed
- states that there are products with it (DSDT excerpt is a must)
- lists (a few?) products where that ID is used
- ideally explains who invented that and Cc them to the patch, so they will
  know they made a big mistake

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

On Mon, 21 Aug 2023 11:06:20 +0200,
Andy Shevchenko wrote:
> 
> On Sun, Aug 20, 2023 at 11:16:08AM +0200, Takashi Iwai wrote:
> > On Fri, 18 Aug 2023 19:01:16 +0200,
> > Andy Shevchenko wrote:
> > > On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:
> 
> ...
> 
> > > > > +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> > > > > +	const struct hda_fixup *fix, int action)
> > > > > +{
> > > > > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> > > > 
> > > > TI ACPI ID is TXNW
> > > > 
> > > > https://uefi.org/ACPI_ID_List?search=TEXAS
> > > > 
> > > > There's also a PNP ID PXN
> > > > 
> > > > https://uefi.org/PNP_ID_List?search=TEXAS
> > > > 
> > > > "TIAS" looks like an invented identifier. It's not uncommon but should
> > > > be recorded with a comment if I am not mistaken.
> > > > 
> > > > > +}
> > > 
> > > Thank you, but actually it's a strong NAK to this even with the comment.
> > > We have to teach people to follow the specification (may be even hard way).
> > > 
> > > So where did you get the ill-formed ACPI ID?
> > > Is Texas Instrument aware of this?
> > > Can we have a confirmation letter from TI for this ID, please?
> > 
> > This is used already for products that have been long in the market,
> > so it's way too late to correct it, I'm afraid.
> > 
> > What we can do is to get the confirmation from TI, complain it, and
> > some verbose comment in the code, indeed.
> 
> Oh, no! Who made that ID, I really want to point that at their faces.
> Look at the Coreboot (successful) case, they created something, but
> in time asked and then actually fixed the ill-formed ID (that was for
> one of RTC chips).
> 
> For this, please make sure that commit message has that summary, explaining that
> - states that ID is ill-formed
> - states that there are products with it (DSDT excerpt is a must)
> - lists (a few?) products where that ID is used
> - ideally explains who invented that and Cc them to the patch, so they will
>   know they made a big mistake

Sure, we should complain further and ask them that such a problem
won't happen again.  I'm 100% for it.

But the fact is that lots of machines have been already shipped with
this ID since long time ago, and 99.99% of them have been running on
Windows.  Hence I expect that the chance to get a corrected ID is very
very low, and waiting for the support on Linux until the correction of
ID actually happens makes little sense; that's my point.


Takashi

Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 11:14:59AM +0200, Takashi Iwai wrote:
> On Mon, 21 Aug 2023 11:06:20 +0200,
> Andy Shevchenko wrote:
> > On Sun, Aug 20, 2023 at 11:16:08AM +0200, Takashi Iwai wrote:
> > > On Fri, 18 Aug 2023 19:01:16 +0200,
> > > Andy Shevchenko wrote:
> > > > On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:

...

> > > > > > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> > > > > 
> > > > > TI ACPI ID is TXNW
> > > > > 
> > > > > https://uefi.org/ACPI_ID_List?search=TEXAS
> > > > > 
> > > > > There's also a PNP ID PXN
> > > > > 
> > > > > https://uefi.org/PNP_ID_List?search=TEXAS
> > > > > 
> > > > > "TIAS" looks like an invented identifier. It's not uncommon but should
> > > > > be recorded with a comment if I am not mistaken.
> > > > > 
> > > > > > +}
> > > > 
> > > > Thank you, but actually it's a strong NAK to this even with the comment.
> > > > We have to teach people to follow the specification (may be even hard way).
> > > > 
> > > > So where did you get the ill-formed ACPI ID?
> > > > Is Texas Instrument aware of this?
> > > > Can we have a confirmation letter from TI for this ID, please?
> > > 
> > > This is used already for products that have been long in the market,
> > > so it's way too late to correct it, I'm afraid.
> > > 
> > > What we can do is to get the confirmation from TI, complain it, and
> > > some verbose comment in the code, indeed.
> > 
> > Oh, no! Who made that ID, I really want to point that at their faces.
> > Look at the Coreboot (successful) case, they created something, but
> > in time asked and then actually fixed the ill-formed ID (that was for
> > one of RTC chips).
> > 
> > For this, please make sure that commit message has that summary, explaining that
> > - states that ID is ill-formed
> > - states that there are products with it (DSDT excerpt is a must)
> > - lists (a few?) products where that ID is used
> > - ideally explains who invented that and Cc them to the patch, so they will
> >   know they made a big mistake
> 
> Sure, we should complain further and ask them that such a problem
> won't happen again.  I'm 100% for it.
> 
> But the fact is that lots of machines have been already shipped with
> this ID since long time ago, and 99.99% of them have been running on
> Windows.  Hence I expect that the chance to get a corrected ID is very
> very low, and waiting for the support on Linux until the correction of
> ID actually happens makes little sense; that's my point.

Yes, I understand that. But we have to inform them to prevent from
repeating this big mistake in the future.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Pierre-Louis Bossart]

>>> +static void tas2781_hda_playback_hook(struct device *dev, int action)
>>> +{
>>> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
>>> +
>>> +	dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
>>> +	switch (action) {
>>> +	case HDA_GEN_PCM_ACT_OPEN:
>>> +		pm_runtime_get_sync(dev);
>>
>> test if this actually works?
> 
> To be fair, most of driver codes don't check it, including the
> HD-audio core.  (Actually, over 900 of 1300 calls have no check in the
> whole tree.)
> 
> It implies that forcing the check in each place is moot; rather the
> helper needs to be coded not to fail, IMO.

Maybe that's true for HDaudio, for the SoundWire parts we absolutely
need to detect if the resume worked. There are more steps involved, the
clock-stop mode entry/exit, context restoration, re-enumeration, etc.

I think it'd be a mistake to sit on our hands and assume the world is
perfect. We have to track cases where the codec isn't properly resumed
and prevent it from accessing resources that are just unavailable.

>>> +static int tas2781_system_suspend(struct device *dev)
>>> +{
>>> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	dev_dbg(tas_priv->dev, "System Suspend\n");
>>> +
>>> +	ret = pm_runtime_force_suspend(dev);
>>> +	if (ret)
>>> +		return ret;
>>
>> that's usually the other way around, for system suspend you either want
>> the device to be pm_runtime active, or if it's already suspended do nothing.
>>
>> This is very odd to me.
> 
> This is a normal procedure, as stated in pm_runtime_force_suspend()
> definition:
> 
> /**
>  * pm_runtime_force_suspend - Force a device into suspend state if needed.
> ....
>  * Typically this function may be invoked from a system suspend callback to make
>  * sure the device is put into low power state and it should only be used during
>  * system-wide PM transitions to sleep states.  It assumes that the analogous
>  * pm_runtime_force_resume() will be used to resume the device.

It's possible that it's fine for HDaudio, it wouldn't work in all cases
for SoundWire where we have to make sure all pm_runtime suspended
devices are brought back to D0 and then the regular system suspend
happens. That's mainly because pm_runtime suspend relies on clock stop
and system suspend does not.

In other words, this isn't a generic solution at all.

Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

On Mon, 21 Aug 2023 16:43:31 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> >>> +static void tas2781_hda_playback_hook(struct device *dev, int action)
> >>> +{
> >>> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> >>> +
> >>> +	dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
> >>> +	switch (action) {
> >>> +	case HDA_GEN_PCM_ACT_OPEN:
> >>> +		pm_runtime_get_sync(dev);
> >>
> >> test if this actually works?
> > 
> > To be fair, most of driver codes don't check it, including the
> > HD-audio core.  (Actually, over 900 of 1300 calls have no check in the
> > whole tree.)
> > 
> > It implies that forcing the check in each place is moot; rather the
> > helper needs to be coded not to fail, IMO.
> 
> Maybe that's true for HDaudio, for the SoundWire parts we absolutely
> need to detect if the resume worked. There are more steps involved, the
> clock-stop mode entry/exit, context restoration, re-enumeration, etc.
> 
> I think it'd be a mistake to sit on our hands and assume the world is
> perfect. We have to track cases where the codec isn't properly resumed
> and prevent it from accessing resources that are just unavailable.

Yeah, I don't mean that it's wrong or bad to have the check.  The
check should be there.

But, I feel that it's time to rather switch to the proper call.
Basically pm_runtime_resume_and_get() is the better alternative
(except for its long naming), and we may think of converting the
whole.

> >>> +static int tas2781_system_suspend(struct device *dev)
> >>> +{
> >>> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> >>> +	int ret;
> >>> +
> >>> +	dev_dbg(tas_priv->dev, "System Suspend\n");
> >>> +
> >>> +	ret = pm_runtime_force_suspend(dev);
> >>> +	if (ret)
> >>> +		return ret;
> >>
> >> that's usually the other way around, for system suspend you either want
> >> the device to be pm_runtime active, or if it's already suspended do nothing.
> >>
> >> This is very odd to me.
> > 
> > This is a normal procedure, as stated in pm_runtime_force_suspend()
> > definition:
> > 
> > /**
> >  * pm_runtime_force_suspend - Force a device into suspend state if needed.
> > ....
> >  * Typically this function may be invoked from a system suspend callback to make
> >  * sure the device is put into low power state and it should only be used during
> >  * system-wide PM transitions to sleep states.  It assumes that the analogous
> >  * pm_runtime_force_resume() will be used to resume the device.
> 
> It's possible that it's fine for HDaudio, it wouldn't work in all cases
> for SoundWire where we have to make sure all pm_runtime suspended
> devices are brought back to D0 and then the regular system suspend
> happens. That's mainly because pm_runtime suspend relies on clock stop
> and system suspend does not.
> 
> In other words, this isn't a generic solution at all.

Well, I suppose rather that soundwire is an exception :)

For majority of devices, the system suspend/resume is nothing but
pm_runtime_force_*() calls.  e.g. take a look at
DEFINE_RUNTIME_DEV_PM_OPS() in linux/pm_runtime.h.


Takashi

Re: [PATCH v3 1/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Pierre-Louis Bossart]

On 8/21/23 09:57, Takashi Iwai wrote:
> On Mon, 21 Aug 2023 16:43:31 +0200,
> Pierre-Louis Bossart wrote:
>>
>>
>>>>> +static void tas2781_hda_playback_hook(struct device *dev, int action)
>>>>> +{
>>>>> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
>>>>> +
>>>>> +	dev_dbg(tas_priv->dev, "%s: action = %d\n", __func__, action);
>>>>> +	switch (action) {
>>>>> +	case HDA_GEN_PCM_ACT_OPEN:
>>>>> +		pm_runtime_get_sync(dev);
>>>>
>>>> test if this actually works?
>>>
>>> To be fair, most of driver codes don't check it, including the
>>> HD-audio core.  (Actually, over 900 of 1300 calls have no check in the
>>> whole tree.)
>>>
>>> It implies that forcing the check in each place is moot; rather the
>>> helper needs to be coded not to fail, IMO.
>>
>> Maybe that's true for HDaudio, for the SoundWire parts we absolutely
>> need to detect if the resume worked. There are more steps involved, the
>> clock-stop mode entry/exit, context restoration, re-enumeration, etc.
>>
>> I think it'd be a mistake to sit on our hands and assume the world is
>> perfect. We have to track cases where the codec isn't properly resumed
>> and prevent it from accessing resources that are just unavailable.
> 
> Yeah, I don't mean that it's wrong or bad to have the check.  The
> check should be there.
> 
> But, I feel that it's time to rather switch to the proper call.
> Basically pm_runtime_resume_and_get() is the better alternative
> (except for its long naming), and we may think of converting the
> whole.

Oh, I broke so many drivers by trying a well-indented conversion to
pm_runtime_resume_and_get().
The flow is different wrt -EACCESs and we ended-up with multiple errors.
In hindsight I wish we had left the legacy code alone.

>>>>> +static int tas2781_system_suspend(struct device *dev)
>>>>> +{
>>>>> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
>>>>> +	int ret;
>>>>> +
>>>>> +	dev_dbg(tas_priv->dev, "System Suspend\n");
>>>>> +
>>>>> +	ret = pm_runtime_force_suspend(dev);
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>
>>>> that's usually the other way around, for system suspend you either want
>>>> the device to be pm_runtime active, or if it's already suspended do nothing.
>>>>
>>>> This is very odd to me.
>>>
>>> This is a normal procedure, as stated in pm_runtime_force_suspend()
>>> definition:
>>>
>>> /**
>>>  * pm_runtime_force_suspend - Force a device into suspend state if needed.
>>> ....
>>>  * Typically this function may be invoked from a system suspend callback to make
>>>  * sure the device is put into low power state and it should only be used during
>>>  * system-wide PM transitions to sleep states.  It assumes that the analogous
>>>  * pm_runtime_force_resume() will be used to resume the device.
>>
>> It's possible that it's fine for HDaudio, it wouldn't work in all cases
>> for SoundWire where we have to make sure all pm_runtime suspended
>> devices are brought back to D0 and then the regular system suspend
>> happens. That's mainly because pm_runtime suspend relies on clock stop
>> and system suspend does not.
>>
>> In other words, this isn't a generic solution at all.
> 
> Well, I suppose rather that soundwire is an exception :)
> 
> For majority of devices, the system suspend/resume is nothing but
> pm_runtime_force_*() calls.  e.g. take a look at
> DEFINE_RUNTIME_DEV_PM_OPS() in linux/pm_runtime.h.

I guess you are right. SoundWire has indeed these quirky modes and
differences between SOC vendors that will force us to be extra careful
in what the codec driver implements.

[PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Shenghao Ding]

Integrate tas2781 configs for Lenovo Laptops. All of the tas2781s in the
laptop will be aggregated as one audio device. The code support realtek
as the primary codec. Rename "struct cs35l41_dev_name" to
"struct scodec_dev_name" for all other side codecs instead of the certain
one.

Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
Changes in v3:
 - remove workaround code for 0x17aa38be, laptop vendor will fix it in acpi.
 - rename comp_match_tas2781_dev_name to avoid indentation
 - simplify the check of vendor id with Lenovo
 - ThinkPad is one of Lenovo's brands, I was suggested me to use
   ALC269_FIXUP_THINKPAD_ACPI.
 - Add comments on ACARD_SINGLE_RANGE_EXT_TLV
 - Add the range check for tas_priv->tasdevice[] in tas2781_acpi_get_i2c_res.
 - remove acpi_subsystem_id
 - Issue in Laptop 0x17aa38be ACPI talbe caused codec->bus->pci->subsystem_device
   is not equal to (codec->core.subsystem_id & 0xffff) in snd_hda_pick_fixup.
   The former is 0x3802 and the latter is 0x38be leads to getting the wrong
   fixup_id and enter into the wrong entry. Although, this issue has been raised
   to the laptop manufacturer, but the ACPI table is locked, cannot be changed
   any more. Correct the wrong entry in the code.
 - Rename "struct cs35l41_dev_name" to "struct scodec_dev_name" for all
   other side codecs instead of one certain codec.
 - Ignore the checkpatch complaints in alc269_fixup_tbl
 - Drop the hunk which is irrelevant with my code in
   alc_fixup_headset_mode_alc255_no_hp_mic
 - Add tiwai@suse.de into Cc list
 - remove useless index
 - combine ALC287_FIXUP_TAS2781_I2C_2 and ALC287_FIXUP_TAS2781_I2C_4 together as
   ALC287_FIXUP_TAS2781_I2C, The code view all the tas2781s in the laptop as one instance.
 - delete the white space at the end of the line in alc_fixup_headset_mode_alc255_no_hp_mic
---
 sound/pci/hda/patch_realtek.c | 88 +++++++++++++++++++++++++++++++++--
 1 file changed, 85 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index 44fccfb93cff..ba1b02ed184a 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6721,7 +6721,7 @@ static void comp_generic_playback_hook(struct hda_pcm_stream *hinfo, struct hda_
 	}
 }
 
-struct cs35l41_dev_name {
+struct scodec_dev_name {
 	const char *bus;
 	const char *hid;
 	int index;
@@ -6730,7 +6730,7 @@ struct cs35l41_dev_name {
 /* match the device name in a slightly relaxed manner */
 static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
 {
-	struct cs35l41_dev_name *p = data;
+	struct scodec_dev_name *p = data;
 	const char *d = dev_name(dev);
 	int n = strlen(p->bus);
 	char tmp[32];
@@ -6746,12 +6746,32 @@ static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
 	return !strcmp(d + n, tmp);
 }
 
+static int comp_match_tas2781_dev_name(struct device *dev,
+	void *data)
+{
+	struct scodec_dev_name *p = data;
+	const char *d = dev_name(dev);
+	int n = strlen(p->bus);
+	char tmp[32];
+
+	/* check the bus name */
+	if (strncmp(d, p->bus, n))
+		return 0;
+	/* skip the bus number */
+	if (isdigit(d[n]))
+		n++;
+	/* the rest must be exact matching */
+	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
+
+	return !strcmp(d + n, tmp);
+}
+
 static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char *bus,
 				  const char *hid, int count)
 {
 	struct device *dev = hda_codec_dev(cdc);
 	struct alc_spec *spec = cdc->spec;
-	struct cs35l41_dev_name *rec;
+	struct scodec_dev_name *rec;
 	int ret, i;
 
 	switch (action) {
@@ -6779,6 +6799,41 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char
 	}
 }
 
+static void tas2781_generic_fixup(struct hda_codec *cdc, int action,
+	const char *bus, const char *hid)
+{
+	struct device *dev = hda_codec_dev(cdc);
+	struct alc_spec *spec = cdc->spec;
+	struct scodec_dev_name *rec;
+	int ret;
+
+	switch (action) {
+	case HDA_FIXUP_ACT_PRE_PROBE:
+		rec = devm_kmalloc(dev, sizeof(*rec), GFP_KERNEL);
+		if (!rec)
+			return;
+		rec->bus = bus;
+		rec->hid = hid;
+		rec->index = 0;
+		spec->comps[0].codec = cdc;
+		component_match_add(dev, &spec->match,
+			comp_match_tas2781_dev_name, rec);
+		ret = component_master_add_with_match(dev, &comp_master_ops,
+			spec->match);
+		if (ret)
+			codec_err(cdc,
+				"Fail to register component aggregator %d\n",
+				ret);
+		else
+			spec->gen.pcm_playback_hook =
+				comp_generic_playback_hook;
+		break;
+	case HDA_FIXUP_ACT_FREE:
+		component_master_del(dev, &comp_master_ops);
+		break;
+	}
+}
+
 static void cs35l41_fixup_i2c_two(struct hda_codec *cdc, const struct hda_fixup *fix, int action)
 {
 	cs35l41_generic_fixup(cdc, action, "i2c", "CSC3551", 2);
@@ -6806,6 +6861,12 @@ static void alc287_fixup_legion_16ithg6_speakers(struct hda_codec *cdc, const st
 	cs35l41_generic_fixup(cdc, action, "i2c", "CLSA0101", 2);
 }
 
+static void tas2781_fixup_i2c(struct hda_codec *cdc,
+	const struct hda_fixup *fix, int action)
+{
+	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
+}
+
 /* for alc295_fixup_hp_top_speakers */
 #include "hp_x360_helper.c"
 
@@ -7231,6 +7292,7 @@ enum {
 	ALC295_FIXUP_DELL_INSPIRON_TOP_SPEAKERS,
 	ALC236_FIXUP_DELL_DUAL_CODECS,
 	ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI,
+	ALC287_FIXUP_TAS2781_I2C,
 };
 
 /* A special fixup for Lenovo C940 and Yoga Duet 7;
@@ -9309,6 +9371,12 @@ static const struct hda_fixup alc269_fixups[] = {
 		.chained = true,
 		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
 	},
+	[ALC287_FIXUP_TAS2781_I2C] = {
+		.type = HDA_FIXUP_FUNC,
+		.v.func = tas2781_fixup_i2c,
+		.chained = true,
+		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
+	},
 };
 
 static const struct snd_pci_quirk alc269_fixup_tbl[] = {
@@ -9884,6 +9952,20 @@ 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, 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 powe mode2 YC", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x3884, "Y780 YG DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x3886, "Y780 VECO DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38a7, "Y780P AMD YG dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38a8, "Y780P AMD VECO dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38ba, "Yoga S780-14.5 Air AMD quad YC", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38bb, "Yoga S780-14.5 Air AMD quad AAC", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38be, "Yoga S980-14.5 proX YC Dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38bf, "Yoga S980-14.5 proX LX Dual", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38c3, "Y980 DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38cb, "Y790 YG DUAL", ALC287_FIXUP_TAS2781_I2C),
+	SND_PCI_QUIRK(0x17aa, 0x38cd, "Y790 VECO DUAL", ALC287_FIXUP_TAS2781_I2C),
 	SND_PCI_QUIRK(0x17aa, 0x3902, "Lenovo E50-80", ALC269_FIXUP_DMIC_THINKPAD_ACPI),
 	SND_PCI_QUIRK(0x17aa, 0x3977, "IdeaPad S210", ALC283_FIXUP_INT_MIC),
 	SND_PCI_QUIRK(0x17aa, 0x3978, "Lenovo B50-70", ALC269_FIXUP_DMIC_THINKPAD_ACPI),
-- 
2.34.1