diff mbox series

[v1] ALSA: hda/tas2781: Support ACPI_ID both TXNW2781 and TIAS2781

Message ID 20230910072704.1359-1-shenghao-ding@ti.com (mailing list archive)
State New, archived
Headers show
Series [v1] ALSA: hda/tas2781: Support ACPI_ID both TXNW2781 and TIAS2781 | expand

Commit Message

Shenghao Ding Sept. 10, 2023, 7:27 a.m. UTC
Support ACPI_ID both TXNW2781 and TIAS2781, TXNW2781 is the only one legal
ACPI ID, TIAS2781 is the widely-used ACPI ID named by our customers, so
far it is not registered. 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.

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

---
Changes in v1:
 - Add TXNW2781 into tas2781_acpi_hda_match and move it to the top
 - 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
---
 sound/pci/hda/patch_realtek.c   | 36 ++++++++++++++++++---------------
 sound/pci/hda/tas2781_hda_i2c.c | 33 ++++++++++++++++++------------
 2 files changed, 40 insertions(+), 29 deletions(-)

Comments

Biju Das Sept. 10, 2023, 7:52 a.m. UTC | #1
Hi Shenghao Ding,

Thanks for the patch.

> Subject: [PATCH v1] ALSA: hda/tas2781: Support ACPI_ID both TXNW2781 and
> TIAS2781
> 
> Support ACPI_ID both TXNW2781 and TIAS2781, TXNW2781 is the only one legal
> ACPI ID, TIAS2781 is the widely-used ACPI ID named by our customers, so far
> it is not registered. 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.
> 
> Signed-off-by: Shenghao Ding <shenghao-ding@ti.com>
> 
> ---
> Changes in v1:
>  - Add TXNW2781 into tas2781_acpi_hda_match and move it to the top
>  - 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
> ---
>  sound/pci/hda/patch_realtek.c   | 36 ++++++++++++++++++---------------
>  sound/pci/hda/tas2781_hda_i2c.c | 33 ++++++++++++++++++------------
>  2 files changed, 40 insertions(+), 29 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
> index b7e78bfcff..6dae58a8ef 100644
> --- a/sound/pci/hda/patch_realtek.c
> +++ b/sound/pci/hda/patch_realtek.c
> @@ -6770,24 +6770,35 @@ static int comp_match_cs35l41_dev_name(struct
> device *dev, void *data)
>  	return !strcmp(d + n, tmp);
>  }
> 
> +/* TIAS2781 is the unofficial ACPI id, but widely used in current devices.
> + * TXNW2781 is the official ACPI id, and will be used in the new devices.
> + * Check TIAS2781 or TXNW2781
> + */
>  static int comp_match_tas2781_dev_name(struct device *dev,
>  	void *data)
>  {
> -	struct scodec_dev_name *p = data;
> +	const char c[][10] = { "TXNW2781", "TIAS2781" };

This you should get from match_data().
See below.

>  	const char *d = dev_name(dev);
> -	int n = strlen(p->bus);
> +	const char *bus = data;
> +	int n = strlen(bus), i;
>  	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);
> 
> -	return !strcmp(d + n, tmp);
> +	for (i = 0; i < ARRAY_SIZE(c); i++) {
> +		/* the rest must be exact matching */
> +		snprintf(tmp, sizeof(tmp), "-%s:00", c[i]);
> +
> +		if (!strcmp(d + n, tmp))
> +			return 1;
> +	}
> +
> +	return 0;
>  }
> 
>  static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const
> char *bus, @@ -6824,24 +6835,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 +6892,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 */
> diff --git a/sound/pci/hda/tas2781_hda_i2c.c
> b/sound/pci/hda/tas2781_hda_i2c.c index fb80280293..8493952305 100644
> --- a/sound/pci/hda/tas2781_hda_i2c.c
> +++ b/sound/pci/hda/tas2781_hda_i2c.c
> @@ -65,6 +65,16 @@ enum calib_data {
>  	CALIB_MAX
>  };
> 
> +/* TIAS2781 is the unofficial ACPI id, but widely used in current devices.
> + * TXNW2781 is the official ACPI id, and will be used in the new devices.
> + */
> +static const struct acpi_device_id tas2781_acpi_hda_match[] = {
> +	{"TIAS2781", 0 },
> +	{"TXNW2781", 1 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
> +
>  static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)  {
>  	struct tasdevice_priv *tas_priv = data; @@ -644,20 +654,23 @@ static
> void tas2781_hda_remove(struct device *dev)  static int
> tas2781_hda_i2c_probe(struct i2c_client *clt)  {
>  	struct tasdevice_priv *tas_priv;
> -	const char *device_name;
> -	int ret;
> +	int ret, i;
> 
> -	if (strstr(dev_name(&clt->dev), "TIAS2781"))
> -		device_name = "TIAS2781";
> -	else
> -		return -ENODEV;
> +	/* Check TIAS2781 or TXNW2781 */
> +	for (i = 0; i < ARRAY_SIZE(tas2781_acpi_hda_match); i++)

Why not aviding for loop as it can be retrieved directly
using i2c_get_match_data()?

Update the ACPI/ID table to use pointer to the device_name
as data in the table.

Then,

device_name = i2c_get_match_data(client);
if (!device_name && strstr(dev_name(&clt->dev), device_name)))
	return dev_err_probe(tas_priv->dev, -ENODEV,
	"Device not available\n");

Cheers,
Biju

> +static const struct acpi_device_id tas2781_acpi_hda_match[] = {
> +	{"TIAS2781", 0 },
> +	{"TXNW2781", 1 },
> +	{}
> +};


> +		if (strstr(dev_name(&clt->dev), tas2781_acpi_hda_match[i].id))
> +			break;


> +
> +	if (i == ARRAY_SIZE(tas2781_acpi_hda_match))
> +		return dev_err_probe(tas_priv->dev, -ENODEV,
> +			"Device not available\n");
> 
>  	tas_priv = tasdevice_kzalloc(clt);
>  	if (!tas_priv)
>  		return -ENOMEM;
> 
>  	tas_priv->irq_info.irq = clt->irq;
> -	ret = tas2781_read_acpi(tas_priv, device_name);
> +	ret = tas2781_read_acpi(tas_priv, tas2781_acpi_hda_match[i].id);
>  	if (ret)
>  		return dev_err_probe(tas_priv->dev, ret,
>  			"Platform not supported\n");
> @@ -822,12 +835,6 @@ static const struct i2c_device_id tas2781_hda_i2c_id[]
> = {
>  	{}
>  };
> 
> -static const struct acpi_device_id tas2781_acpi_hda_match[] = {
> -	{"TIAS2781", 0 },
> -	{}
> -};
> -MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
> -
>  static struct i2c_driver tas2781_hda_i2c_driver = {
>  	.driver = {
>  		.name		= "tas2781-hda",
> --
> 2.34.1
Andy Shevchenko Sept. 11, 2023, 10:23 a.m. UTC | #2
On Sun, Sep 10, 2023 at 03:27:03PM +0800, Shenghao Ding wrote:
> Support ACPI_ID both TXNW2781 and TIAS2781, TXNW2781 is the only one legal
> ACPI ID, TIAS2781 is the widely-used ACPI ID named by our customers, so
> far it is not registered. 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.

...

> +/* TIAS2781 is the unofficial ACPI id, but widely used in current devices.
> + * TXNW2781 is the official ACPI id, and will be used in the new devices.
> + * Check TIAS2781 or TXNW2781
> + */

/*
 * This style is only for networking.
 * please use one as in this example.
 */

...

> +	const char c[][10] = { "TXNW2781", "TIAS2781" };

Can you put this to the ACPI device ID table, it will be easier to use it with
some other acpi_*() APIs?
That table might need a comment why it has no MODULE_DEVICE_TABLE() with it.

...

> +	int n = strlen(bus), i;

>  
> -	if (strncmp(d, p->bus, n))
> +	if (strncmp(d, bus, n))
>  		return 0;

It means you need to use str_has_prefix().

...

> +	for (i = 0; i < ARRAY_SIZE(c); i++) {
> +		/* the rest must be exact matching */
> +		snprintf(tmp, sizeof(tmp), "-%s:00", c[i]);
> +
> +		if (!strcmp(d + n, tmp))
> +			return 1;
> +	}

This can be done differently.
You are comparing the instance of the device to the actual id, right?
We have ACPI match APIs for that. Have you tried to look at them?

...

> +/* TIAS2781 is the unofficial ACPI id, but widely used in current devices.
> + * TXNW2781 is the official ACPI id, and will be used in the new devices.
> + */
> +static const struct acpi_device_id tas2781_acpi_hda_match[] = {
> +	{"TIAS2781", 0 },
> +	{"TXNW2781", 1 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);

So, besides the style of the comment, why do you have two different data
structures for the same? Can you find a common place and deduplicate it?

...

> -MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);

Ah, I see now, it's used for probing. Please, don't move it. The hid is
available via device pointer.

...

This patch requires much more work, and esp. be redesigned to use proper
ACPI APIs.
kernel test robot Sept. 12, 2023, 4:11 p.m. UTC | #3
Hi Shenghao,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tiwai-sound/for-next]
[also build test WARNING on tiwai-sound/for-linus linus/master v6.6-rc1 next-20230912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Shenghao-Ding/ALSA-hda-tas2781-Support-ACPI_ID-both-TXNW2781-and-TIAS2781/20230910-153010
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git for-next
patch link:    https://lore.kernel.org/r/20230910072704.1359-1-shenghao-ding%40ti.com
patch subject: [PATCH v1] ALSA: hda/tas2781: Support ACPI_ID both TXNW2781 and TIAS2781
config: i386-buildonly-randconfig-003-20230912 (https://download.01.org/0day-ci/archive/20230912/202309122358.aEcJnIUJ-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230912/202309122358.aEcJnIUJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309122358.aEcJnIUJ-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> sound/pci/hda/tas2781_hda_i2c.c:665:24: warning: variable 'tas_priv' is uninitialized when used here [-Wuninitialized]
                   return dev_err_probe(tas_priv->dev, -ENODEV,
                                        ^~~~~~~~
   sound/pci/hda/tas2781_hda_i2c.c:656:33: note: initialize the variable 'tas_priv' to silence this warning
           struct tasdevice_priv *tas_priv;
                                          ^
                                           = NULL
   1 warning generated.


vim +/tas_priv +665 sound/pci/hda/tas2781_hda_i2c.c

   653	
   654	static int tas2781_hda_i2c_probe(struct i2c_client *clt)
   655	{
   656		struct tasdevice_priv *tas_priv;
   657		int ret, i;
   658	
   659		/* Check TIAS2781 or TXNW2781 */
   660		for (i = 0; i < ARRAY_SIZE(tas2781_acpi_hda_match); i++)
   661			if (strstr(dev_name(&clt->dev), tas2781_acpi_hda_match[i].id))
   662				break;
   663	
   664		if (i == ARRAY_SIZE(tas2781_acpi_hda_match))
 > 665			return dev_err_probe(tas_priv->dev, -ENODEV,
   666				"Device not available\n");
   667	
   668		tas_priv = tasdevice_kzalloc(clt);
   669		if (!tas_priv)
   670			return -ENOMEM;
   671	
   672		tas_priv->irq_info.irq = clt->irq;
   673		ret = tas2781_read_acpi(tas_priv, tas2781_acpi_hda_match[i].id);
   674		if (ret)
   675			return dev_err_probe(tas_priv->dev, ret,
   676				"Platform not supported\n");
   677	
   678		ret = tasdevice_init(tas_priv);
   679		if (ret)
   680			goto err;
   681	
   682		pm_runtime_set_autosuspend_delay(tas_priv->dev, 3000);
   683		pm_runtime_use_autosuspend(tas_priv->dev);
   684		pm_runtime_mark_last_busy(tas_priv->dev);
   685		pm_runtime_set_active(tas_priv->dev);
   686		pm_runtime_get_noresume(tas_priv->dev);
   687		pm_runtime_enable(tas_priv->dev);
   688	
   689		pm_runtime_put_autosuspend(tas_priv->dev);
   690	
   691		ret = component_add(tas_priv->dev, &tas2781_hda_comp_ops);
   692		if (ret) {
   693			dev_err(tas_priv->dev, "Register component failed: %d\n", ret);
   694			pm_runtime_disable(tas_priv->dev);
   695			goto err;
   696		}
   697	
   698		tas2781_reset(tas_priv);
   699	err:
   700		if (ret)
   701			tas2781_hda_remove(&clt->dev);
   702		return ret;
   703	}
   704
diff mbox series

Patch

diff --git a/sound/pci/hda/patch_realtek.c b/sound/pci/hda/patch_realtek.c
index b7e78bfcff..6dae58a8ef 100644
--- a/sound/pci/hda/patch_realtek.c
+++ b/sound/pci/hda/patch_realtek.c
@@ -6770,24 +6770,35 @@  static int comp_match_cs35l41_dev_name(struct device *dev, void *data)
 	return !strcmp(d + n, tmp);
 }
 
+/* TIAS2781 is the unofficial ACPI id, but widely used in current devices.
+ * TXNW2781 is the official ACPI id, and will be used in the new devices.
+ * Check TIAS2781 or TXNW2781
+ */
 static int comp_match_tas2781_dev_name(struct device *dev,
 	void *data)
 {
-	struct scodec_dev_name *p = data;
+	const char c[][10] = { "TXNW2781", "TIAS2781" };
 	const char *d = dev_name(dev);
-	int n = strlen(p->bus);
+	const char *bus = data;
+	int n = strlen(bus), i;
 	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);
 
-	return !strcmp(d + n, tmp);
+	for (i = 0; i < ARRAY_SIZE(c); i++) {
+		/* the rest must be exact matching */
+		snprintf(tmp, sizeof(tmp), "-%s:00", c[i]);
+
+		if (!strcmp(d + n, tmp))
+			return 1;
+	}
+
+	return 0;
 }
 
 static void cs35l41_generic_fixup(struct hda_codec *cdc, int action, const char *bus,
@@ -6824,24 +6835,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 +6892,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 */
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index fb80280293..8493952305 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -65,6 +65,16 @@  enum calib_data {
 	CALIB_MAX
 };
 
+/* TIAS2781 is the unofficial ACPI id, but widely used in current devices.
+ * TXNW2781 is the official ACPI id, and will be used in the new devices.
+ */
+static const struct acpi_device_id tas2781_acpi_hda_match[] = {
+	{"TIAS2781", 0 },
+	{"TXNW2781", 1 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
+
 static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)
 {
 	struct tasdevice_priv *tas_priv = data;
@@ -644,20 +654,23 @@  static void tas2781_hda_remove(struct device *dev)
 static int tas2781_hda_i2c_probe(struct i2c_client *clt)
 {
 	struct tasdevice_priv *tas_priv;
-	const char *device_name;
-	int ret;
+	int ret, i;
 
-	if (strstr(dev_name(&clt->dev), "TIAS2781"))
-		device_name = "TIAS2781";
-	else
-		return -ENODEV;
+	/* Check TIAS2781 or TXNW2781 */
+	for (i = 0; i < ARRAY_SIZE(tas2781_acpi_hda_match); i++)
+		if (strstr(dev_name(&clt->dev), tas2781_acpi_hda_match[i].id))
+			break;
+
+	if (i == ARRAY_SIZE(tas2781_acpi_hda_match))
+		return dev_err_probe(tas_priv->dev, -ENODEV,
+			"Device not available\n");
 
 	tas_priv = tasdevice_kzalloc(clt);
 	if (!tas_priv)
 		return -ENOMEM;
 
 	tas_priv->irq_info.irq = clt->irq;
-	ret = tas2781_read_acpi(tas_priv, device_name);
+	ret = tas2781_read_acpi(tas_priv, tas2781_acpi_hda_match[i].id);
 	if (ret)
 		return dev_err_probe(tas_priv->dev, ret,
 			"Platform not supported\n");
@@ -822,12 +835,6 @@  static const struct i2c_device_id tas2781_hda_i2c_id[] = {
 	{}
 };
 
-static const struct acpi_device_id tas2781_acpi_hda_match[] = {
-	{"TIAS2781", 0 },
-	{}
-};
-MODULE_DEVICE_TABLE(acpi, tas2781_acpi_hda_match);
-
 static struct i2c_driver tas2781_hda_i2c_driver = {
 	.driver = {
 		.name		= "tas2781-hda",