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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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.

-- 
With Best Regards,
Andy Shevchenko