-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Tuesday, September 5, 2023 2:42 PM To: Ding, Shenghao shenghao-ding@ti.com Cc: robh+dt@kernel.org; andriy.shevchenko@linux.intel.com; lgirdwood@gmail.com; perex@perex.cz; pierre-louis.bossart@linux.intel.com; Lu, Kevin kevin-lu@ti.com; 13916275206@139.com; alsa-devel@alsa- project.org; linux-kernel@vger.kernel.org; liam.r.girdwood@intel.com; mengdong.lin@intel.com; Xu, Baojun baojun.xu@ti.com; thomas.gfeller@q- drop.com; Gupta, Peeyush peeyush@ti.com; Navada Kanyana, Mukund navada@ti.com; broonie@kernel.org; gentuser@gmail.com Subject: [EXTERNAL] Re: [PATCH v3] ALSA: hda/tas2781: Revert structure name back to cs35l41_dev_name
On Tue, 05 Sep 2023 06:13:30 +0200, Shenghao Ding wrote:
Revert structure name back to cs35l41_dev_name, this structure won't be used any more in tas25781 driver code, the "bus" name can be passed by tas2781_generic_fixup, hid is actually "TIAS2781", can be hardcode and become an unneeded argument passed from tas2781_generic_fixup, as to "index", it is a redundant member in tas2781 driver, and have never been used in tas2781 hda driver before. And redefine tas2781_generic_fixup.
OK, now it's clearer.
So the purpose of this patch is rather to simplify the code. The revert of struct name is merely a side-effect.
That said, you can split even more, two logical changes: one is to just simplify the code by the fixed name string, and another is the rename back to struct cs35l41_dev_name.
But honestly speaking, I don't see the need for rename again; the struct may be used by other codecs, and it's fine to keep a generic struct name like the current one. Leaving the struct name as is will make the patch only as a cleanup (and you should correct the patch subject accordingly).
And, I believe a more sensible improvement would be the use of the standard acpi_*_match*() API as Andy already suggested. It can be folded into this cleanup or create a new patch on top of the cleanup.
thanks,
Takashi
OK
Signed-off-by: Shenghao Ding shenghao-ding@ti.com
Changes in v3:
- Redefine tas2781_generic_fixup, remove hid argument, and do not use structure scodec_dev_name any more in tas2781_generic_fixup.
- remove cs35l41_dev_name from comp_match_tas2781_dev_name, which
is
useless in tas2781 driver. bus name is passed by tas2781_generic_fixup, hid is actually "TIAS2781", can be hardcode and unneeded argument passed from tas2781_generic_fixup. Index is a redundant member, and never used in tas2781 hda driver.
- revert from scodec_dev_name back to cs35l41_dev_name, tas2781 pass
only
bus name instead of cs35l41_dev_name, so it seemed unnecessary for tas2781.
- revert from scodec_dev_name back to cs35l41_dev_name in cs35l41_generic_fixup
sound/pci/hda/patch_realtek.c | 27 ++++++++++----------------- 1 file changed, 10 insertions(+), 17 deletions(-)
diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c index a07df6f929..c3e410152b 100644 --- a/sound/pci/hda/patch_realtek.c +++ b/sound/pci/hda/patch_realtek.c @@ -6745,7 +6745,7 @@ static void comp_generic_playback_hook(struct
hda_pcm_stream *hinfo, struct hda_
} }
-struct scodec_dev_name { +struct cs35l41_dev_name { const char *bus; const char *hid; int index; @@ -6754,7 +6754,7 @@ struct scodec_dev_name { /* match the device name in a slightly relaxed manner */ static int comp_match_cs35l41_dev_name(struct device *dev, void *data) {
- struct scodec_dev_name *p = data;
- struct cs35l41_dev_name *p = data; const char *d = dev_name(dev); int n = strlen(p->bus); char tmp[32];
@@ -6773,19 +6773,19 @@ static int
comp_match_cs35l41_dev_name(struct
device *dev, void *data) static int comp_match_tas2781_dev_name(struct
device *dev,
void *data) {
- struct scodec_dev_name *p = data;
- const char *bus = data; const char *d = dev_name(dev);
- int n = strlen(p->bus);
int n = strlen(bus); char tmp[32];
/* check the bus name */
- if (strncmp(d, p->bus, n))
- if (strncmp(d, 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);
snprintf(tmp, sizeof(tmp), "-%s:00", "TIAS2781");
return !strcmp(d + n, tmp);
} @@ -6795,7 +6795,7 @@ static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char { struct device *dev = hda_codec_dev(cdc); struct alc_spec *spec = cdc->spec;
- struct scodec_dev_name *rec;
struct cs35l41_dev_name *rec; int ret, i;
switch (action) {
@@ -6824,24 +6824,17 @@ 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)
- const char *bus)
{ 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);
comp_match_tas2781_dev_name, (void *)bus);&comp_master_ops,
spec->match); if (ret)@@ -6888,7 +6881,7 @@ static void alc287_fixup_legion_16ithg6_speakers(struct hda_codec *cdc, const st static
void tas2781_fixup_i2c(struct hda_codec *cdc,
const struct hda_fixup *fix, int action) {
tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
tas2781_generic_fixup(cdc, action, "i2c");}
/* for alc295_fixup_hp_top_speakers */
2.34.1