Date:   Sun, 21 May 2023 10:02:44 +0200
From:   Takashi Iwai <tiwai@...e.de>
To:     Shenghao Ding <13916275206@....com>
Cc:     broonie@...nel.org, devicetree@...r.kernel.org,
        krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
        lgirdwood@...il.com, perex@...ex.cz,
        pierre-louis.bossart@...ux.intel.com, kevin-lu@...com,
        shenghao-ding@...com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org, x1077012@...com, peeyush@...com,
        navada@...com, gentuser@...il.com, Ryan_Chu@...tron.com,
        Sam_Wu@...tron.com
Subject: 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@....com>

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...

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

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

> --- /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.

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

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

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

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

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

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

> +/**
> + * 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.

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

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

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

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

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

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

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

Powered by blists - more mailing lists