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

On Sun, 28 May 2023 00:36:13 +0200, Shenghao Ding wrote:

Create tas2781 HDA driver.

Signed-off-by: Shenghao Ding 13916275206@139.com

You still give too short text for this kind of largish and intensive changes. Please write more about what the patch does, caveats, whatever worth to mention.

--- /dev/null +++ b/sound/pci/hda/tas2781_hda_i2c.c +static int tas2781_acpi_get_i2c_resource(struct acpi_resource

• *ares, void *data)

The line break there looks weird...

+{

• struct tasdevice_priv *tas_priv = (struct tasdevice_priv *)data;

Superfluous cast.

+static int tas2781_hda_read_acpi(struct tasdevice_priv *tas_priv,

• 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(tas_priv->dev,
  

• 	"Failed to find an ACPI device for %s\n", hid);
  

• return -ENODEV;
  

• }
• strcpy(tas_priv->dev_name, hid);

Safer to use strscpy().

• physdev = get_device(acpi_get_first_physical_node(adev));
• acpi_dev_put(adev);
• sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
• if (IS_ERR(sub))

• sub = NULL;
  

• dev_info(tas_priv->dev, "subid = %s\n", sub);
• tas_priv->acpi_subsystem_id = sub;
• ret = acpi_dev_get_resources(adev, &resources,

• tas2781_acpi_get_i2c_resource, tas_priv);
  

Referencing adev after acpi_dev_put() doesn't sound good. acpi_dev_put() should be done later instead.

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

Should be ret=1 when the value changes.

Also, you should check the given value. It's not checked in the core side, so any random value can be passed by user-space. (Ditto for other *_put() functions.)

+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");
  

The control name is way too ugly. It's not understandable for human. Please use a more descriptive name.

Also, you need no temporary name buffer. Just pass the constant string to the name field.

+static int tasdevice_dsp_create_ctrls(struct tasdevice_priv

• *tas_priv)

+{

....

• scnprintf(prog_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "spk-prog-id");
• scnprintf(conf_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "spk-conf-id");

Again, too ugly names. Be more descriptive.

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

• }
  

... so at this point, the function still keeps ret=-ENODEV, and it ends up with the final error code to be returned? It's not clear from the code whether it's the intended flow.

+static void tasdevice_fw_ready(const struct firmware *fmw,

• void *context)

+{

• struct tasdevice_priv *tas_priv = (struct tasdevice_priv *)context;

Superfluous cast.

• struct hda_codec *codec = tas_priv->codec;
• int i, ret = 0;

Superfluous initialization.

+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 & 0xFFFF;
• //Lenovo devices
• if ((subid == 0x387d) || (subid == 0x387e) || (subid == 0x3881)

• || (subid == 0x3884) || (subid == 0x3886) || (subid == 0x38a7)
  

• || (subid == 0x38a8) || (subid == 0x38ba) || (subid == 0x38bb)
  

• || (subid == 0x38be) || (subid == 0x38bf) || (subid == 0x38c3)
  

• || (subid == 0x38cb) || (subid == 0x38cd))
  

• tas_priv->catlog_id = LENOVO;
  

• else

• tas_priv->catlog_id = OTHERS;
  

Hmm, I don't like checking subid here, but we can live with it for now...

+static int tas2781_runtime_suspend(struct device *dev) +{

• struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
• int i;
• dev_info(tas_priv->dev, "Runtime Suspend\n");

Don't spew at each runtime suspend/resume. It'll be way too much. It should be dev_dbg(), if any.

thanks,

Takashi