> -----Original Message-----
> From: Takashi Iwai <tiwai@xxxxxxx>
> Sent: Tuesday, September 5, 2023 2:42 PM
> To: Ding, Shenghao <shenghao-ding@xxxxxx>
> Cc: robh+dt@xxxxxxxxxx; andriy.shevchenko@xxxxxxxxxxxxxxx;
> lgirdwood@xxxxxxxxx; perex@xxxxxxxx; pierre-louis.bossart@xxxxxxxxxxxxxxx;
> Lu, Kevin <kevin-lu@xxxxxx>; 13916275206@xxxxxxx; alsa-devel@alsa-
> project.org; linux-kernel@xxxxxxxxxxxxxxx; liam.r.girdwood@xxxxxxxxx;
> mengdong.lin@xxxxxxxxx; Xu, Baojun <baojun.xu@xxxxxx>; thomas.gfeller@q-
> drop.com; Gupta, Peeyush <peeyush@xxxxxx>; Navada Kanyana, Mukund
> <navada@xxxxxx>; broonie@xxxxxxxxxx; gentuser@xxxxxxxxx
> 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@xxxxxx>
> >
> > ---
> > 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
> >
