From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A1D6AC7EE23 for ; Sun, 21 May 2023 08:05:16 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229488AbjEUIFO (ORCPT ); Sun, 21 May 2023 04:05:14 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46334 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229519AbjEUIDI (ORCPT ); Sun, 21 May 2023 04:03:08 -0400 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E3DC8E53; Sun, 21 May 2023 01:02:46 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 779031FD7B; Sun, 21 May 2023 08:02:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1684656165; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=pCK3meqcPobnyOfFHNVNquedHciiEj56/ANbHw3V9G0=; b=g7+YFBuKM8l4Dzz/5km5k9RkyklczO+aJCZDx7HB6Wg0fueUKjgrTy+ACYvF9vfMDaXRPd Z3+sk1771dLOfc5jDPJOLMkxiZVMdCv7H2+gPk1jPW8x/F3ZefExcGHXyWks10NxwDyrYW D7bzKBp5Hzi5XD1jYlGkZdaxGzfMPS4= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1684656165; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=pCK3meqcPobnyOfFHNVNquedHciiEj56/ANbHw3V9G0=; b=8OBNf4PibLW2junhTZok8nZ8XLPBMpy+chGHjUvmsfyxlvSYkrS45BQbKXL3SdZttcOPBZ l5igGOtL/05LfMBQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 142501346B; Sun, 21 May 2023 08:02:45 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id cODvAyXQaWT0LgAAMHmgww (envelope-from ); Sun, 21 May 2023 08:02:45 +0000 Date: Sun, 21 May 2023 10:02:44 +0200 Message-ID: <871qjayuvv.wl-tiwai@suse.de> From: Takashi Iwai 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, kevin-lu@ti.com, shenghao-ding@ti.com, alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org, x1077012@ti.com, peeyush@ti.com, navada@ti.com, gentuser@gmail.com, Ryan_Chu@wistron.com, Sam_Wu@wistron.com Subject: Re: [PATCH v3 4/5] ALSA: hda/tas2781: Add tas2781 HDA driver In-Reply-To: <20230519080227.20224-1-13916275206@139.com> References: <20230519080227.20224-1-13916275206@139.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org 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... > +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