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

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

[Shenghao Ding]

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

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

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

[Takashi Iwai]

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
> 

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

[Ding, Shenghao]

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