On Sun, 28 May 2023 00:36:13 +0200,
Shenghao Ding wrote:
>
> Create tas2781 HDA driver.
>
> Signed-off-by: Shenghao Ding <13916275206@xxxxxxx>
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
