Re: [PATCH v3 2/2] ALSA: hda/tas2781: Add tas2781 HDA driver

[Takashi Iwai]

On Fri, 18 Aug 2023 19:01:16 +0200,
Andy Shevchenko wrote:
> 
> On Fri, Aug 18, 2023 at 11:00:34AM -0500, Pierre-Louis Bossart wrote:
> 
> ...
> 
> > > +static int comp_match_tas2781_dev_name(struct device *dev,
> > > +	void *data)
> > > +{
> > > +	struct scodec_dev_name *p = data;
> > > +	const char *d = dev_name(dev);
> > > +	int n = strlen(p->bus);
> > > +	char tmp[32];
> > > +
> > > +	/* check the bus name */
> > > +	if (strncmp(d, p->bus, n))
> > > +		return 0;
> 
> > > +	/* skip the bus number */
> > > +	if (isdigit(d[n]))
> > > +		n++;
> 
> Why do you think it can't be two or more digits?
> 
> > > +	/* the rest must be exact matching */
> > > +	snprintf(tmp, sizeof(tmp), "-%s:00", p->hid);
> > 
> > ACPI can sometimes add :01 suffixes, this looks like the re-invention of
> > an ACPI helper?
> > 
> > Adding Andy for the ACPI review.
> > 
> > > +	return !strcmp(d + n, tmp);
> > > +}
> 
> Yes, this looks like reinventing a wheel.
> Just compare dev_name() against what is in p->....

Note that comp_match_tas7281_dev_name() is a copy of
comp_patch_cs35l41_dev_name() and it was implemented in a hackish way
to be applicable to both I2C and SPI device names that have slightly
different naming rules.

> ...
> 
> > > +static void tas2781_fixup_i2c(struct hda_codec *cdc,
> > > +	const struct hda_fixup *fix, int action)
> > > +{
> > > +	 tas2781_generic_fixup(cdc, action, "i2c", "TIAS2781");
> > 
> > TI ACPI ID is TXNW
> > 
> > https://uefi.org/ACPI_ID_List?search=TEXAS
> > 
> > There's also a PNP ID PXN
> > 
> > https://uefi.org/PNP_ID_List?search=TEXAS
> > 
> > "TIAS" looks like an invented identifier. It's not uncommon but should
> > be recorded with a comment if I am not mistaken.
> > 
> > > +}
> 
> Thank you, but actually it's a strong NAK to this even with the comment.
> We have to teach people to follow the specification (may be even hard way).
> 
> So where did you get the ill-formed ACPI ID?
> Is Texas Instrument aware of this?
> Can we have a confirmation letter from TI for this ID, please?

This is used already for products that have been long in the market,
so it's way too late to correct it, I'm afraid.

What we can do is to get the confirmation from TI, complain it, and
some verbose comment in the code, indeed.


thanks,

Takashi