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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

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