Re: [PATCH v4 5/6] ALSA: hda/tas2781: Add tas2781 HDA driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 28/05/2023 à 00:36, Shenghao Ding a écrit :
Create tas2781 HDA driver.

Signed-off-by: Shenghao Ding <13916275206-7R9yAhoRP9E@xxxxxxxxxxxxxxxx>

---
Changes in v4:
  - Add tiwai-l3A5Bk7waGM@xxxxxxxxxxxxxxxx into Cc list
  - remove unused ret in tas2781_hda_playback
  - in all *__put function, return 0, if the value is unchanged
  - remove superfluous
  - rewrite the subid judgement
  - dev_info to dev_dbg
  Changes to be committed:
	modified:   sound/pci/hda/Kconfig
	modified:   sound/pci/hda/Makefile
	new file:   sound/pci/hda/tas2781_hda_i2c.c
---
  sound/pci/hda/Kconfig           |  15 +
  sound/pci/hda/Makefile          |   2 +
  sound/pci/hda/tas2781_hda_i2c.c | 834 ++++++++++++++++++++++++++++++++
  3 files changed, 851 insertions(+)
  create mode 100644 sound/pci/hda/tas2781_hda_i2c.c

[...]

+static int tas2781_acpi_get_i2c_resource(struct acpi_resource
+	*ares, void *data)
+{
+	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *)data;

Nit: Is the cast really needed?
(should you feel like removing it to ease reading, their are a few other onces elsewhere)

+	struct acpi_resource_i2c_serialbus *sb;
+
+	if (i2c_acpi_get_i2c_resource(ares, &sb)) {
+		if (sb->slave_address != TAS2781_GLOBAL_ADDR) {
+			tas_priv->tasdevice[tas_priv->ndev].dev_addr =
+				(unsigned int)sb->slave_address;
+			tas_priv->ndev++;
+		} else
+			tas_priv->glb_addr.dev_addr = TAS2781_GLOBAL_ADDR;
+

Nit: Unneeded NL (or missing {} around the 'else' branch if it is the style you prefer)

+	}
+
+	return 1;
+}

[...]

+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 ret = 0;
+
+	if (tas_priv->rcabin.profile_cfg_id !=
+		ucontrol->value.integer.value[0]) {
+		tas_priv->rcabin.profile_cfg_id =
+			ucontrol->value.integer.value[0];
+		ret = 0;

So ret is always 0?

Either it is not needed and a "return 0;" below would be enough, either the function should be void (if changinf the prototype is possible, not sure), either there is a typo.

+	}
+
+	return ret;
+}
+
+static int tasdevice_create_control(struct tasdevice_priv *tas_priv)
+{
+	char prof_ctrl_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+	struct hda_codec *codec = tas_priv->codec;
+	struct snd_kcontrol_new prof_ctrl = {
+		.name = prof_ctrl_name,
+		.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+		.info = tasdevice_info_profile,
+		.get = tasdevice_get_profile_id,
+		.put = tasdevice_set_profile_id,
+	};
+	int ret;
+
+	/* Create a mixer item for selecting the active profile */
+	scnprintf(prof_ctrl_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN,
+		"spk-profile-id");
+	ret = snd_ctl_add(codec->card, snd_ctl_new1(&prof_ctrl, tas_priv));
+	if (ret) {
+		dev_err(tas_priv->dev, "Failed to add KControl %s = %d\n",

Nit: KControl here, Control a few lines below. I guess they should be the same.

+			prof_ctrl.name, ret);
+		goto out;
+	}
+
+	dev_dbg(tas_priv->dev, "Added Control %s\n", prof_ctrl.name);
+
+out:
+	return ret;
+}

[...]

+static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+	unsigned int nr_program = ucontrol->value.integer.value[0];
+	int ret = 0;
+
+	if (tas_priv->cur_prog != nr_program) {
+		tas_priv->cur_prog = nr_program;
+		ret = 1;

(Base on this, I guess, that the answer above for tasdevice_set_profile_id() is : typo s/0/1/)

+	}
+
+	return ret;
+}
+
+static int tasdevice_config_get(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+

Nit: Unneeded NL

+	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
+
+	ucontrol->value.integer.value[0] = tas_priv->cur_conf;
+
+	return 0;
+}

[...]

+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;
+	int ret = 0;
+
+	//Lenovo devices

Nit: why a different style for comment?

+	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) {
+		ret = -ENODEV;
+		/* 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) {
+			ret = -EINVAL;
+			goto out;

Nit: return -EINVAL; as just a few lines above?

+		}

If so, return -ENODEV; here would be more explicit.
So, 'ret' becomes useless and return 0; at the end of the function looks enough.

+	}
+
+	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);
+	}
+out:
+	return ret;
+}
+
+static void tasdevice_fw_ready(const struct firmware *fmw,
+	void *context)
+{
+	struct tasdevice_priv *tas_priv = (struct tasdevice_priv *)context;
+	struct hda_codec *codec = tas_priv->codec;
+	int i, ret = 0;

Nit: = 0 is not needed

+
+	pm_runtime_get_sync(tas_priv->dev);
+	mutex_lock(&tas_priv->codec_lock);
+
+	ret = tasdevice_rca_parser(tas_priv, fmw);
+	if (ret)
+		goto out;
+	tasdevice_create_control(tas_priv);

CJ




[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux