Add speaker id check by gpio in ACPI for ASUS projects.
In other vendors, speaker id was checked by BIOS, and was applied in
last bit of subsys id, so we can load corresponding firmware binary file
for its speaker by subsys id.
But in ASUS project, the firmware binary name will be appended an extra
number to tell the speakers from different vendors. And this single digit
come from gpio level of speaker id in BIOS.

Signed-off-by: Baojun Xu <baojun.xu@ti.com>
---
v2:
 - Change ASUS id from 0x10430000 to "1043".
 - Move gpio setting to tas2781_read_acpi() from probe.
 - Remove interrupt gpio in acpi_gpio_mapping.
 - Add sub and physdev in tas2781_read_acpi() for subsys id read.
 - Add debug log for get acpi resource failed.
 - Return error if get resource or subsys id failed.
 - Return error if get gpio fail for speaker id with ASUS projects.
 - Change fixed buffer lengh to sizeof().
 - Change bits calculator to lower_16_bits().
 - Remove unnecessary empty line in tas2781_hda_i2c_probe().
---
 include/sound/tas2781.h         |  3 ++
 sound/pci/hda/tas2781_hda_i2c.c | 62 +++++++++++++++++++++++++++++----
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/include/sound/tas2781.h b/include/sound/tas2781.h
index 8cd6da0480b7..621ae485d3cb 100644
--- a/include/sound/tas2781.h
+++ b/include/sound/tas2781.h
@@ -107,6 +107,8 @@
 #define TASDEVICE_CMD_DELAY		0x3
 #define TASDEVICE_CMD_FIELD_W		0x4
 
+#define TAS2781_ASUS_ID			"1043"
+
 enum audio_device {
 	TAS2563,
 	TAS2781,
@@ -156,6 +158,7 @@ struct tasdevice_priv {
 	struct tasdevice_rca rcabin;
 	struct calidata cali_data;
 	struct tasdevice_fw *fmw;
+	struct gpio_desc *speaker_id;
 	struct gpio_desc *reset;
 	struct mutex codec_lock;
 	struct regmap *regmap;
diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
index 370d847517f9..4b238bf7e3c1 100644
--- a/sound/pci/hda/tas2781_hda_i2c.c
+++ b/sound/pci/hda/tas2781_hda_i2c.c
@@ -110,10 +110,19 @@ static int tas2781_get_i2c_res(struct acpi_resource *ares, void *data)
 	return 1;
 }
 
+static const struct acpi_gpio_params speakerid_gpios = { 0, 0, false };
+
+static const struct acpi_gpio_mapping tas2781_speaker_id_gpios[] = {
+	{ "speakerid-gpios", &speakerid_gpios, 1 },
+	{ }
+};
+
 static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
 {
 	struct acpi_device *adev;
+	struct device *physdev;
 	LIST_HEAD(resources);
+	const char *sub;
 	int ret;
 
 	adev = acpi_dev_get_first_match_dev(hid, NULL, -1);
@@ -122,19 +131,44 @@ static int tas2781_read_acpi(struct tasdevice_priv *p, const char *hid)
 			"Failed to find an ACPI device for %s\n", hid);
 		return -ENODEV;
 	}
-
+	physdev = get_device(acpi_get_first_physical_node(adev));
 	ret = acpi_dev_get_resources(adev, &resources, tas2781_get_i2c_res, p);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err(p->dev, "Failed to get ACPI resource.\n");
 		goto err;
+	}
+	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
+	if (IS_ERR(sub)) {
+		dev_err(p->dev, "Failed to get SUBSYS ID.\n");
+		goto err;
+	}
+	// Speaker id was needed for ASUS projects.
+	if (strstr(sub, TAS2781_ASUS_ID)) {
+		ret = devm_acpi_dev_add_driver_gpios(p->dev,
+			tas2781_speaker_id_gpios);
+		if (ret) {
+			dev_err(p->dev, "Unable to add GPIO.\n");
+			goto err;
+		}
+		p->speaker_id = devm_gpiod_get(p->dev, "speakerid", GPIOD_IN);
+		if (IS_ERR(p->speaker_id)) {
+			dev_err(p->dev, "Failed to get Speaker id.\n");
+			goto err;
+		}
+	} else {
+		p->speaker_id = NULL;
+	}
 
 	acpi_dev_free_resource_list(&resources);
 	strscpy(p->dev_name, hid, sizeof(p->dev_name));
