RE: [EXTERNAL] Re: [PATCH v3 4/5] ALSA: hda/tas2781: Add tas2781 HDA driver

[Ding, Shenghao]

-----Original Message-----
From: Takashi Iwai <tiwai@xxxxxxx> 
Sent: Sunday, May 21, 2023 4:03 PM
To: Shenghao Ding <13916275206@xxxxxxx>
Cc: broonie@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; robh+dt@xxxxxxxxxx; lgirdwood@xxxxxxxxx; perex@xxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx; Lu, Kevin <kevin-lu@xxxxxx>; Ding, Shenghao <shenghao-ding@xxxxxx>; alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Xu, Baojun <x1077012@xxxxxx>; Gupta, Peeyush <peeyush@xxxxxx>; Navada Kanyana, Mukund <navada@xxxxxx>; gentuser@xxxxxxxxx; Ryan_Chu@xxxxxxxxxxx; Sam_Wu@xxxxxxxxxxx
Subject: [EXTERNAL] Re: [PATCH v3 4/5] ALSA: hda/tas2781: Add tas2781 HDA driver

On Fri, 19 May 2023 10:02:27 +0200,
Shenghao Ding wrote:
> 
> Create tas2781 HDA driver.
> 
> Signed-off-by: Shenghao Ding <13916275206@xxxxxxx>

First of all, please give more description.  It's far more changes than written in four words.

Also, don't forget to put me on Cc.  I almost overlooked this one.

> diff --git a/sound/pci/hda/patch_realtek.c 
> b/sound/pci/hda/patch_realtek.c index 172ffc2c332b..f5b912f90018 
> 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> +static int comp_match_tas2781_dev_name(struct device *dev, void 
> +*data) {
> +	struct scodec_dev_name *p = data;
> +	const char *d = dev_name(dev);
> +	int n = strlen(p->bus);
> +	char tmp[32];
> +
> +	/* check the bus name */
> +	if (strncmp(d, p->bus, n))
> +		return 0;
> +	/* skip the bus number */
> +	if (isdigit(d[n]))
> +		n++;
> +	/* the rest must be exact matching */
> +	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
> +
> +	return !strcmp(d + n, tmp);
> +}

