Le 28/05/2023 à 00:36, Shenghao Ding a écrit :
Create tas2781 HDA driver.
Signed-off-by: Shenghao Ding 13916275206-7R9yAhoRP9E@public.gmane.org
Changes in v4:
- Add tiwai-l3A5Bk7waGM@public.gmane.org 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++;} elsetas_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