+	put_device(physdev);
 	acpi_dev_put(adev);
 
 	return 0;
 
 err:
 	dev_err(p->dev, "read acpi error, ret: %d\n", ret);
+	put_device(physdev);
 	acpi_dev_put(adev);
 
 	return ret;
@@ -615,7 +649,7 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
 	struct tasdevice_priv *tas_priv = context;
 	struct tas2781_hda *tas_hda = dev_get_drvdata(tas_priv->dev);
 	struct hda_codec *codec = tas_priv->codec;
-	int i, ret;
+	int i, ret, spk_id;
 
 	pm_runtime_get_sync(tas_priv->dev);
 	mutex_lock(&tas_priv->codec_lock);
@@ -648,8 +682,25 @@ static void tasdev_fw_ready(const struct firmware *fmw, void *context)
 	tasdevice_dsp_remove(tas_priv);
 
 	tas_priv->fw_state = TASDEVICE_DSP_FW_PENDING;
-	scnprintf(tas_priv->coef_binaryname, 64, "TAS2XXX%04X.bin",
-		codec->core.subsystem_id & 0xffff);
+	if (tas_priv->speaker_id != NULL) {
+		// Speaker id need to be checked for ASUS only.
+		spk_id = gpiod_get_value(tas_priv->speaker_id);
+		if (spk_id < 0 || spk_id > 1) {
+			// Speaker id is not valid, use default.
+			dev_dbg(tas_priv->dev, "Wrong spk_id = %d\n", spk_id);
+			spk_id = 0;
+		}
+		scnprintf(tas_priv->coef_binaryname,
+			  sizeof(tas_priv->coef_binaryname),
+			  "TAS2XXX%04X%01d.bin",
+			  lower_16_bits(codec->core.subsystem_id),
+			  spk_id);
+	} else {
+		scnprintf(tas_priv->coef_binaryname,
+			  sizeof(tas_priv->coef_binaryname),
+			  "TAS2XXX%04X.bin",
+			  lower_16_bits(codec->core.subsystem_id));
+	}
 	ret = tasdevice_dsp_parser(tas_priv);
 	if (ret) {
 		dev_err(tas_priv->dev, "dspfw load %s error\n",
@@ -793,7 +844,6 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
 	const char *device_name;
 	int ret;
 
-
 	tas_hda = devm_kzalloc(&clt->dev, sizeof(*tas_hda), GFP_KERNEL);
 	if (!tas_hda)
 		return -ENOMEM;
-- 
2.43.0

On Fri, Oct 18, 2024 at 03:11:18PM +0800, Baojun Xu wrote:
> Add speaker id check by gpio in ACPI for ASUS projects.
> In other vendors, speaker id was checked by BIOS, and was applied in
> last bit of subsys id, so we can load corresponding firmware binary file
> for its speaker by subsys id.
> But in ASUS project, the firmware binary name will be appended an extra
> number to tell the speakers from different vendors. And this single digit
> come from gpio level of speaker id in BIOS.

...

> +	sub = acpi_get_subsystem_id(ACPI_HANDLE(physdev));
> +	if (IS_ERR(sub)) {
> +		dev_err(p->dev, "Failed to get SUBSYS ID.\n");
> +		goto err;
> +	}
> +	// Speaker id was needed for ASUS projects.
> +	if (strstr(sub, TAS2781_ASUS_ID)) {

strstr() is wrong. You can get 1043 in the middle of the bigger number,
or even text.

> +		ret = devm_acpi_dev_add_driver_gpios(p->dev,
> +			tas2781_speaker_id_gpios);
> +		if (ret) {
> +			dev_err(p->dev, "Unable to add GPIO.\n");

> +			goto err;

No need. If this fails, the below most likely fail as well.

> +		}
> +		p->speaker_id = devm_gpiod_get(p->dev, "speakerid", GPIOD_IN);
> +		if (IS_ERR(p->speaker_id)) {
> +			dev_err(p->dev, "Failed to get Speaker id.\n");
> +			goto err;
> +		}
> +	} else {
> +		p->speaker_id = NULL;
> +	}

...

> +	if (tas_priv->speaker_id != NULL) {
> +		// Speaker id need to be checked for ASUS only.
> +		spk_id = gpiod_get_value(tas_priv->speaker_id);
> +		if (spk_id < 0 || spk_id > 1) {

When "> 1" is possible? Isn't it a dead code?

> +			// Speaker id is not valid, use default.
> +			dev_dbg(tas_priv->dev, "Wrong spk_id = %d\n", spk_id);
> +			spk_id = 0;
> +		}
> +		scnprintf(tas_priv->coef_binaryname,

Why 'c' variant? You do not check the return value anyway. So, what's the point?

> +			  sizeof(tas_priv->coef_binaryname),
> +			  "TAS2XXX%04X%01d.bin",
> +			  lower_16_bits(codec->core.subsystem_id),
> +			  spk_id);
> +	} else {
> +		scnprintf(tas_priv->coef_binaryname,

Ditto.

> +			  sizeof(tas_priv->coef_binaryname),
> +			  "TAS2XXX%04X.bin",
> +			  lower_16_bits(codec->core.subsystem_id));
> +	}

...

> @@ -793,7 +844,6 @@ static int tas2781_hda_i2c_probe(struct i2c_client *clt)
>  	const char *device_name;
>  	int ret;
>  
> -
>  	tas_hda = devm_kzalloc(&clt->dev, sizeof(*tas_hda), GFP_KERNEL);
>  	if (!tas_hda)
>  		return -ENOMEM;

Stray change.

-- 
With Best Regards,
Andy Shevchenko

On Fri, 18 Oct 2024 14:31:05 +0200,
Andy Shevchenko wrote:
> 
> On Fri, Oct 18, 2024 at 03:11:18PM +0800, Baojun Xu wrote:
> > +			// Speaker id is not valid, use default.
> > +			dev_dbg(tas_priv->dev, "Wrong spk_id = %d\n", spk_id);
> > +			spk_id = 0;
> > +		}
> > +		scnprintf(tas_priv->coef_binaryname,
> 
> Why 'c' variant? You do not check the return value anyway. So, what's the point?

There is a difference between snprintf() and scnprintf().
With W=1, the compiler (at least the recent gcc version) will warn you
when the string truncation may happen in the former case while not
complaining for the latter.
So, when the truncation is intentional and acceptable (that's
certainly most cases), the use of scnprintf() will result in less
warnings.


Takashi

On Mon, Oct 21, 2024 at 09:19:19AM +0200, Takashi Iwai wrote:
> On Fri, 18 Oct 2024 14:31:05 +0200,
> Andy Shevchenko wrote:
> > On Fri, Oct 18, 2024 at 03:11:18PM +0800, Baojun Xu wrote:

...

> > > +			// Speaker id is not valid, use default.
> > > +			dev_dbg(tas_priv->dev, "Wrong spk_id = %d\n", spk_id);
> > > +			spk_id = 0;
> > > +		}
> > > +		scnprintf(tas_priv->coef_binaryname,
> > 
> > Why 'c' variant? You do not check the return value anyway. So, what's the point?
> 
> There is a difference between snprintf() and scnprintf().
> With W=1, the compiler (at least the recent gcc version) will warn you
> when the string truncation may happen in the former case while not
> complaining for the latter.
> So, when the truncation is intentional and acceptable (that's
> certainly most cases), the use of scnprintf() will result in less
> warnings.

Yes, which is a papering over the potential problem, right?
I agree that in this case it might be not critical or even
practical to check for an error, but in general the whole lot
of s*nprintf() should be used with this is in mind.

-- 
With Best Regards,
Andy Shevchenko