You don't use the index here...
Accepted
> +static void tas2781_generic_fixup(struct hda_codec *cdc, int action,
> +	const char *bus, const char *hid, int count) {
> +	struct device *dev = hda_codec_dev(cdc);
> +	struct alc_spec *spec = cdc->spec;
> +	struct scodec_dev_name *rec;
> +	int ret, i;
> +
> +	switch (action) {
> +	case HDA_FIXUP_ACT_PRE_PROBE:
> +		for (i = 0; i < count; i++) {
> +			rec = devm_kmalloc(dev, sizeof(*rec), GFP_KERNEL);
> +			if (!rec)
> +				return;
> +			rec->bus = bus;
> +			rec->hid = hid;
> +			rec->index = i;

... and assigning here.  It means that the multiple instances would silently break.  It's better to catch here instead.
Accepted
> +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> +	const struct hda_fixup *fix, int action) {
> +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781", 1); }
> +
>  /* for alc295_fixup_hp_top_speakers */  #include "hp_x360_helper.c"
>  
> @@ -7201,6 +7260,8 @@ enum {
>  	ALC287_FIXUP_YOGA9_14IAP7_BASS_SPK_PIN,
>  	ALC295_FIXUP_DELL_INSPIRON_TOP_SPEAKERS,
>  	ALC236_FIXUP_DELL_DUAL_CODECS,
> +	ALC287_FIXUP_TAS2781_I2C_2,
> +	ALC287_FIXUP_TAS2781_I2C_4
>  };
>  
>  /* A special fixup for Lenovo C940 and Yoga Duet 7; @@ -9189,6 
> +9250,18 @@ static const struct hda_fixup alc269_fixups[] = {
>  		.chained = true,
>  		.chain_id = ALC255_FIXUP_DELL1_MIC_NO_PRESENCE,
>  	},
> +	[ALC287_FIXUP_TAS2781_I2C_2] = {
> +		.type = HDA_FIXUP_FUNC,
> +		.v.func = tas2781_fixup_i2c,
> +		.chained = true,
> +		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
> +	},
> +	[ALC287_FIXUP_TAS2781_I2C_4] = {
> +		.type = HDA_FIXUP_FUNC,
> +		.v.func = tas2781_fixup_i2c,
> +		.chained = true,
> +		.chain_id = ALC269_FIXUP_THINKPAD_ACPI,
> +	},

What's a difference between *_2 and *_4?
Combine them into ALC287_FIXUP_TAS2781_I2C
> --- /dev/null
> +++ b/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;
> +	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;
> +

Did you run checkpatch.pl?  I thought it would complain.
Accept.
> +static void tas2781_hda_playback_hook(struct device *dev, int action) 
> +{
> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	dev_info(tas_priv->dev, "%s: action = %d\n", __func__, action);

Don't use dev_info().  It'd be dev_dbg() at most.
Accept.
> +	switch (action) {
> +	case HDA_GEN_PCM_ACT_OPEN:
> +		pm_runtime_get_sync(dev);
> +		mutex_lock(&tas_priv->codec_lock);
> +		tas_priv->cur_conf = 0;
> +		tas_priv->rcabin.profile_cfg_id = 1;
> +		tasdevice_tuning_switch(tas_priv, 0);
> +		mutex_unlock(&tas_priv->codec_lock);
> +		break;
> +	case HDA_GEN_PCM_ACT_CLOSE:
> +		mutex_lock(&tas_priv->codec_lock);
> +		tasdevice_tuning_switch(tas_priv, 1);
> +		mutex_unlock(&tas_priv->codec_lock);
> +
> +		pm_runtime_mark_last_busy(dev);
> +		pm_runtime_put_autosuspend(dev);
> +		break;
> +	default:
> +		dev_warn(tas_priv->dev, "Playback action not supported: %d\n",
> +			action);
> +		break;
> +	}
> +
> +	if (ret)
> +		dev_err(tas_priv->dev, "Regmap access fail: %d\n", ret);

The ret is never used.
Accept.
> +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);
> +
> +	tas_priv->rcabin.profile_cfg_id = ucontrol->value.integer.value[0];
> +
> +	return 1;

It should return 0 if the value is unchanged.
(Ditto for other *_put functions)
Accept.
> +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,
> +		"tasdev-profile-id");

A too bad name as a control element.  Use a more readable one.
Accept.
> +static int tasdevice_info_programs(struct snd_kcontrol *kcontrol,
> +			struct snd_ctl_elem_info *uinfo)
> +{
> +	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
> +	struct tasdevice_fw *tas_fw = tas_priv->fmw;
> +
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = (int)tas_fw->nr_programs;

The cast is superfluous.
Accept.
> +static int tasdevice_info_configurations(
> +	struct snd_kcontrol *kcontrol, struct snd_ctl_elem_info *uinfo) {
> +	struct tasdevice_priv *tas_priv = snd_kcontrol_chip(kcontrol);
> +	struct tasdevice_fw *tas_fw = tas_priv->fmw;
> +
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> +	uinfo->count = 1;
> +	uinfo->value.integer.min = 0;
> +	uinfo->value.integer.max = (int)tas_fw->nr_configurations - 1;

Ditto.
Accept.
> +/**
> + * tas2781_digital_getvol - get the volum control
> + * @kcontrol: control pointer
> + * @ucontrol: User data
> + * Customer Kcontrol for tas2781 is primarily for regmap booking, 
> +paging
> + * depends on internal regmap mechanism.
> + * tas2781 contains book and page two-level register map, especially
> + * book switching will set the register BXXP00R7F, after switching to 
> +the
> + * correct book, then leverage the mechanism for paging to access the
> + * register.
> + */

You shouldn't use the kerneldoc marker "/**" for local static functions.  It's not a part of API.
Accept.
> +static int tasdevice_dsp_create_ctrls(struct tasdevice_priv
> +	*tas_priv)
> +{
> +	char prog_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	char conf_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> +	struct hda_codec *codec = tas_priv->codec;
> +	struct snd_kcontrol_new prog_ctl = {
> +		.name = prog_name,
> +		.iface = SNDRV_CTL_ELEM_IFACE_CARD,
> +		.info = tasdevice_info_programs,
> +		.get = tasdevice_program_get,
> +		.put = tasdevice_program_put,
> +	};
> +	struct snd_kcontrol_new conf_ctl = {
> +		.name = conf_name,
> +		.iface = SNDRV_CTL_ELEM_IFACE_CARD,
> +		.info = tasdevice_info_configurations,
> +		.get = tasdevice_config_get,
> +		.put = tasdevice_config_put,
> +	};
> +	int ret;
> +
> +	scnprintf(prog_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, "tasdev-prog-id");
> +	scnprintf(conf_name, SNDRV_CTL_ELEM_ID_NAME_MAXLEN, 
> +"tasdev-conf-id");

Please use better names.
Accept.
> +static void tas2781_apply_calib(struct tasdevice_priv *tas_priv) {
> +	unsigned char page_array[CALIB_MAX] = {0x17, 0x18, 0x18, 0x0d, 0x18};
> +	unsigned char rgno_array[CALIB_MAX] = {0x74, 0x0c, 0x14, 0x3c, 
> +0x7c};

Should be static const arrays.
Accept.
> +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 hda_codec *codec = tas_priv->codec;
> +	unsigned int subid = codec->core.subsystem_id & 0xFFFF;
> +	struct tm *tm = &tas_priv->tm;
> +	unsigned int attr, crc;
> +	unsigned int *tmp_val;
> +	efi_status_t status;
> +	int ret = 0;
> +
> +	//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))
> +		efi_guid = EFI_GUID(0x1f52d2a1, 0xbb3a, 0x457d, 0xbc, 0x09,
> +			0x43, 0xa3, 0xf4, 0x31, 0x0a, 0x92);

Here can be a problem: the device ID is embedded here, and it's hard to find out.  You'd better to make it some quirk flag that is set in a common place and check the flag here instead of checking ID at each place.

Do you have example of the solution? I found some quirk flag is static in the patch_realtek.c, can't be accessed outside that file.

> +	crc = crc32(~0, tas_priv->cali_data.data, 84) ^ ~0;
> +	dev_info(tas_priv->dev, "cali crc 0x%08x PK tmp_val 0x%08x\n",
> +		crc, tmp_val[21]);

If it's a dev_info() output, make it more understandable.
I'll fix it
> +	if (crc == tmp_val[21]) {
> +		time64_to_tm(tmp_val[20], 0, tm);
> +		dev_info(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);

Ditto.  Or, make them a debug print instead.
Accepted
> +static int tas2781_runtime_suspend(struct device *dev) {
> +	struct tasdevice_priv *tas_priv = dev_get_drvdata(dev);
> +	int i, ret = 0;
> +
> +	dev_info(tas_priv->dev, "Runtime Suspend\n");

It must be a debug print.  Otherwise it'll be too annoying.
Accepted
Also, as a minor nitpicking, there are many functions that set ret = 0 at the beginning but never used.  The unconditional 0 initialization is often a bad sign indicating that the author doesn't think fully of the code flow.  Please revisit those.


thanks,

Takashi