RE: [EXTERNAL] Re: [PATCH v3] ALSA: hda/tas2781: Revert structure name back to cs35l41_dev_name

[Ding, Shenghao]

> -----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
> >