Re: [PATCH v4 5/6] ALSA: hda/tas2781: Add tas2781 HDA driver
From: Christophe JAILLET
Date: Sun May 28 2023 - 03:54:59 EST
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