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

-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Sunday, May 21, 2023 4:03 PM To: Shenghao Ding 13916275206@139.com Cc: broonie@kernel.org; devicetree@vger.kernel.org; krzysztof.kozlowski+dt@linaro.org; robh+dt@kernel.org; lgirdwood@gmail.com; perex@perex.cz; pierre-louis.bossart@linux.intel.com; Lu, Kevin kevin-lu@ti.com; Ding, Shenghao shenghao-ding@ti.com; alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; Xu, Baojun x1077012@ti.com; Gupta, Peeyush peeyush@ti.com; Navada Kanyana, Mukund navada@ti.com; gentuser@gmail.com; Ryan_Chu@wistron.com; Sam_Wu@wistron.com 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@139.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... 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