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

[Biju Das]

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