| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4c1b44b5-995a-fac7-a72b-89b8bf816dd2@linux.intel.com>
Date: Fri, 18 Aug 2023 11:00:34 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Shenghao Ding <shenghao-ding@...com>, tiwai@...e.de
Cc: robh+dt@...nel.org, lgirdwood@...il.com, perex@...ex.cz,
kevin-lu@...com, 13916275206@....com, alsa-devel@...a-project.org,
linux-kernel@...r.kernel.org, liam.r.girdwood@...el.com,
mengdong.lin@...el.com, baojun.xu@...com,
thomas.gfeller@...rop.com, peeyush@...com, navada@...com,
broonie@...nel.org, gentuser@...il.com,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver
The first patch in this series has the same commit title as the second
one, can this be updated with a more meaningful description of the two
patches?
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index 44fccfb93cff..ba1b02ed184a 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6721,7 +6721,7 @@ static void comp_generic_playback_hook(struct hda_pcm_stream *hinfo, struct hda_
> }
> }
>
> -struct cs35l41_dev_name {
> +struct scodec_dev_name {
you are changing things in patch_realtek.c that are completely unrelated
to the tas2781, usually the recommendation is that the changes have to
be part of a different patch so that the real addition of tas2781 parts
are easier to review.
> const char *bus;
> const char *hid;
> int index;
> @@ -6730,7 +6730,7 @@ struct cs35l41_dev_name {
> /* match the device name in a slightly relaxed manner */
> static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
> {
> - struct cs35l41_dev_name *p = data;
> + struct scodec_dev_name *p = data;
> const char *d = dev_name(dev);
> int n = strlen(p->bus);
> char tmp[32];
> @@ -6746,12 +6746,32 @@ static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
> return !strcmp(d + n, tmp);
> }
>
> +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);
ACPI can sometimes add :01 suffixes, this looks like the re-invention of
an ACPI helper?
Adding Andy for the ACPI review.
> +
> + return !strcmp(d + n, tmp);
> +}
> +
> static void cs35l41_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 cs35l41_dev_name *rec;
> + struct scodec_dev_name *rec;
> int ret, i;
>
> switch (action) {
> @@ -6779,6 +6799,41 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char
> }
> }
>
> +static void tas2781_generic_fixup(struct hda_codec *cdc, int action,
> + const char *bus, const char *hid)
> +{
> + struct device *dev = hda_codec_dev(cdc);
> + struct alc_spec *spec = cdc->spec;
> + struct scodec_dev_name *rec;
> + int ret;
> +
> + switch (action) {
> + case HDA_FIXUP_ACT_PRE_PROBE:
> + rec = devm_kmalloc(dev, sizeof(*rec), GFP_KERNEL);
> + if (!rec)
> + return;
> + rec->bus = bus;
> + rec->hid = hid;
> + rec->index = 0;
> + spec->comps[0].codec = cdc;
> + component_match_add(dev, &spec->match,
> + comp_match_tas2781_dev_name, rec);
> + ret = component_master_add_with_match(dev, &comp_master_ops,
> + spec->match);
> + if (ret)
> + codec_err(cdc,
> + "Fail to register component aggregator %d\n",
> + ret);
> + else
> + spec->gen.pcm_playback_hook =
> + comp_generic_playback_hook;
> + break;
> + case HDA_FIXUP_ACT_FREE:
This is the first use of FIXUP_ACT_FREE in this function, is this
required/intentional?
> + component_master_del(dev, &comp_master_ops);
Also is there a need to test that the PRE_PROBE actually worked?
> + break;
> + }
> +}
> +
> static void cs35l41_fixup_i2c_two(struct hda_codec *cdc, const struct hda_fixup *fix, int action)
> {
> cs35l41_generic_fixup(cdc, action, "i2c", "CSC3551", 2);
> @@ -6806,6 +6861,12 @@ static void alc287_fixup_legion_16ithg6_speakers(struct hda_codec *cdc, const st
> cs35l41_generic_fixup(cdc, action, "i2c", "CLSA0101", 2);
> }
>
> +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> + const struct hda_fixup *fix, int action)
> +{
> + tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
TI ACPI ID is TXNW
https://uefi.org/ACPI_ID_List?search=TEXAS
There's also a PNP ID PXN
https://uefi.org/PNP_ID_List?search=TEXAS
"TIAS" looks like an invented identifier. It's not uncommon but should
be recorded with a comment if I am not mistaken.
> +}
> +
> /* for alc295_fixup_hp_top_speakers */
> #include "hp_x360_helper.c"
>
> @@ -7231,6 +7292,7 @@ enum {
> ALC295_FIXUP_DELL_INSPIRON_TOP_SPEAKERS,
> ALC236_FIXUP_DELL_DUAL_CODECS,
> ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI,
> + ALC287_FIXUP_TAS2781_I2C,
> };
>
> /* A special fixup for Lenovo C940 and Yoga Duet 7;
> @@ -9309,6 +9371,12 @@ static const struct hda_fixup alc269_fixups[] = {
> .chained = true,
> .chain_id = ALC269_FIXUP_THINKPAD_ACPI,
> },
> + [ALC287_FIXUP_TAS2781_I2C] = {
> + .type = HDA_FIXUP_FUNC,
> + .v.func = tas2781_fixup_i2c,
> + .chained = true,
> + .chain_id = ALC269_FIXUP_THINKPAD_ACPI,
If this is part of the THINKPAD chain, should this fixup name also refer
to THINKPAD, as e.g. ALC287_FIXUP_CS35L41_I2C_2_THINKPAD_ACPI
Powered by blists - more mailing lists