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