[PATCH v1 1/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[PATCH v1 1/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[Shenghao Ding]

Support ACPI_ID both TXNW2781 and TIAS2781, and revert structure
cs35l41_dev_name.

Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
Changes in v1:
 - Redefine tas2781_generic_fixup, remove hid param
 - TIAS2781 has been used by our customers, see following dstd.dsl. We
    have discussed this with them, they requested TIAS2781 must be
    supported for the laptops already released to market, their new
    laptops will switch to TXNW2781
   Name (_HID, "TIAS2781")  // _HID: Hardware ID
   Name (_UID, Zero)  // _UID: Unique ID
   Method (_SUB, 0, NotSerialized)  // _SUB: Subsystem ID
   {
       If ((SPID == Zero))
       {
          Return ("17AA3886")
       }

       If ((SPID == One))
       {
           Return ("17AA3884")
       }
   }
 - Add TXNW2781 support in comp_match_tas2781_dev_name
 - revert from scodec_dev_name back to cs35l41_dev_name
---
 sound/pci/hda/patch_realtek.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index a07df6f929..1dc50fdbdf 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,27 @@ 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++;
+
+	/* exactly match either TXNW2781 or TIAS2781 */
+	/* the rest must be exact matching */
+	snprintf(tmp, sizeof(tmp), "-%s:00", "TXNW2781");
+
+	if (!strcmp(d + n, tmp))
+		return 1;
+
 	/* 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 +6803,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 +6832,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 +6889,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

[PATCH v1 2/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[Shenghao Ding]

Support ACPI_ID both TXNW2781 and TIAS2781, update dsp/bypass mode
switching in tasdevice_program_put.

Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>

---
Changes in v1:
 - Add comment on dsp/bypass mode in tasdevice_program_put and
   tasdevice_info_programs
 - TIAS2781 has been used by our customers, see following dstd.dsl. We
   have discussed this with them, they requested TIAS2781 must be
   supported for the laptops already released to market, their new laptop
   can switch to TXNW2781
   Name (_HID, "TIAS2781")  // _HID: Hardware ID
   Name (_UID, Zero)  // _UID: Unique ID
   Method (_SUB, 0, NotSerialized)  // _SUB: Subsystem ID
   {
       If ((SPID == Zero))
       {
          Return ("17AA3886")
       }

       If ((SPID == One))
       {
           Return ("17AA3884")
       }
   }
---
 sound/pci/hda/tas2781_hda_i2c.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index fb80280293..5250d300a2 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -199,8 +199,11 @@ static int tasdevice_info_programs(struct snd_kcontrol *kcontrol,
 
 	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
 	uinfo->count = 1;
+	/* 0:			dsp mode
+	 * non-zero:	bypass mode
+	 */
 	uinfo->value.integer.min = 0;
-	uinfo->value.integer.max = tas_fw->nr_programs - 1;
+	uinfo->value.integer.max = tas_fw->nr_programs;
 
 	return 0;
 }
@@ -238,7 +241,10 @@ static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
 	int max = tas_fw->nr_programs - 1;
 	int val, ret = 0;
 
-	val = clamp(nr_program, 0, max);
+	/* 0:			dsp mode
+	 * non-zero:	bypass mode
+	 */
+	val = (nr_program) ? max : 0;
 
 	if (tas_priv->cur_prog != val) {
 		tas_priv->cur_prog = val;
@@ -647,7 +653,9 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
 	const char *device_name;
 	int ret;
 
-	if (strstr(dev_name(&clt->dev), "TIAS2781"))
+	if (strstr(dev_name(&clt->dev), "TXNW2781"))
+		device_name = "TXNW2781";
+	else if (strstr(dev_name(&clt->dev), "TIAS2781"))
 		device_name = "TIAS2781";
 	else
 		return -ENODEV;
@@ -824,6 +832,7 @@ static const struct i2c_device_id tas2781_hda_i2c_id[] = {
 
 static const struct acpi_device_id tas2781_acpi_hda_match[] = {
 	{"TIAS2781", 0 },
+	{"TXNW2781", 1 },
 	{}
 };
 MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
-- 
2.34.1

Re: [PATCH v1 2/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[Takashi Iwai]

On Sun, 03 Sep 2023 16:37:58 +0200,
Shenghao Ding wrote:
> 
> Support ACPI_ID both TXNW2781 and TIAS2781, update dsp/bypass mode
> switching in tasdevice_program_put.

Again, if you change multiple things, split to individual patches.


thanks,

Takashi


> 
> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> 
> ---
> Changes in v1:
>  - Add comment on dsp/bypass mode in tasdevice_program_put and
>    tasdevice_info_programs
>  - TIAS2781 has been used by our customers, see following dstd.dsl. We
>    have discussed this with them, they requested TIAS2781 must be
>    supported for the laptops already released to market, their new laptop
>    can switch to TXNW2781
>    Name (_HID, "TIAS2781")  // _HID: Hardware ID
>    Name (_UID, Zero)  // _UID: Unique ID
>    Method (_SUB, 0, NotSerialized)  // _SUB: Subsystem ID
>    {
>        If ((SPID == Zero))
>        {
>           Return ("17AA3886")
>        }
> 
>        If ((SPID == One))
>        {
>            Return ("17AA3884")
>        }
>    }
> ---
>  sound/pci/hda/tas2781_hda_i2c.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> index fb80280293..5250d300a2 100644
> --- a/sound/pci/hda/tas2781_hda_i2c.c
> +++ b/sound/pci/hda/tas2781_hda_i2c.c
> @@ -199,8 +199,11 @@ static int tasdevice_info_programs(struct snd_kcontrol *kcontrol,
>  
>  	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
>  	uinfo->count = 1;
> +	/* 0:			dsp mode
> +	 * non-zero:	bypass mode
> +	 */
>  	uinfo->value.integer.min = 0;
> -	uinfo->value.integer.max = tas_fw->nr_programs - 1;
> +	uinfo->value.integer.max = tas_fw->nr_programs;
>  
>  	return 0;
>  }
> @@ -238,7 +241,10 @@ static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
>  	int max = tas_fw->nr_programs - 1;
>  	int val, ret = 0;
>  
> -	val = clamp(nr_program, 0, max);
> +	/* 0:			dsp mode
> +	 * non-zero:	bypass mode
> +	 */
> +	val = (nr_program) ? max : 0;
>  
>  	if (tas_priv->cur_prog != val) {
>  		tas_priv->cur_prog = val;
> @@ -647,7 +653,9 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
>  	const char *device_name;
>  	int ret;
>  
> -	if (strstr(dev_name(&clt->dev), "TIAS2781"))
> +	if (strstr(dev_name(&clt->dev), "TXNW2781"))
> +		device_name = "TXNW2781";
> +	else if (strstr(dev_name(&clt->dev), "TIAS2781"))
>  		device_name = "TIAS2781";
>  	else
>  		return -ENODEV;
> @@ -824,6 +832,7 @@ static const struct i2c_device_id tas2781_hda_i2c_id[] = {
>  
>  static const struct acpi_device_id tas2781_acpi_hda_match[] = {
>  	{"TIAS2781", 0 },
> +	{"TXNW2781", 1 },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
> -- 
> 2.34.1
> 

Re: [PATCH v1 2/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[Takashi Iwai]

On Sun, 03 Sep 2023 18:33:34 +0200,
Takashi Iwai wrote:
> 
> On Sun, 03 Sep 2023 16:37:58 +0200,
> Shenghao Ding wrote:
> > 
> > Support ACPI_ID both TXNW2781 and TIAS2781, update dsp/bypass mode
> > switching in tasdevice_program_put.
> 
> Again, if you change multiple things, split to individual patches.

Also, the support of two IDs can be unified to a single patch for
changing both files.

A patch isn't necessarily split per file; instead, it's split per
logic.  If applying the changes to multiple files at the same time
makes more sense, those should be put in the same patch.


thanks,

Takashi


> 
> 
> thanks,
> 
> Takashi
> 
> 
> > 
> > Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> > 
> > ---
> > Changes in v1:
> >  - Add comment on dsp/bypass mode in tasdevice_program_put and
> >    tasdevice_info_programs
> >  - TIAS2781 has been used by our customers, see following dstd.dsl. We
> >    have discussed this with them, they requested TIAS2781 must be
> >    supported for the laptops already released to market, their new laptop
> >    can switch to TXNW2781
> >    Name (_HID, "TIAS2781")  // _HID: Hardware ID
> >    Name (_UID, Zero)  // _UID: Unique ID
> >    Method (_SUB, 0, NotSerialized)  // _SUB: Subsystem ID
> >    {
> >        If ((SPID == Zero))
> >        {
> >           Return ("17AA3886")
> >        }
> > 
> >        If ((SPID == One))
> >        {
> >            Return ("17AA3884")
> >        }
> >    }
> > ---
> >  sound/pci/hda/tas2781_hda_i2c.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> > index fb80280293..5250d300a2 100644
> > --- a/sound/pci/hda/tas2781_hda_i2c.c
> > +++ b/sound/pci/hda/tas2781_hda_i2c.c
> > @@ -199,8 +199,11 @@ static int tasdevice_info_programs(struct snd_kcontrol *kcontrol,
> >  
> >  	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
> >  	uinfo->count = 1;
> > +	/* 0:			dsp mode
> > +	 * non-zero:	bypass mode
> > +	 */
> >  	uinfo->value.integer.min = 0;
> > -	uinfo->value.integer.max = tas_fw->nr_programs - 1;
> > +	uinfo->value.integer.max = tas_fw->nr_programs;
> >  
> >  	return 0;
> >  }
> > @@ -238,7 +241,10 @@ static int tasdevice_program_put(struct snd_kcontrol *kcontrol,
> >  	int max = tas_fw->nr_programs - 1;
> >  	int val, ret = 0;
> >  
> > -	val = clamp(nr_program, 0, max);
> > +	/* 0:			dsp mode
> > +	 * non-zero:	bypass mode
> > +	 */
> > +	val = (nr_program) ? max : 0;
> >  
> >  	if (tas_priv->cur_prog != val) {
> >  		tas_priv->cur_prog = val;
> > @@ -647,7 +653,9 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
> >  	const char *device_name;
> >  	int ret;
> >  
> > -	if (strstr(dev_name(&clt->dev), "TIAS2781"))
> > +	if (strstr(dev_name(&clt->dev), "TXNW2781"))
> > +		device_name = "TXNW2781";
> > +	else if (strstr(dev_name(&clt->dev), "TIAS2781"))
> >  		device_name = "TIAS2781";
> >  	else
> >  		return -ENODEV;
> > @@ -824,6 +832,7 @@ static const struct i2c_device_id tas2781_hda_i2c_id[] = {
> >  
> >  static const struct acpi_device_id tas2781_acpi_hda_match[] = {
> >  	{"TIAS2781", 0 },
> > +	{"TXNW2781", 1 },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
> > -- 
> > 2.34.1
> > 

Re: [PATCH v1 1/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[Takashi Iwai]

On Sun, 03 Sep 2023 16:37:57 +0200,
Shenghao Ding wrote:
> 
> Support ACPI_ID both TXNW2781 and TIAS2781, and revert structure
> cs35l41_dev_name.

Could you explain why you need to revert?  That's the important
piece.

And, what is found in below...

> ---
> Changes in v1:
>  - Redefine tas2781_generic_fixup, remove hid param
>  - TIAS2781 has been used by our customers, see following dstd.dsl. We
>     have discussed this with them, they requested TIAS2781 must be
>     supported for the laptops already released to market, their new
>     laptops will switch to TXNW2781
>    Name (_HID, "TIAS2781")  // _HID: Hardware ID
>    Name (_UID, Zero)  // _UID: Unique ID
>    Method (_SUB, 0, NotSerialized)  // _SUB: Subsystem ID
>    {
>        If ((SPID == Zero))
>        {
>           Return ("17AA3886")
>        }
> 
>        If ((SPID == One))
>        {
>            Return ("17AA3884")
>        }

... such information should be mentioned in the changelog.
It can be even commented in the code.

Last but not least, if you do two things, split to two patches --
unless the rename is closely tied with the support of both IDs.


thanks,

Takashi

Re: [PATCH v1 1/2] ALSA: hda/tas2781: Update tas2781 HDA driver

[Andy Shevchenko]

On Sun, Sep 03, 2023 at 10:37:57PM +0800, Shenghao Ding wrote:
> Support ACPI_ID both TXNW2781 and TIAS2781, and revert structure
> cs35l41_dev_name.

...

> +	/* exactly match either TXNW2781 or TIAS2781 */
> +	/* the rest must be exact matching */
> +	snprintf(tmp, sizeof(tmp), "-%s:00", "TXNW2781");
> +
> +	if (!strcmp(d + n, tmp))
> +		return 1;
> +
>  	/* the rest must be exact matching */
> -	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
> +	snprintf(tmp, sizeof(tmp), "-%s:00", "TIAS2781");

Basically this should use acpi_dev_match infra like Intel drivers do.
But I haven't read code deeply to understand if it's the case here or
not.

-- 
With Best Regards,
Andy Shevchenko