| Message ID | 20230905041331.834-1-shenghao-ding@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] ALSA: hda/tas2781: Revert structure name back to cs35l41_dev_name | expand |
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 > > 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); > ret = component_master_add_with_match(dev, &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 >
> -----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); > > ret = component_master_add_with_match(dev, > &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 > >
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); ret = component_master_add_with_match(dev, &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 */
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. 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(-)